Add current_node_cc_headers

[dzbarsky]

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

[avdv]

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]

@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 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]

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]

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]

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

[DavidZbarsky-at]

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

…

On Thu, Nov 16, 2023, 9:59 AM Alex Eagle ***@***.***> wrote: Making headers optional until 7.0 seems like the right fix to me. How difficult is that? — Reply to this email directly, view it on GitHub <#3694 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWQHWZMJVQ4QQZOF475XEWDYEYS5JAVCNFSM6AAAAAA5SXDTTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJUGYZDGMZWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[avdv]

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

I opened a PR: #3704