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

Add example for nodejs toolchain with npm_install repository rule #309

Merged
merged 10 commits into from
Feb 8, 2023

Conversation

kczulko
Copy link
Contributor

@kczulko kczulko commented Jan 26, 2023

While applying latest rules_nodejs(5.8.0) to one of the projects I'm working on, I saw there's no longer an option to pass vendored_node to npm_install rule.

Following this "migration guide" it was found out (thx @avdv @aherrmann ) that using proper npm_install::node_reporitory value does the job.

However there's an issue of suffixing toolchain names which rules_nodejs does - see nixpkgs-nodejs_linux_amd64 as one of the names. E.g. now macos build fails with:

Error in path: Unable to load package for @nixpkgs-nodejs_darwin_amd64//:bin/node: The repository '@nixpkgs-nodejs_darwin_amd64' could not be resolved: Repository '@nixpkgs-nodejs_darwin_amd64' is not defined

Probably we have to mimic the behavior that rules_haskell does (thx @ylecornec )

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kczulko Thank you for raising and investigating this!

Probably we have to mimic the behavior that rules_haskell does (thx @ylecornec )

The link points to this PR again. Did you mean this?

examples/toolchains/nodejs/WORKSPACE Outdated Show resolved Hide resolved
examples/toolchains/nodejs/WORKSPACE Show resolved Hide resolved
toolchains/nodejs/nodejs.bzl Outdated Show resolved Hide resolved
@kczulko
Copy link
Contributor Author

kczulko commented Feb 3, 2023

@aherrmann @avdv
I think it's ready for the next review round. I think I am also missing some docs about this new way of configuring rules_nodejs under nix. I added rules_nodejs_core as explicit dependency of this project. Otherwise there were some "circular" (?) dependency issues which lead to a failures.

Copy link
Member

@aherrmann aherrmann 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! That looks good!

Otherwise there were some "circular" (?) dependency issues which lead to a failures.

Bazel reports missing external repositories like that sometimes.

I think I am also missing some docs about this new way of configuring rules_nodejs under nix.

A good place to add them would be the docstring of the new nixpkgs_nodejs_configure_platforms and also a docstring in nixpkgs_nodejs_configure pointing to it. (Also a note in the CHANGELOG). You can look at other macros for the format of docstrings.

toolchains/nodejs/nodejs.bzl Show resolved Hide resolved
examples/toolchains/nodejs/WORKSPACE Show resolved Hide resolved
toolchains/nodejs/BUILD.bazel Show resolved Hide resolved
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank @kczulko for pushing this through, great work!
Only one small comment. Feel free to apply and then add the merge-queue label.

examples/toolchains/nodejs/shell.nix Outdated Show resolved Hide resolved
@kczulko kczulko added the merge-queue merge on green CI label Feb 8, 2023
@mergify mergify bot merged commit 6563f78 into tweag:master Feb 8, 2023
@mergify mergify bot removed the merge-queue merge on green CI label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants