Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

claws-mail{,-gtk3}: 3.17.7->3.17.8 and refactoring #100143

Closed
wants to merge 1 commit into from

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Oct 10, 2020

Motivation for this change

claws-mail{,-gtk3}: major refactoring

Previously, the configurable arguments were neither complete nor named
according to the configure.ac file. Likewise, the values did not
correspond to the defaults, but rather to a personal preference.

This has now been changed to enable the arguments which are enabled in
the configure.ac file. Also the variable names have been adjusted. For
compatibility the old parameters also exist.

Next to the claws-mail package is the "experimental" claws-mail-gtk3
package for the non official gtk3 git branch. This package started as an
almost one-to-one copy of the claws-mail derivation which small
modifications. This package was of course not updated.

This has also been changed so that both packages are built from the same
derivative.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pros use lib.strings.enableFeature, but it's not that suprt important :). Unless, you would want to go all the way, and do something similar to what mpd does:

featureDependencies = {
# Storage plugins
udisks = [ dbus ];
webdav = [ curl expat ];
# Input plugins
curl = [ curl ];
io_uring = [ liburing ];
mms = [ libmms ];
nfs = [ libnfs ];
smbclient = [ samba ];
# Archive support
bzip2 = [ bzip2 ];
zzip = [ zziplib ];
# Decoder plugins
audiofile = [ audiofile ];
faad = [ faad2 ];
ffmpeg = [ ffmpeg ];
flac = [ flac ];
fluidsynth = [ fluidsynth ];
gme = [ game-music-emu ];
mad = [ libmad ];
mikmod = [ libmikmod ];
mpg123 = [ mpg123 ];
opus = [ libopus ];
vorbis = [ libvorbis ];
# Encoder plugins
vorbisenc = [ libvorbis ];
lame = [ lame ];
# Filter plugins
libsamplerate = [ libsamplerate ];
# Output plugins
alsa = [ alsaLib ];
jack = [ libjack2 ];
pulse = [ libpulseaudio ];
shout = [ libshout ];
# Commercial services
qobuz = [ curl libgcrypt yajl ];
soundcloud = [ curl yajl ];
tidal = [ curl yajl ];
# Client support
libmpdclient = [ mpd_clientlib ];
# Tag support
id3tag = [ libid3tag ];
# Misc
dbus = [ dbus ];
expat = [ expat ];
icu = [ icu ];
pcre = [ pcre ];
sqlite = [ sqlite ];
syslog = [ ];
systemd = [ systemd ];
yajl = [ yajl ];
zeroconf = [ avahi dbus ];
};

And use the word after --disable- as the attribute name.

All of this requires more Nix experience which is not mandatory, but is encouraged :). See another example:

https://github.com/doronbehar/nixpkgs/blob/c4ffa921a7ff5ef25f2ab867562fc29d76c622ef/pkgs/applications/radio/gnuradio/default.nix#L50

'';

postPatch = ''
substituteInPlace src/procmime.c \
--subst-var-by MIMEROOTDIR ${shared-mime-info}/share
'';

nativeBuildInputs = [ autoreconfHook pkgconfig wrapGAppsHook python.pkgs.wrapPython ];
nativeBuildInputs = [ autoreconfHook pkgconfig bison flex wrapGAppsHook python.pkgs.wrapPython ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come all of a sudden?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tarballs contain some generated content, like precompiled translations. Those needed to be generated when compiled from git. The old -gtk3 package had those inputs included and they did the job. Thus, I added them to the merged package description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add it only if gtk3 is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we are also sourcing GTK2 from git because reasons.

Update Claws Mail to its latest version and perform a major refactoring.

Previously, the configurable arguments were neither complete nor named
according to the configure.ac file. Likewise, the values did not
correspond to the defaults, but rather to a personal preference.

This has now been changed to enable the arguments which are enabled in
the configure.ac file. Also the variable names have been adjusted. For
compatibility the old parameters also exist.

Next to the claws-mail package is the "experimental" claws-mail-gtk3
package for the non official gtk3 git branch. This package started as an
almost one-to-one copy of the claws-mail derivation which small
modifications. This package was of course not updated.

This has also been changed so that both packages are built from the same
derivative.
@oxzi oxzi changed the title claws-mail{,-gtk3}: major refactoring claws-mail{,-gtk3}: 3.17.7->3.17.8 and refactoring Oct 19, 2020
@oxzi
Copy link
Member Author

oxzi commented Oct 19, 2020

@doronbehar: Thanks a lot for your feedback and please excuse the delay on my side.

I updated the PR to use lib.strings.enableFeature. This resulted in a forced enabling which brought up some errors which were silenced before. For example the GTK3 version requires Python 3 and other Python packages. I adjusted the PR accordingly.

Furthermore, a new Claws release came along the way, which I included in this PR.

Comment on lines +151 to +178
[ enablePluginAcpiNotifier "acpi_notifier-plugin" ]
[ enablePluginAddressKeeper "address_keeper-plugin" ]
[ enablePluginArchive "archive-plugin" ]
[ enablePluginAttRemover "att_remover-plugin" ]
[ enablePluginAttachWarner "attachwarner-plugin" ]
[ enablePluginBogofilter "bogofilter-plugin" ]
[ enablePluginBsfilter "bsfilter-plugin" ]
[ enablePluginClamd "clamd-plugin" ]
[ enablePluginDillo "dillo-plugin" ]
[ enablePluginFetchInfo "fetchinfo-plugin" ]
[ enablePluginLibravatar "libravatar-plugin" ]
[ enablePluginLitehtmlViewer "litehtml_viewer-plugin" ]
[ enablePluginMailmbox "mailmbox-plugin" ]
[ enablePluginManageSieve "managesieve-plugin" ]
[ enablePluginNewMail "newmail-plugin" ]
[ enablePluginNotification "notification-plugin" ]
[ enablePluginPdfViewer "pdf_viewer-plugin" ]
[ enablePluginPerl "perl-plugin" ]
[ enablePluginPython "python-plugin" ]
[ enablePluginPgp "pgpcore-plugin" ]
[ enablePluginPgp "pgpmime-plugin" ]
[ enablePluginPgp "pgpinline-plugin" ]
[ enablePluginRssyl "rssyl-plugin" ]
[ enablePluginSmime "smime-plugin" ]
[ enablePluginSpamassassin "spamassassin-plugin" ]
[ enablePluginSpamReport "spam_report-plugin" ]
[ enablePluginTnefParse "tnef_parse-plugin" ]
[ enablePluginVcalendar "vcalendar-plugin" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get a lot cleaner and nicer with some Nix attributes and functions applied. Start with this up above:

pluginsDeps = {
  acpi_notifier = [];
  archive = [ /*..*/ ];
  att_remover = [];
  # ...
};

Then use mapAttributesToList to map the plugins enabled, to list of configureFlags (with enableFeature) and a list of pluginsDeps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've got a point there. I will take a look at this later / tomorrow. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxzi Are you still planning to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it'd be worth re-reviewing this, for the sake of the update, and revisit in the future the idea at using such Nix attributes..

@oxzi
Copy link
Member Author

oxzi commented Feb 8, 2021

Please excuse this stalled pull request. Both the successive change requests and the fact that I no longer use Claws as my go-to email client have caused me to stop actively working on this pull request. Thus, I am going to close this one. Maybe someone else will keep up this work. Thanks to all the reviewers.

@oxzi oxzi closed this Feb 8, 2021
@oxzi oxzi deleted the claws-refactoring branch February 8, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants