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-test,elm-analyse..}: tooling additions and fixes #62899

Merged
merged 0 commits into from Jun 18, 2019

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Jun 9, 2019

tl;dr

This adds missing and fixes some existing tooling for elm-lang ecosystem

Motivation

  • nodePackages.elm-test is broken
  • many commonly used tools are missing
  • many tools fail to install via npm due to the binwrap issue.

This PR is based on nix-elm-tools

detailed info can be found in commit message

few additions remarks which can become dated:

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md. [I hope]

@turboMaCk turboMaCk changed the title Elm tooling initial version [WIP] Elm-lang tooling Jun 9, 2019
@turboMaCk turboMaCk force-pushed the elm-lang-packages branch 2 times, most recently from 9cbc827 to 5e34439 Compare June 9, 2019 18:17
@turboMaCk turboMaCk marked this pull request as ready for review June 9, 2019 18:18
@turboMaCk
Copy link
Member Author

turboMaCk commented Jun 9, 2019

@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/node-packages/node-packages-v10.json 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
Copy link
Member Author

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

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

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@turboMaCk turboMaCk changed the title [WIP] Elm-lang tooling elmPackages: improvements, additions, fixes Jun 9, 2019
Copy link
Member

@BrianHicks BrianHicks left a 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

pkgs/development/compilers/elm/packages/README.md Outdated Show resolved Hide resolved
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;
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

@turboMaCk turboMaCk Jun 10, 2019

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.

Copy link
Member

@domenkozar domenkozar left a 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 :)

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
pkgs/development/node-packages/node-packages-v10.json Outdated Show resolved Hide resolved
@turboMaCk turboMaCk force-pushed the elm-lang-packages branch 2 times, most recently from e93c9eb to 2bf586c Compare June 10, 2019 20:44
@turboMaCk turboMaCk changed the title elmPackages: improvements, additions, fixes elmPackages.{elm-test,elm-analyse..}: tooling additions and fixes Jun 12, 2019
@turboMaCk turboMaCk force-pushed the elm-lang-packages branch 2 times, most recently from 4d559c2 to 6885186 Compare June 12, 2019 18:40
"elm-test",
"elm-verify-examples",
"elm-doc-preview",
{ "elm-analyse": "0.16.3" },
Copy link
Member Author

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

@turboMaCk
Copy link
Member Author

🤔 not sure what happens with this

@worldofpeace
Copy link
Contributor

I don't recall merging this... And it says 0 commits were merged so it looks like GitHub was maybe having a hard day 🤣

@turboMaCk
Copy link
Member Author

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.

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

5 participants