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

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

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jun 24, 2024

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
  • 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
  • No

Other information

redsun82 added a commit to github/codeql that referenced this pull request Jun 24, 2024
This patches `rules_nodejs` with the contents of
bazel-contrib/rules_nodejs#3763
in order to allow specifying a mirror for nodejs, as nodejs.org has
hit us with intermittent downtimes.
nodejs/extensions.bzl Outdated Show resolved Hide resolved
nodejs/extensions.bzl Outdated Show resolved Hide resolved
nodejs/extensions.bzl Outdated Show resolved Hide resolved
The conflict check was not comparing the `include_headers` attribute.
Now the check will always compare all attributes of the declared
toolchains, even if adding new ones in the future.
@redsun82
Copy link
Contributor Author

@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
Copy link
Collaborator

jbedard commented Jun 25, 2024

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

@redsun82
Copy link
Contributor Author

redsun82 commented Jun 25, 2024

@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 🙂 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.

@redsun82 redsun82 requested a review from jbedard August 8, 2024 08:47
@criemen
Copy link

criemen commented Sep 5, 2024

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

nodejs/extensions.bzl Outdated Show resolved Hide resolved
@jbedard
Copy link
Collaborator

jbedard commented Sep 7, 2024

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 😅

@jbedard jbedard changed the title Add node_urls parameter to bzlmod toolchain in the node extension feat: add node_urls parameter to bzlmod toolchain in the node extension Sep 17, 2024
@jbedard jbedard enabled auto-merge (rebase) September 17, 2024 21:27
auto-merge was automatically disabled September 18, 2024 09:05

Head branch was pushed to by a user without write access

@jbedard jbedard merged commit 4c48449 into bazel-contrib:main Sep 18, 2024
16 checks passed
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