feat: add `node_urls` parameter to bzlmod `toolchain` in the `node` extension

redsun82 commented 4 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features)
I don't think I have the capacity to also add tests for this.
I don't think there's currently any bzlmod documentation, so not sure where I would be adding this.

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature (please, look at the "Scope of the project" section in the README.md file)
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

node_urls can be provided to add nodejs mirrors to the WORKFLOW based setup, but the same can't be done when using bzlmod.

Issue Number: N/A

What is the new behavior?

When using bzlmod, node_urls can be provided to the toolchain tag class in the node extension, behaving just like the WORKFLOW counterpart.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

redsun82 commented 4 months ago

@jbedard thanks for the review.

While going over the code again I figured there was also a problem in the existing conflict check, in that it didn't check a difference on the include_headers setting. I've refactored the check so that it takes that into account and any new attributes we might add in the future. It makes the diff bigger, but I think it's worth it.

jbedard commented 4 months ago

Are there any tests in the WORKSPACE version that can maybe be copied for the bzlmod extension?

redsun82 commented 4 months ago

@jbedard I'll try to take a quick look.

By the way: CI failed on a temporary nodejs.org failure, which is exactly the kind of problem I want to solve with this in my code base with mirrors :slightly_smiling_face: I didn't see any way I could rerun the check, but maybe I didn't look enough. On the other hand if I commit something more I'll get the reruns any way.

criemen commented 2 months ago

@jbedard any chance you could take another look at this PR please?

jbedard commented 2 months ago

Sorry I forgot about this.

If @redsun82 doesn't respond maybe you can update this? I assume my latest suggestion is easy enough, and would have saved me more time trying to remember what this PR is doing that I think it's worth it 😅

bazel-contrib / rules_nodejs