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: Refactor lib
and expression for related packages
#85324
Conversation
''; | ||
}); | ||
|
||
patchNpmElm = pkg: |
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 pretty stupid name. Ideas are welcome.
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.
patchNodeModules
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 specific to elm
package as a npm dependecy. Essentially just this https://www.npmjs.com/search?q=elm. I think calling it nodeModules
might be too vague.
what about patchNodeElm
?
6cdfbf0
to
858dd35
Compare
@domenkozar please have a look. I think I've managed to remove the biggest hacks from recent additions to elm tooling. |
@GrahamcOfBorg build elmPackages.elm-coverage |
@GrahamcOfBorg build elmPackages.create-elm-app |
lib
and expression for related packageslib
and expression for related packages
marking as WIP due to FAILURE of OfBorg. I probably disabled sandboxing on my machine and forgot about it, sorry. |
858dd35
to
c532aa3
Compare
@GrahamcOfBorg build elmPackages.elm-coverage |
@GrahamcOfBorg build elmPackages |
Seems to fail on darwin:
|
c532aa3
to
4e59ec6
Compare
For the record failure on darwin seems to be related to gyp. OfBorg log:
Which might be related to #84797 |
I think it might be time to update to node 12 anyway (newest LTS). Elm compiler itself uses |
I'm too lazy to fiddle with all the OS updates and then installation of nix on Catalina but please try this again and if it fails I'll do it over the weekend on my old macbook. |
@GrahamcOfBorg build elmPackages |
It seems like it wants to run |
fbf9bcb
to
b9e9eff
Compare
@domenkozar sorry it took so long for me to revisit it. I got a new job and wanted to get on speed with work related stuff. I just rebased this PR and tested it on both NixOS and MacOS and it seems to build now on both machines. |
@GrahamcOfBorg build elmPackages |
lib
and expression for related packageslib
and expression for related packages
🙌 seems MacOS is passing now. It must have been something unrelated to this. |
Refactoring
elmPackages.lib
and related packages:elmPackages.coverage
- now reuses new lib function to patch npm instalation of elmelmPacages.create-elm-app
- same as above + there is a fix for bug where elm-create-app tries to download elm binary when certain command is run (which fails due to read only FS)I recomend looking at new file for review (diff is a bit hard to read)
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)