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

elmPackages.elm-coverage: init at 0.0.3 #82779

Merged
merged 1 commit into from Mar 17, 2020

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Mar 17, 2020

2/2 of elm-coverage packaging.

This indirectly depends on this fix of elm-test

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

cc @domenkozar

@@ -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!
Copy link
Member Author

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.

Copy link
Member

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 :)

Copy link
Member Author

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.

Copy link
Member

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
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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"
Copy link
Member Author

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

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

2 participants