fix: make CcInfo/cc dep in nodejs toolchain opt-in via include_headers attribute (#3760)

gregmagolan · web-flow · commit 111e65bae042 · 2024-06-08T11:53:39.000-07:00
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml
@@ -16,6 +16,16 @@ tasks:
       - "--test_tag_filters=-skip-on-bazelci-ubuntu"
     test_targets:
       - "//..."
+  ubuntu1804-headers:
+    name: ubuntu1804-headers
+    platform: ubuntu1804
+    working_directory: "e2e/headers"
+    build_targets:
+      - "//..."
+    test_flags:
+      - "--test_tag_filters=-skip-on-bazelci-ubuntu"
+    test_targets:
+      - "//..."
   ubuntu1804-smoke:
     name: ubuntu1804-smoke
     platform: ubuntu1804
@@ -36,9 +46,28 @@ tasks:
       - "--test_tag_filters=-skip-on-bazelci-ubuntu"
     test_targets:
       - "//..."
-  macos-smoke:
+  macos:
     name: macos
     platform: macos
+    build_targets:
+      - "//..."
+    test_flags:
+      - "--test_tag_filters=-skip-on-bazelci-macos"
+    test_targets:
+      - "//..."
+  macos-headers:
+    name: macos-headers
+    platform: macos
+    working_directory: "e2e/headers"
+    build_targets:
+      - "//..."
+    test_flags:
+      - "--test_tag_filters=-skip-on-bazelci-macos"
+    test_targets:
+      - "//..."
+  macos-smoke:
+    name: macos-smoke
+    platform: macos
     working_directory: "e2e/smoke"
     build_targets:
       - "//..."
@@ -66,12 +95,11 @@ tasks:
       - "--test_tag_filters=-skip-on-bazelci-windows"
     test_targets:
       - "//..."
-  # Temporarily disabled RBE CI until cc toolchain failures are resolved
-  # rbe_ubuntu1604-smoke:
-  #   name: rbe_ubuntu1604-smoke
-  #   platform: rbe_ubuntu1604
-  #   working_directory: "e2e/smoke"
-  #   build_targets:
-  #   - "//..."
-  #   test_targets:
-  #   - "//..."
+  rbe_ubuntu1604-smoke:
+    name: rbe_ubuntu1604-smoke
+    platform: rbe_ubuntu1604
+    working_directory: "e2e/smoke"
+    build_targets:
+      - "//..."
+    test_targets:
+      - "//..."
diff --git a/.bazelrc b/.bazelrc
@@ -28,6 +28,11 @@ common --nolegacy_external_runfiles
 # https://bazelbuild.slack.com/archives/C014RARENH0/p1691158021917459?thread_ts=1691156601.420349&cid=C014RARENH0
 common --check_direct_dependencies=off
 
+# In the root MODULE.bazel file we don't set include_headers on the nodejs toolchain
+# so the `//nodejs/headers:current_node_cc_headers`` target will not build. This target
+# is instead tested in `e2e/headers``
+common --deleted_packages=nodejs/headers
+
 # Load any settings specific to the current user.
 # .bazelrc.user should appear in .gitignore so that settings are not shared with team members
 # This needs to be last statement in this
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -19,7 +19,7 @@ jobs:
   test:
     uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6
     with:
-      folders: '[".", "e2e/smoke", "e2e/nodejs_host"]'
+      folders: '[".", "e2e/headers", "e2e/smoke", "e2e/nodejs_host"]'
       # stardoc generated docs fail on diff_test with Bazel 6.4.0 so don't test against it in root repository
       exclude: |
         [
diff --git a/docs/Core.md b/docs/Core.md
@@ -34,7 +34,7 @@ UserBuildSettingInfo(<a href="#UserBuildSettingInfo-value">value</a>)
 
 <pre>
 nodejs_repositories(<a href="#nodejs_repositories-name">name</a>, <a href="#nodejs_repositories-node_download_auth">node_download_auth</a>, <a href="#nodejs_repositories-node_repositories">node_repositories</a>, <a href="#nodejs_repositories-node_urls">node_urls</a>, <a href="#nodejs_repositories-node_version">node_version</a>,
-                    <a href="#nodejs_repositories-node_version_from_nvmrc">node_version_from_nvmrc</a>, <a href="#nodejs_repositories-kwargs">kwargs</a>)
+                    <a href="#nodejs_repositories-node_version_from_nvmrc">node_version_from_nvmrc</a>, <a href="#nodejs_repositories-include_headers">include_headers</a>, <a href="#nodejs_repositories-kwargs">kwargs</a>)
 </pre>
 
 To be run in user's WORKSPACE to install rules_nodejs dependencies.
@@ -143,6 +143,14 @@ If set then the version found in the .nvmrc file is used instead of the one spec
 
 Defaults to `None`
 
+<h4 id="nodejs_repositories-include_headers">include_headers</h4>
+
+Set headers field in NodeInfo provided by this toolchain.
+
+This setting creates a dependency on a c++ toolchain.
+
+Defaults to `False`
+
 <h4 id="nodejs_repositories-kwargs">kwargs</h4>
 
 Additional parameters
diff --git a/e2e/headers/.bazelrc b/e2e/headers/.bazelrc
@@ -0,0 +1,20 @@
+# Specifies desired output mode for running tests.
+# Valid values are
+#   'summary' to output only test status summary
+#   'errors' to also print test logs for failed tests
+#   'all' to print logs for all tests
+#   'streamed' to output logs for all tests in real time
+#     (this will force tests to be executed locally one at a time regardless of --test_strategy value).
+common --test_output=errors
+
+# Turn on --incompatible_strict_action_env which was on by default
+# in Bazel 0.21.0 but turned off again in 0.22.0. Follow
+# https://github.com/bazelbuild/bazel/issues/7026 for more details.
+# This flag is needed to so that the bazel cache is not invalidated
+# when running bazel via `yarn bazel`.
+# See https://github.com/angular/angular/issues/27514.
+common --incompatible_strict_action_env
+
+# Turn off legacy external runfiles
+# This prevents accidentally depending on this feature, which Bazel will remove.
+common --nolegacy_external_runfiles
diff --git a/e2e/headers/.bazelversion b/e2e/headers/.bazelversion
@@ -0,0 +1 @@
+../../.bazelversion
diff --git a/e2e/headers/BUILD.bazel b/e2e/headers/BUILD.bazel
@@ -0,0 +1,14 @@
+cc_test(
+    name = "using_headers_test",
+    srcs = ["using_headers.cc"],
+    copts = select({
+        "@platforms//os:windows": ["/std:c++14"],
+        "//conditions:default": ["-std=c++14"],
+    }),
+    target_compatible_with = select({
+        # Windows does not ship headers in the release artifact so this won't work yet.
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
+    deps = ["@rules_nodejs//nodejs/headers:current_node_cc_headers"],
+)
diff --git a/e2e/headers/MODULE.bazel b/e2e/headers/MODULE.bazel
@@ -0,0 +1,10 @@
+bazel_dep(name = "rules_nodejs", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "rules_nodejs",
+    path = "../..",
+)
+
+bazel_dep(name = "platforms", version = "0.0.10", dev_dependency = True)
+
+node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
+node.toolchain(include_headers = True)
diff --git a/e2e/headers/WORKSPACE.bazel b/e2e/headers/WORKSPACE.bazel
@@ -0,0 +1,16 @@
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+
+local_repository(
+    name = "rules_nodejs",
+    path = "../..",
+)
+
+load("@rules_nodejs//nodejs:repositories.bzl", "nodejs_register_toolchains")
+
+nodejs_register_toolchains(include_headers = True)
+
+http_archive(
+    name = "bazel_skylib",
+    sha256 = "bc283cdfcd526a52c3201279cda4bc298652efa898b10b4db0837dc51652756f",
+    urls = ["https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz"],
+)
diff --git a/e2e/headers/WORKSPACE.bzlmod b/e2e/headers/WORKSPACE.bzlmod
diff --git a/e2e/headers/using_headers.cc b/e2e/headers/using_headers.cc
diff --git a/e2e/smoke/BUILD.bazel b/e2e/smoke/BUILD.bazel
@@ -268,18 +268,3 @@ diff_test(
     file1 = "write_node_version_16",
     file2 = "thing_toolchain_16",
 )
-
-cc_binary(
-    name = "using_headers_test",
-    srcs = ["using_headers.cc"],
-    copts = select({
-        "@platforms//os:windows": ["/std:c++14"],
-        "//conditions:default": ["-std=c++14"],
-    }),
-    target_compatible_with = select({
-        # Windows does not ship headers in the release artifact so this won't work yet.
-        "@platforms//os:windows": ["@platforms//:incompatible"],
-        "//conditions:default": [],
-    }),
-    deps = ["@rules_nodejs//nodejs:current_node_cc_headers"],
-)
diff --git a/e2e/smoke/WORKSPACE.bazel b/e2e/smoke/WORKSPACE.bazel
@@ -44,23 +44,3 @@ load("@aspect_bazel_lib//lib:repositories.bzl", "aspect_bazel_lib_dependencies",
 aspect_bazel_lib_dependencies()
 
 aspect_bazel_lib_register_toolchains()
-
-#
-# RBE configuration
-#
-# See https://github.com/bazelbuild/continuous-integration/releases/tag/rules-1.0.0
-http_archive(
-    name = "bazelci_rules",
-    sha256 = "eca21884e6f66a88c358e580fd67a6b148d30ab57b1680f62a96c00f9bc6a07e",
-    strip_prefix = "bazelci_rules-1.0.0",
-    url = "https://github.com/bazelbuild/continuous-integration/releases/download/rules-1.0.0/bazelci_rules-1.0.0.tar.gz",
-)
-
-load("@bazelci_rules//:rbe_repo.bzl", "rbe_preconfig")
-
-# Creates toolchain configuration for remote execution with BuildKite CI
-# for rbe_ubuntu1604
-rbe_preconfig(
-    name = "buildkite_config",
-    toolchain = "ubuntu1804-bazel-java11",
-)
diff --git a/e2e/smoke/WORKSPACE.bzlmod b/e2e/smoke/WORKSPACE.bzlmod
@@ -0,0 +1,21 @@
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+
+#
+# RBE configuration
+#
+# See https://github.com/bazelbuild/continuous-integration/releases/tag/rules-1.0.0
+http_archive(
+    name = "bazelci_rules",
+    sha256 = "eca21884e6f66a88c358e580fd67a6b148d30ab57b1680f62a96c00f9bc6a07e",
+    strip_prefix = "bazelci_rules-1.0.0",
+    url = "https://github.com/bazelbuild/continuous-integration/releases/download/rules-1.0.0/bazelci_rules-1.0.0.tar.gz",
+)
+
+load("@bazelci_rules//:rbe_repo.bzl", "rbe_preconfig")
+
+# Creates toolchain configuration for remote execution with BuildKite CI
+# for rbe_ubuntu1604
+rbe_preconfig(
+    name = "buildkite_config",
+    toolchain = "ubuntu1804-bazel-java11",
+)
diff --git a/nodejs/BUILD.bazel b/nodejs/BUILD.bazel
@@ -1,5 +1,4 @@
 load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
-load("//nodejs/private:current_node_cc_headers.bzl", "current_node_cc_headers")
 load("//nodejs/private:nodejs_toolchains_repo.bzl", "PLATFORMS")
 load("//nodejs/private:user_build_settings.bzl", "user_args")
 
@@ -41,8 +40,3 @@ user_args(
     name = "default_args",
     build_setting_default = "--preserve-symlinks",
 )
-
-# This target provides the C headers for whatever the current toolchain is
-# for the consuming rule. It basically acts like a cc_library by forwarding
-# on the providers for the underlying cc_library that the toolchain is using.
-current_node_cc_headers(name = "current_node_cc_headers")
diff --git a/nodejs/extensions.bzl b/nodejs/extensions.bzl
@@ -26,13 +26,15 @@ def _toolchain_extension(module_ctx):
                 registrations[toolchain.name] = struct(
                     node_version = toolchain.node_version,
                     node_version_from_nvmrc = toolchain.node_version_from_nvmrc,
+                    include_headers = toolchain.include_headers,
                 )
 
     for k, v in registrations.items():
         nodejs_register_toolchains(
             name = k,
             node_version = v.node_version,
             node_version_from_nvmrc = v.node_version_from_nvmrc,
+            include_headers = v.include_headers,
             register = False,
         )
 
@@ -54,6 +56,12 @@ node = module_extension(
 
 If set then the version found in the .nvmrc file is used instead of the one specified by node_version.""",
             ),
+            "include_headers": attr.bool(
+                doc = """Set headers field in NodeInfo provided by this toolchain.
+
+This setting creates a dependency on a c++ toolchain.
+""",
+            ),
         }),
     },
 )
diff --git a/nodejs/headers/BUILD.bazel b/nodejs/headers/BUILD.bazel
@@ -0,0 +1,8 @@
+load("//nodejs/private:current_node_cc_headers.bzl", "current_node_cc_headers")
+
+package(default_visibility = ["//visibility:public"])
+
+# This target provides the C headers for whatever the current toolchain is
+# for the consuming rule. It basically acts like a cc_library by forwarding
+# on the providers for the underlying cc_library that the toolchain is using.
+current_node_cc_headers(name = "current_node_cc_headers")
diff --git a/nodejs/private/current_node_cc_headers.bzl b/nodejs/private/current_node_cc_headers.bzl
@@ -43,7 +43,7 @@ cc_library(
     srcs = ["foo.cc"],
     # If toolchain sets this already, you can omit.
     copts = ["-std=c++14"],
-    deps = ["@rules_nodejs//:current_node_cc_headers"]
+    deps = ["@rules_nodejs//nodejs/headers:current_node_cc_headers"]
 )
 ```
 """,
diff --git a/nodejs/repositories.bzl b/nodejs/repositories.bzl
@@ -31,6 +31,7 @@ _ATTRS = {
     "node_urls": attr.string_list(),
     "node_version": attr.string(),
     "node_version_from_nvmrc": attr.label(allow_single_file = True),
+    "include_headers": attr.bool(),
     "platform": attr.string(
         doc = "Internal use only. Which platform to install as a toolchain. If unset, we assume the repository is named nodejs_[platform]",
         values = BUILT_IN_NODE_PLATFORMS,
@@ -241,6 +242,20 @@ filegroup(
   name = "npm_files",
   srcs = glob(["bin/nodejs/**"]) + [":node_files"],
 )
+""".format(
+        node_bin_export = "\n  \"%s\"," % node_bin,
+        npm_bin_export = "\n  \"%s\"," % npm_bin,
+        npx_bin_export = "\n  \"%s\"," % npx_bin,
+        node_bin_label = node_bin_label,
+        npm_bin_label = npm_bin_label,
+        npx_bin_label = npx_bin_label,
+        node_entry = node_entry,
+        npm_entry = npm_entry,
+        npx_entry = npx_entry,
+    )
+
+    if repository_ctx.attr.include_headers:
+        build_content += """
 cc_library(
   name = "headers",
   hdrs = glob(
@@ -256,17 +271,7 @@ cc_library(
   ),
   includes = ["bin/nodejs/include/node"],
 )
-""".format(
-        node_bin_export = "\n  \"%s\"," % node_bin,
-        npm_bin_export = "\n  \"%s\"," % npm_bin,
-        npx_bin_export = "\n  \"%s\"," % npx_bin,
-        node_bin_label = node_bin_label,
-        npm_bin_label = npm_bin_label,
-        npx_bin_label = npx_bin_label,
-        node_entry = node_entry,
-        npm_entry = npm_entry,
-        npx_entry = npx_entry,
-    )
+"""
 
     if repository_ctx.attr.platform:
         build_content += """
@@ -276,14 +281,15 @@ nodejs_toolchain(
     node = ":node_bin",
     npm = ":npm",
     npm_srcs = [":npm_files"],
-    headers = ":headers",
+    headers = {headers},
 )
 # alias for backward compat
 alias(
     name = "node_toolchain",
     actual = ":toolchain",
 )
-"""
+""".format(headers = "\":headers\"" if repository_ctx.attr.include_headers else "None")
+
     repository_ctx.file("BUILD.bazel", content = build_content)
 
 def _strip_bin(path):
@@ -314,6 +320,7 @@ def nodejs_repositories(
         node_urls = [DEFAULT_NODE_URL],
         node_version = DEFAULT_NODE_VERSION,
         node_version_from_nvmrc = None,
+        include_headers = False,
         **kwargs):
     """To be run in user's WORKSPACE to install rules_nodejs dependencies.
 
@@ -394,6 +401,10 @@ def nodejs_repositories(
 
             If set then the version found in the .nvmrc file is used instead of the one specified by node_version.
 
+        include_headers: Set headers field in NodeInfo provided by this toolchain.
+
+            This setting creates a dependency on a c++ toolchain.
+
         **kwargs: Additional parameters
     """
     use_nvmrc = kwargs.pop("use_nvmrc", None)
@@ -411,6 +422,7 @@ WARNING: use_nvmrc attribute of node_repositories is deprecated; use node_versio
         node_urls = node_urls,
         node_version = node_version,
         node_version_from_nvmrc = node_version_from_nvmrc,
+        include_headers = include_headers,
         **kwargs
     )
 
diff --git a/nodejs/toolchain.bzl b/nodejs/toolchain.bzl

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ jobs:`
`19`	`19`	`test:`
`20`	`20`	`uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6`
`21`	`21`	`with:`
`22`		`- folders: '[".", "e2e/smoke", "e2e/nodejs_host"]'`
	`22`	`+ folders: '[".", "e2e/headers", "e2e/smoke", "e2e/nodejs_host"]'`
`23`	`23`	`# stardoc generated docs fail on diff_test with Bazel 6.4.0 so don't test against it in root repository`
`24`	`24`	`exclude: \|`
`25`	`25`	`[`