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-test,elm-analyse..}: tooling additions and fixes #62899
Conversation
9cbc827
to
5e34439
Compare
@avh4 @BrianHicks and @domenkozar might be interested in reviewing this. Sorry for 🚨 - feel free to ignore if you're busy or not interested. |
pkgs/development/compilers/elm/packages/patches/elmi-to-json.patch
Outdated
Show resolved
Hide resolved
set -eu -o pipefail | ||
|
||
rm -f node-env.nix | ||
node2nix --nodejs-10 -i node-packages.json -o node-packages.nix -c node-composition.nix |
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.
1. It replaces binwrap scripts with noop shell scripts | ||
2. It uses nix for installing the binaries to expected location in `node_modeules` | ||
|
||
Example usage be found in `elm/default.nix`. |
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.
please review my english czenglish
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.
thank you for getting this all in one place! ❤️
I'm not a maintainer here so I can't give an approval or request CI, but since you pinged me as an Elm person it all looks reasonable. 😄
I have only ever submitted one nixpkgs change so I do not know how important this actually is, but the commit message here does not match the prescribed format: https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#submitting-changes
and can be patched using `patchBinwrap` function defined in `packages/patch-binwrap.nix`. | ||
*/ | ||
elm-test = patchBinwrap [elmi-to-json] elmNodePackages.elm-test; | ||
elm-verify-examples = patchBinwrap [elmi-to-json] elmNodePackages.elm-verify-examples; |
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.
Wouldn't it be better to have elm-test and elm-verify-examples use patches on their own source code to remove the dependency on the npm elmi-to-json, instead of having them depend on a elmi-to-json with a patched binwrap?
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.
well, it would be possible for sure. In the end, it's a sort of tradeoff. This solution is likely to be cheaper to maintain going forward I guess.
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.
feel free to submit suggestion as I'm not entirely sure if I understand what you mean fully.
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.
Good work, commented how to get some good practices in place :)
pkgs/development/compilers/elm/packages/patches/elmi-to-json.patch
Outdated
Show resolved
Hide resolved
e93c9eb
to
2bf586c
Compare
4d559c2
to
6885186
Compare
"elm-test", | ||
"elm-verify-examples", | ||
"elm-doc-preview", | ||
{ "elm-analyse": "0.16.3" }, |
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.
possible fix for 0.16.4 stil4m/elm-analyse#208
6885186
to
a1dd419
Compare
🤔 not sure what happens with this |
I don't recall merging this... And it says |
I think it was my screwup and potentially even GitHub issue (not really sure if an issue or on purpose behavior). Probably a rebase made gh think this is merged. |
tl;dr
This adds missing and fixes some existing tooling for elm-lang ecosystem
Motivation
nodePackages.elm-test
is brokenThis PR is based on nix-elm-tools
detailed info can be found in commit message
few additions remarks which can become dated:
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)