fix: de-couple CcInfo/cc toolchain from nodejs toolchain
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index 953f3f6..59230f3 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml
@@ -14,67 +14,68 @@ name: ubuntu1804 platform: ubuntu1804 build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-ubuntu" + - "--test_tag_filters=-skip-on-bazelci-ubuntu" test_targets: - - "//..." + - "//..." ubuntu1804-smoke: name: ubuntu1804-smoke platform: ubuntu1804 working_directory: "e2e/smoke" build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-ubuntu" + - "--test_tag_filters=-skip-on-bazelci-ubuntu" test_targets: - - "//..." + - "//..." ubuntu1804-nodejs_host: name: ubuntu1804-nodejs_host platform: ubuntu1804 working_directory: "e2e/nodejs_host" build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-ubuntu" + - "--test_tag_filters=-skip-on-bazelci-ubuntu" test_targets: - - "//..." + - "//..." macos-smoke: name: macos platform: macos working_directory: "e2e/smoke" build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-macos" + - "--test_tag_filters=-skip-on-bazelci-macos" test_targets: - - "//..." + - "//..." windows-smoke: name: windows-smoke platform: windows working_directory: "e2e/smoke" build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-windows" + - "--test_tag_filters=-skip-on-bazelci-windows" test_targets: - - "//..." + - "//..." windows-nodejs_host: name: windows-nodejs_host platform: windows working_directory: "e2e/nodejs_host" build_targets: - - "//..." + - "//..." test_flags: - - "--test_tag_filters=-skip-on-bazelci-windows" + - "--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_flags: + - "--build_tag_filters=-skip-on-rbe" + build_targets: + - "//..." + test_targets: + - "//..."
diff --git a/docs/Core.md b/docs/Core.md index 41bceab..3afcc24 100644 --- a/docs/Core.md +++ b/docs/Core.md
@@ -249,7 +249,7 @@ <h4 id="nodejs_toolchain-headers">headers</h4> -cc_library that contains the Node/v8 header files +filegroup that contains the Node/v8 header files Defaults to `None`
diff --git a/e2e/smoke/BUILD.bazel b/e2e/smoke/BUILD.bazel index 27a0534..619af58 100644 --- a/e2e/smoke/BUILD.bazel +++ b/e2e/smoke/BUILD.bazel
@@ -276,10 +276,11 @@ "@platforms//os:windows": ["/std:c++14"], "//conditions:default": ["-std=c++14"], }), + tags = ["skip-on-rbe"], 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"], + deps = ["@rules_nodejs//nodejs/headers:current_node_cc_headers"], )
diff --git a/e2e/smoke/WORKSPACE.bazel b/e2e/smoke/WORKSPACE.bazel index b7b821b..3a71e7d 100644 --- a/e2e/smoke/WORKSPACE.bazel +++ b/e2e/smoke/WORKSPACE.bazel
@@ -44,23 +44,3 @@ sha256 = "d8f9d40c4656537a60bf0c6daae6f0553f54df5ff2518f86464b7c785f20376b", urls = ["https://registry.npmjs.org/acorn/-/acorn-8.5.0.tgz"], ) - -# -# 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 index e69de29..cb8c8b7 100644 --- 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 index cc5b1c0..e66eda2 100644 --- a/nodejs/BUILD.bazel +++ b/nodejs/BUILD.bazel
@@ -1,8 +1,9 @@ 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") +package(default_visibility = ["//visibility:public"]) + exports_files([ "index.for_docs.bzl", "providers.bzl", @@ -11,7 +12,6 @@ bzl_library( name = "index.for_docs", srcs = glob(["*.bzl"]) + ["@bazel_tools//tools:bzl_srcs"], - visibility = ["//visibility:public"], deps = [ "//nodejs/private:bzl", "//nodejs/private/providers:bzl", @@ -23,10 +23,7 @@ # attribute in order to get a node interpreter for the correct # platform. # See https://docs.bazel.build/versions/main/toolchains.html#writing-rules-that-use-toolchains -toolchain_type( - name = "toolchain_type", - visibility = ["//visibility:public"], -) +toolchain_type(name = "toolchain_type") [ platform( @@ -42,13 +39,4 @@ user_args( name = "default_args", build_setting_default = "--preserve-symlinks", - 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", - visibility = ["//visibility:public"], )
diff --git a/nodejs/headers/BUILD.bazel b/nodejs/headers/BUILD.bazel new file mode 100644 index 0000000..1343e3f --- /dev/null +++ b/nodejs/headers/BUILD.bazel
@@ -0,0 +1,34 @@ +load("//nodejs/private:current_node_cc_headers.bzl", "current_node_cc_headers") + +package(default_visibility = ["//visibility:public"]) + +# Provides the currently active Node toolchain's C++ headers. +# +# Note, "node.h" is only usable from C++, and you'll need to +# ensure you are compiling with c++14 or later. +# +# Also, on Windows, NodeJS releases do not ship headers, so this rule is currently +# not usable with the built-in toolchains. If you define your own toolchain on Windows, +# you can include the headers and then this rule will work. +# +# To use, simply depend on this target where you would have wanted the +# toolchain's underlying `:headers` target: +# +# ```starlark +# cc_library( +# name = "foo", +# srcs = ["foo.cc"], +# # If toolchain sets this already, you can omit. +# copts = ["-std=c++14"], +# deps = ["@rules_nodejs//:current_node_cc_headers"] +# ) +# ``` +cc_library( + name = "current_node_cc_headers", + hdrs = [":current_node_cc_headers_filegroup"], + includes = ["../bin/nodejs/include/node"], +) + +current_node_cc_headers( + name = "current_node_cc_headers_filegroup", +)
diff --git a/nodejs/private/current_node_cc_headers.bzl b/nodejs/private/current_node_cc_headers.bzl index 01bf064..e836787 100644 --- a/nodejs/private/current_node_cc_headers.bzl +++ b/nodejs/private/current_node_cc_headers.bzl
@@ -15,36 +15,11 @@ """Implementation of current_node_cc_headers rule.""" def _current_node_cc_headers_impl(ctx): - return ctx.toolchains["//nodejs:toolchain_type"].nodeinfo.headers.providers_map.values() + return DefaultInfo( + files = ctx.toolchains["//nodejs:toolchain_type"].nodeinfo.headers.files, + ) current_node_cc_headers = rule( implementation = _current_node_cc_headers_impl, toolchains = ["//nodejs:toolchain_type"], - provides = [CcInfo], - doc = """\ -Provides the currently active Node toolchain's C++ headers. - -This is a wrapper around the underlying `cc_library()` for the -C headers for the consuming target's currently active Node toolchain. - -Note, "node.h" is only usable from C++, and you'll need to -ensure you are compiling with c++14 or later. - -Also, on Windows, Node.js releases do not ship headers, so this rule is currently -not usable with the built-in toolchains. If you define your own toolchain on Windows, -you can include the headers and then this rule will work. - -To use, simply depend on this target where you would have wanted the -toolchain's underlying `:headers` target: - -```starlark -cc_library( - name = "foo", - srcs = ["foo.cc"], - # If toolchain sets this already, you can omit. - copts = ["-std=c++14"], - deps = ["@rules_nodejs//:current_node_cc_headers"] -) -``` -""", )
diff --git a/nodejs/repositories.bzl b/nodejs/repositories.bzl index b722618..e6ae7f5 100644 --- a/nodejs/repositories.bzl +++ b/nodejs/repositories.bzl
@@ -241,9 +241,9 @@ name = "npm_files", srcs = glob(["bin/nodejs/**"]) + [":node_files"], ) -cc_library( +filegroup( name = "headers", - hdrs = glob( + srcs = glob( ["bin/nodejs/include/node/**"], # Apparently, node.js doesn't ship the headers in their Windows package. # https://stackoverflow.com/questions/50745670/nodejs-headers-on-windows-are-not-installed-automatically @@ -254,7 +254,6 @@ # -> no results ... allow_empty = True, ), - includes = ["bin/nodejs/include/node"], ) """.format( node_bin_export = "\n \"%s\"," % node_bin,
diff --git a/nodejs/toolchain.bzl b/nodejs/toolchain.bzl index 62dd4c2..103d3bf 100644 --- a/nodejs/toolchain.bzl +++ b/nodejs/toolchain.bzl
@@ -33,27 +33,7 @@ For backward compability, npm_path is set to the runfiles path of npm if npm is set. """, "npm_sources": """Additional source files required to run npm""", - "headers": """\ -(struct) Information about the header files, with fields: - * providers_map: a dict of string to provider instances. The key should be - a fully qualified name (e.g. `@rules_foo//bar:baz.bzl#MyInfo`) of the - provider to uniquely identify its type. - - The following keys are always present: - * CcInfo: the CcInfo provider instance for the headers. - * DefaultInfo: the DefaultInfo provider instance for the headers. - - A map is used to allow additional providers from the originating headers - target (typically a `cc_library`) to be propagated to consumers (directly - exposing a Target object can cause memory issues and is an anti-pattern). - - When consuming this map, it's suggested to use `providers_map.values()` to - return all providers; or copy the map and filter out or replace keys as - appropriate. Note that any keys begining with `_` (underscore) are - considered private and should be forward along as-is (this better allows - e.g. `:current_node_cc_headers` to act as the underlying headers target it - represents). -""", + "headers": "A filegroup containing the Node headers.", # DEPRECATED "target_tool_path": "(DEPRECATED) Path to Node.js executable for backward compatibility", "tool_files": """(DEPRECATED) Alias for [node] for backward compatibility""", @@ -93,12 +73,7 @@ npm = ctx.file.npm, npm_path = ctx.attr.npm_path if ctx.attr.npm_path else (_to_manifest_path(ctx, ctx.file.npm) if ctx.file.npm else ""), # _to_manifest_path for backward compat npm_sources = npm_sources, - headers = struct( - providers_map = { - "CcInfo": ctx.attr.headers[CcInfo], - "DefaultInfo": ctx.attr.headers[DefaultInfo], - }, - ) if ctx.attr.headers else None, + headers = ctx.attr.headers, # For backward compat target_tool_path = _to_manifest_path(ctx, ctx.file.node) if ctx.attr.node else ctx.attr.node_path, tool_files = [ctx.file.node] if ctx.attr.node else [], @@ -205,7 +180,7 @@ Not necessary if specifying `npm_path` to a non-hermetic npm installation. - headers: cc_library that contains the Node/v8 header files + headers: filegroup that contains the Node/v8 header files **kwargs: Additional parameters """