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
elmPackages.elm-coverage: init at 0.0.3 #82779
Conversation
@@ -1,4 +1,4 @@ | |||
# This file has been generated by node2nix 1.8.0. Do not edit! | |||
# This file has been generated by node2nix 1.7.0. Do not edit! |
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.
I'm a bit behind on my machine because I require Haskell's dbus package for my setup to work thus there is this downgrade.
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.
node2nix should come from nixpkgs, but out of scope for this issue :)
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.
Agree. I originally copied this script from some other place but as you can see it uses nix-shell -i bash -p nodePackages.node2nix
so system's version of <nixpkgs>
. I'm not sure how can this be fixed at the moment but it seems to me it would be better if this used node2nix
from repository itself.
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.
something like:
ROOT="$(realpath "$(dirname -- "$(readlink -f -- "${BASH_SOURCE[0]}")")"/../../../../..)"
$(nix-build $ROOT -A nodePacakges.node2nix --no-out-link)/bin/node2nix ...
|
||
# This should not be needed (thanks to binwrap* being nooped) but for some reason it still needs to be done | ||
# in case of just this package | ||
sed 's/\"install\".*/\"install\":\"echo no-op\",/g' --in-place node_modules/elmi-to-json/package.json |
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.
This is the biggest hack and I can't really explain why is needed.
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.
Probably since it's needed for elm-test, which is installer here as a dependency?
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.
That's the case of elm-verify-examples
as well and in that case the writeScriptBin
works as expected. I have a bad feeling that this somewhat depends on how PATH
is being defined and potentially in case of this package the version from node_modules/.bin
takes precedence over the script. That would be very unfortunate behavior. If that's the case we need a better way to patch this.
binwrap
package also provides library functions which can (and are) called from packages code (and hopefully should just check the binaries are already present and do nothing). I think if we would try to avoid installing that package in any way we would need to do a lot of patching in source files. One thing that comes to mind is to let npm install the library but remove its bin files.
Some part of me always wants to start crying every time I start considering doing hacks like this but so far I don't have idea about how to solve these issues in more robust way.
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.
Yeah most probably need to check $PATH precedence for node_modules, but I think this is good enough to work.
"elm-test", | ||
"elm-upgrade", | ||
"elm-verify-examples", | ||
"elm-xref" |
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 a bit of sorting
2/2 of elm-coverage packaging.
This indirectly depends on this fix of elm-test
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)cc @domenkozar