gmic-qt: build from Git source to avoid mutable tarball #312997
Conversation
There was a problem hiding this comment.
Unclear to me why cimg was not needed in buildInputs before, but it is now.
The tarball also includes CImg.h and now it no longer does, hence the need for the build input
I do still hate that the build system requires more than just the header and shared object for libgmic (although it does also use both and still links to it...), but this looks like a reasonable enough hack (I hate having to use the bespoke tarballs anyway... only trouble for gmic package itself is the stdlib has a bootstrap problem so we still have to download that prebuilt header which could change too)
I left one comment but otherwise this looks good, thank you!
| @@ -107,24 +111,6 @@ stdenv.mkDerivation (finalAttrs: { | |||
| gimp-plugin = gimpPlugins.gmic; | |||
| inherit cimg gmic; | |||
| }; | |||
|
|
|||
| updateScript = writeShellScript "gmic-qt-update-script" '' | |||
There was a problem hiding this comment.
We should still have an update script because as-is nix-update and nixpkgs-update/r-ryantm will have a hard time finding updates both due to the lack of GitHub upstream releases (rather than just tags) and the fact that the tags have weird version strings. Maybe something like this?
updateScript = nix-update-script {
extraArgs = [ "--version-regex" "^v\\.(.*)" ];
};Also please remove the now-unused arguments from the top of the file
There was a problem hiding this comment.
Using gitUpdater would be nicer for this, as it is specifically designed for git tags, which this is using for versions.
updateScript = gitUpdater {
rev-prefix = "v.";
};There was a problem hiding this comment.
Both that and nix-update-script are specifically designed for it I guess (nix-update just usually deals with edge cases better), but I'm largely indifferent as long as there is just something there :)
There was a problem hiding this comment.
Actually this is one of the edge cases that gitUpdater does not handle. If I add that updateScript then it tries to "update" it to version 2.2.0 rather than keeping the version at 3.3.5
So please use nix-update-script unless there is an option/argument for gitUpdater to fix that
There was a problem hiding this comment.
Thanks for the review! Pushed amended commit based on feedback.
The previous tarball src sometimes gets mutated (see e.g. [this comment][1]). This was changed from the Git src in fd3e2b4 (see also [upstream discussion][2]). However the delta seems simple; it had error: ``` In file included from /build/source/src/GmicProcessor.cpp:48: /nix/store/jk1dp7v01pisw0flybqwyjg63in6r9fp-gmic-3.3.5-dev/include/gmic.h:191:21: fatal error: gmic.cpp: No such file or directory 191 | #define cimg_plugin "gmic.cpp" ``` workaround this by linking `gmic.cpp` into the expected location as it appears in the tarball. cimg is now needed in buildInputs as it is bundled in the tarball, but not the Git src. Change the updateScript to simpler one that can use the Git tags. This may appear to be downgrading, but this is only because the previous drv used the gmic version, not the gmic-qt version. [1]: NixOS#311734 (comment) [2]: c-koi/gmic-qt#175
amarshall
force-pushed
the
gmic-qt-from-git
branch
from
9998458
to
5678aa0
Compare
The previous tarball src sometimes gets mutated (see e.g. this comment). This was changed from the Git src in fd3e2b4 (see also upstream discussion). However the delta seems simple; it had error:
workaround this by linking
gmic.cppinto the expected location as it appears in the tarball.Remove the now-unneeded updateScript, as it is trivial GitHub tags now.
Unclear to me why cimg was not needed in buildInputs before, but it is now.
This may appear to be downgrading, but this is only because the previous drv used the gmic version, not the gmic-qt version.
Description of changes
Things done
nix-build -A gmic-qt.passthru.tests; tested in GIMP as plugin.nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.