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
elm-instrument: fix build #122329
elm-instrument: fix build #122329
Conversation
1f4cfd4
to
39e8d98
Compare
39e8d98
to
9aadee7
Compare
The pr upstream does not build at the moment since optparse-applicative 0.16 is not available yet on stackage (If I understand correctly, I'm not familiar with haskell package managers). So I've converted the fix to a patch instead for now. |
@@ -14,6 +14,7 @@ mkDerivation { | |||
rev = "63e15bb5ec5f812e248e61b6944189fa4a0aee4e"; | |||
fetchSubmodules = true; | |||
}; | |||
patches = [ ./update-optparse-applicative.patch ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not to use fetchpatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about it, it would allow to query the commit of the PR in the form of a patch I guess? I'll update the PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's preferable because it makes it much easier to track if the patch is still necessary in the future.
9aadee7
to
eb7e994
Compare
(fetchpatch { | ||
name = "update-optparse-applicative.patch"; | ||
url = "https://github.com/mdevlamynck/elm-instrument/commit/c548709d4818aeef315528e842eaf4c5b34b59b4.patch"; | ||
sha256 = "0ln7ik09n3r3hk7jmwwm46kz660mvxfa71120rkbbaib2falfhsc"; | ||
}) | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to keep it consistent with most other places where we need this in nixpkgs - can you please add a comment with url to PR above the call to fetchpatch
? It just makes it ever so slightly simpler to track the status upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR updated :)
eb7e994
to
efa592b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! Thanks a lot @domenkozar this is ready for merge.
@GrahamcOfBorg build elmPackages.elm-instrument elmPackages.elm-coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Result of nixpkgs-review pr 122329 run on x86_64-linux 1
2 packages built:
- elmPackages.elm-coverage
- elmPackages.elm-instrument
Motivation for this change
Fix build of elm-instrument and elm-coverage that depends on it.
ZHF: #122042
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)