[FR]: Separate toolchain for for C++ headers

[Silic0nS0ldier]

What is the current behavior?

As of #3694 the NodeInfo provider includes CcInfo for the NodeJS N-API headers.

While useful for native addon development, this made the node_toolchain depend on the C++ toolchain as that CcInfo instance is pulled from the cc_library target :headers.

For workflows that don't need those headers, they incur the penalty of fetching the C++ toolchain. This can play out as;

• A sub-second delay while the repo fetches.
• A much more substantial delay (e.g. if using nixpkgs_cc_configure from https://github.com/tweag/rules_nixpkgs)
• An error due to the C++ toolchain not being defined.

Describe the feature

Introduce a separate toolchain targeted at native addon development so that plain JS workflows do not implicitly depend on C++ toolchains.

[jesses-canva]

A specific use case that is broken by this is embedding a js_binary in a Linux container image with rules_docker or rules_oci, but building on macOS or Windows, or Linux of a different arch, as in these cases a cc toolchain targeting the container image platform isn't available and a Error in fail: Unable to find a CC toolchain using toolchain resolution. results, even though a cc toolchain isn't actually needed.

[dzbarsky]

Hmm can this be optional/lazy somehow?

Having it be a separate toolchain might end up with it not matching the selected target toolchain. I'm surprised we haven't heard similar issues with py_binary, I think it's the same setup?

[jesses-canva]

I'm surprised we haven't heard similar issues with py_binary, I think it's the same setup?

py_runtime doesn't contain a label to a cc_library like node_toolchain does, so doesn't have this specific issue.

Although there is a similar issue in https://github.com/bazelbuild/bazel/issues/8751 that causes a py_binary to require a C++ toolchain, we have patched that out for the same reason.

[jesses-canva]

One option would be

1. Use a filegroup instead of a cc_library
2. Include just a DeafaultInfo of the header files in the NodeInfo, instead of a DefaultInfo and a CcInfo, this way analysing the toolchain doesn't require analysing a cc_library.
3. In current_node_cc_headers return the DefaultInfo out of the current NodeInfo.headers instead of a CcInfo.
4. Wrap current_node_cc_headers with a cc_library that contains it in hdrs, to be used like current_node_cc_headers currently is.

[dzbarsky]

@jesses-canva That's an interesting idea! I gave it a shot, but there's one wrinkle - we need to figure out the right includes path so that the consuming code can #include "node.h" directly. I think that's a little tricky though, because the include path depends on the (potentially remapped) repo name. Any ideas how to do this correctly? Here's a sketch PR https://github.com/bazelbuild/rules_nodejs/compare/main...dzbarsky:rules_nodejs:main#diff-386140e96f3923bdf4bbbf4506c102245f6197b92fbfd701c507ced4e3b86ac0R72

[github-actions[bot]]

This issue has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs in 30 days. Note as of rules_nodejs v6 the rules_nodejs repository contains only the core nodejs toolchain and @bazel/runfiles package. All rulesets are removed and unmaintained. For alternate rulesets suggestions include https://github.com/aspect-build/rules_js, https://github.com/aspect-build/rules_ts Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[gregmagolan]

Not stale

[gregmagolan]

I'm going to be looking at this this week to make this fix before rules_js 2.0 final so if it needs to be breaking for some use cases we can update the minimum dep in the rules_js 2.0 final release

[gregmagolan]

Fix landed and released with https://github.com/bazelbuild/rules_nodejs/releases/tag/v6.2.0