Skip to content

Add current_node_cc_headers#3694

Merged
alexeagle merged 1 commit into
bazel-contrib:mainfrom
dzbarsky:headers_binary
Oct 4, 2023
Merged

Add current_node_cc_headers#3694
alexeagle merged 1 commit into
bazel-contrib:mainfrom
dzbarsky:headers_binary

Conversation

@dzbarsky

@dzbarsky dzbarsky commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

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)

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?

Currently the headers are exposed but they need to be accessed from toolchain-specific repos like @node_darwin_amd64//:headers. This is a bit annoying, and I believe these repos are not visible in the main workspace in bzlmod. Copy the rules_python approach to make this more ergonomic.

Issue Number: N/A

What is the new behavior?

You can depend on @rules_nodejs//nodejs:current_node_cc_headers.

Does this PR introduce a breaking change?

  • Yes
  • No

(Unless someone was creating their own toolchains, in which case they will need to pass the extra label)

Other information

@dzbarsky dzbarsky force-pushed the headers_binary branch 6 times, most recently from 5914227 to ade4b09 Compare October 4, 2023 16:12
@alexeagle alexeagle merged commit f4f15e9 into bazel-contrib:main Oct 4, 2023
@dzbarsky dzbarsky deleted the headers_binary branch October 4, 2023 20:29
@avdv

avdv commented Nov 15, 2023

Copy link
Copy Markdown
Contributor

Hi.

This seems to be a breaking change, since now calls to node_toolchain without specifying the headers attribute fails with:

ERROR: /home/runner/.cache/bazel/_bazel_runner/c91220d87f26118b19246641d5266d21/external/rules_haskell_nix_node_toolchain/BUILD:4:15: in node_toolchain rule @rules_haskell_nix_node_toolchain//:node_nixpkgs: 
Traceback (most recent call last):
	File "/home/runner/.cache/bazel/_bazel_runner/c91220d87f26118b19246641d5266d21/external/rules_nodejs/nodejs/toolchain.bzl", line 102, column 43, in _node_toolchain_impl
		"CcInfo": ctx.attr.headers[CcInfo],
Error: type 'NoneType' has no operator [](Provider)

@DavidZbarsky-at

DavidZbarsky-at commented Nov 15, 2023

Copy link
Copy Markdown

@avdv Hi, yes I mentioned in the summary that manually-created toolchains would need a minor adjustment, which is not a very common use case in my understanding. Is that feasible for you?

@avdv

avdv commented Nov 15, 2023

Copy link
Copy Markdown
Contributor

@avdv Hi, yes I mentioned in the summary that manually-created toolchains would need a minor adjustment, which is not a very common use case in my understanding. Is that feasible for you?

Hey @DavidZbarsky-at, well I am looking into that...

The thing is that we want to provide compatibility for rules_nodejs < 6.0.2 and >= 6.0.2. So I would have to pass headers in one case, but not in the other.

@DavidZbarsky-at

DavidZbarsky-at commented Nov 15, 2023

Copy link
Copy Markdown

Got it, sorry about the pain. Perhaps we can make the headers optional for now and default to an empty filegroup? And then mandatory after some transition period? @alexeagle wdyt?

@avdv

avdv commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

Applying this patch works for me:

diff --git a/nodejs/toolchain.bzl b/nodejs/toolchain.bzl
index 7980ed26..546105a0 100644
--- a/nodejs/toolchain.bzl
+++ b/nodejs/toolchain.bzl
@@ -99,13 +99,13 @@ def _node_toolchain_impl(ctx):
         run_npm = ctx.file.run_npm,
         headers = struct(
             providers_map = {
                 "CcInfo": ctx.attr.headers[CcInfo],
                 "DefaultInfo": ctx.attr.headers[DefaultInfo],
             },
-        ),
+        ) if ctx.attr.headers else None,
     )
 
     # Export all the providers inside our ToolchainInfo
     # so the resolved_toolchain rule can grab and re-export them.
     toolchain_info = platform_common.ToolchainInfo(
         nodeinfo = nodeinfo,

@alexeagle

alexeagle commented Nov 16, 2023

Copy link
Copy Markdown
Collaborator

Making headers optional until 7.0 seems like the right fix to me. How difficult is that?

@DavidZbarsky-at

DavidZbarsky-at commented Nov 16, 2023 via email

Copy link
Copy Markdown

@avdv avdv mentioned this pull request Nov 16, 2023
Merged
12 tasks
@avdv

avdv commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

That patch seems reasonable to me. I can send a PR tonight if nobody else gets to it first

I opened a PR: #3704

@Silic0nS0ldier Silic0nS0ldier mentioned this pull request Mar 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexeagle alexeagle alexeagle approved these changes

@mattem mattem Awaiting requested review from mattem mattem is a code owner

@gregmagolan gregmagolan Awaiting requested review from gregmagolan gregmagolan is a code owner

@jbedard jbedard Awaiting requested review from jbedard jbedard is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dzbarsky @avdv @DavidZbarsky-at @alexeagle