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

Open
wants to merge 1 commit into
base: master
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

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 ];

This comment has been minimized.

@doronbehar

doronbehar Oct 12, 2020
Contributor

How come all of a sudden?

This comment has been minimized.

@oxzi

oxzi Oct 19, 2020
Author Member

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.

This comment has been minimized.

@doronbehar

doronbehar Oct 20, 2020
Contributor

You can add it only if gtk3 is used.

This comment has been minimized.

@oxzi

oxzi Oct 20, 2020
Author Member

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 force-pushed the oxzi:claws-refactoring branch from 3d7ec75 to b82e926 Oct 19, 2020
@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 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.

[ 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" ]
Comment on lines +151 to +178

This comment has been minimized.

@doronbehar

doronbehar Oct 20, 2020
Contributor

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.

This comment has been minimized.

@oxzi

oxzi Oct 20, 2020
Author Member

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

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Nov 27, 2020
Contributor

@oxzi Are you still planning to do this?

This comment has been minimized.

@doronbehar

doronbehar Nov 27, 2020
Contributor

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.