fix(builtin): only stamp artifacts when --stamp is passed to bazel (#…

…1441)

Fixes remote cache misses of rollup_bundle and npm_package targets observed by Angular team

docs/index.md

@@ -130,12 +130,19 @@ Bazel is generally only a build tool, and is unaware of your version control sys
 However, when publishing releases, you typically want to embed version information in the resulting distribution.
 Bazel supports this natively, using the following approach:
 
-First, pass the `workspace_status_command` argument to `bazel build`. We prefer to do this with an entry in `.bazelrc`:
+To stamp a build, you must pass the `--stamp` argument to Bazel.
+
+> Previous releases of rules_nodejs stamped builds always.
+> However this caused stamp-aware actions to never be remotely cached, since the volatile
+> status file is passed as an input and its checksum always changes.
+
+Also pass the `workspace_status_command` argument to `bazel build`.
+We prefer to do these with an entry in `.bazelrc`:
 
 ```sh    
 # This tells Bazel how to interact with the version control system
 # Enable this with --config=release
-build:release --workspace_status_command=./tools/bazel_stamp_vars.sh
+build:release --stamp --workspace_status_command=./tools/bazel_stamp_vars.sh
 ```
 
 Then create `tools/bazel_stamp_vars.sh`.
@@ -151,8 +158,6 @@ echo BUILD_SCM_VERSION $(git describe --abbrev=7 --tags HEAD)
 
 For a more full-featured script, take a look at the [bazel_stamp_vars in Angular]
 
-Ideally, `rollup_bundle` and `npm_package` should honor the `--stamp` argument to `bazel build`. However this is not currently possible, see https://github.com/bazelbuild/bazel/issues/1054
-
 Finally, we recommend a release script around Bazel. We typically have more than one npm package published from one Bazel workspace, so we do a `bazel query` to find them, and publish in a loop. Here is a template to get you started:
 
 ```sh

internal/BUILD.bazel

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 load("@build_bazel_rules_nodejs//:index.bzl", "BAZEL_VERSION", "nodejs_test")
+load("//internal/node:context.bzl", "node_context_data")
 
 package(default_visibility = ["//visibility:public"])
 
@@ -33,3 +34,19 @@ nodejs_test(
     # Not necessary here, just a regression test for #1043
     templated_args_file = "package_json_test.params",
 )
+
+# Detect if the build is running under --stamp
+config_setting(
+    name = "stamp",
+    values = {"stamp": "true"},
+)
+
+# Modelled after go_context_data from rules_go
+# passes config values to starlark via a provider
+node_context_data(
+    name = "node_context_data",
+    stamp = select({
+        "@build_bazel_rules_nodejs//internal:stamp": True,
+        "//conditions:default": False,
+    }),
+)

internal/node/context.bzl

@@ -0,0 +1,20 @@
+"node_context_data rule"
+
+load("@build_bazel_rules_nodejs//:providers.bzl", "NodeContextInfo")
+
+_DOC = """node_context_data gathers information about the build configuration.
+It is a common dependency of all targets that are sensitive to configuration.
+(currently npm_package and rollup_bundle)"""
+
+def _impl(ctx):
+    return [NodeContextInfo(stamp = ctx.attr.stamp)]
+
+# Modelled after go_context_data in rules_go
+# Works around github.com/bazelbuild/bazel/issues/1054
+node_context_data = rule(
+    implementation = _impl,
+    attrs = {
+        "stamp": attr.bool(mandatory = True),
+    },
+    doc = _DOC,
+)

internal/npm_package/npm_package.bzl

@@ -6,7 +6,7 @@ If all users of your library code use Bazel, they should just add your library
 to the `deps` of one of their targets.
 """
 
-load("//:providers.bzl", "DeclarationInfo", "JSNamedModuleInfo")
+load("//:providers.bzl", "DeclarationInfo", "JSNamedModuleInfo", "NodeContextInfo")
 load("//internal/common:path_utils.bzl", "strip_external")
 
 # Takes a depset of files and returns a corresponding list of file paths without any files
@@ -40,6 +40,7 @@ def create_package(ctx, deps_sources, nested_packages):
       The tree artifact which is the publishable directory.
     """
 
+    stamp = ctx.attr.node_context_data[NodeContextInfo].stamp
     package_dir = ctx.actions.declare_directory(ctx.label.name)
     package_path = ctx.label.package
 
@@ -61,7 +62,7 @@ def create_package(ctx, deps_sources, nested_packages):
     args.add(ctx.attr.replacements)
     args.add_all([ctx.outputs.pack.path, ctx.outputs.publish.path])
     args.add(ctx.attr.replace_with_version)
-    args.add(ctx.version_file.path if ctx.version_file else "")
+    args.add(ctx.version_file.path if stamp else "")
     args.add_joined(ctx.attr.vendor_external, join_with = ",", omit_if_empty = False)
     args.add("1" if ctx.attr.rename_build_files else "0")
 
@@ -75,7 +76,7 @@ def create_package(ctx, deps_sources, nested_packages):
     # produced by the --workspace_status_command. That command will be executed whenever
     # this action runs, so we get the latest version info on each execution.
     # See https://github.com/bazelbuild/bazel/issues/1054
-    if ctx.version_file:
+    if stamp:
         inputs.append(ctx.version_file)
 
     ctx.actions.run(
@@ -129,6 +130,11 @@ NPM_PACKAGE_ATTRS = {
         doc = """Files inside this directory which are simply copied into the package.""",
         allow_files = True,
     ),
+    "node_context_data": attr.label(
+        default = "@build_bazel_rules_nodejs//internal:node_context_data",
+        providers = [NodeContextInfo],
+        doc = "Internal use only",
+    ),
     "packages": attr.label_list(
         doc = """Other npm_package rules whose content is copied into this package.""",
         allow_files = True,

internal/npm_package/test/BUILD.bazel

@@ -1,6 +1,7 @@
 load("@build_bazel_rules_nodejs//:index.bzl", "npm_package")
 load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test")
 load("@npm_bazel_typescript//:index.from_src.bzl", "ts_library")
+load("//internal/node:context.bzl", "node_context_data")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/write_file.bzl", "write_file")
 
 write_file(
@@ -20,13 +21,21 @@ npm_package(
     srcs = ["dependent_file"],
 )
 
+# Force stamping behavior even in builds without --stamp config
+# by mocking out the config data
+node_context_data(
+    name = "force_stamp",
+    stamp = True,
+)
+
 npm_package(
     name = "test_pkg",
     srcs = [
         "package.json",
         "some_file",
         "@internal_npm_package_test_vendored_external//:vendored_external_file",
     ],
+    node_context_data = ":force_stamp",
     packages = [":dependent_pkg"],
     replacements = {"replace_me": "replaced"},
     vendor_external = [

internal/npm_package/test/npm_package.spec.js

@@ -31,7 +31,7 @@ describe('npm_package srcs', () => {
     expect(read('data.json')).toEqual('[]');
   });
   it('replaced 0.0.0-PLACEHOLDER', () => {
-    expect(read('package.json').version).not.toEqual('0.0.0-PLACEHOLDER');
+    expect(JSON.parse(read('package.json')).version).toEqual('1.2.3');
   });
   it('copies files from deps', () => {
     expect(read('bundle.min.js')).toBe('bundle content');

packages/rollup/src/rollup_bundle.bzl

@@ -1,6 +1,6 @@
 "Rules for running Rollup under Bazel"
 
-load("@build_bazel_rules_nodejs//:providers.bzl", "JSEcmaScriptModuleInfo", "NpmPackageInfo", "node_modules_aspect", "run_node")
+load("@build_bazel_rules_nodejs//:providers.bzl", "JSEcmaScriptModuleInfo", "NodeContextInfo", "NpmPackageInfo", "node_modules_aspect", "run_node")
 load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect")
 
 _DOC = """Runs the Rollup.js CLI under Bazel.
@@ -126,6 +126,11 @@ Either this attribute or `entry_point` must be specified, but not both.
         values = ["amd", "cjs", "esm", "iife", "umd", "system"],
         default = "esm",
     ),
+    "node_context_data": attr.label(
+        default = "@build_bazel_rules_nodejs//internal:node_context_data",
+        providers = [NodeContextInfo],
+        doc = "Internal use only",
+    ),
     "output_dir": attr.bool(
         doc = """Whether to produce a directory output.
 
@@ -277,19 +282,21 @@ def _rollup_bundle(ctx):
 
     args.add_all(["--format", ctx.attr.format])
 
+    stamp = ctx.attr.node_context_data[NodeContextInfo].stamp
+
     config = ctx.actions.declare_file("_%s.rollup_config.js" % ctx.label.name)
     ctx.actions.expand_template(
         template = ctx.file.config_file,
         output = config,
         substitutions = {
-            "bazel_stamp_file": "\"%s\"" % ctx.version_file.path if ctx.version_file else "undefined",
+            "bazel_stamp_file": "\"%s\"" % ctx.version_file.path if stamp else "undefined",
         },
     )
 
     args.add_all(["--config", config.path])
     inputs.append(config)
 
-    if ctx.version_file:
+    if stamp:
         inputs.append(ctx.version_file)
 
     # Prevent rollup's module resolver from hopping outside Bazel's sandbox

packages/rollup/test/integration/golden.amd.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 define(['exports', 'some_global_var'], function (exports, some_global_var) { 'use strict';

packages/rollup/test/integration/golden.cjs.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 'use strict';

packages/rollup/test/integration/golden.esm.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 import { thing } from 'some_global_var';

packages/rollup/test/integration/golden.iife.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 var bundle = (function (exports, some_global_var) {

packages/rollup/test/integration/golden.system.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 System.register('bundle', ['some_global_var'], function (exports, module) {

packages/rollup/test/integration/golden.umd.js_

@@ -1,6 +1,6 @@
 /**
  * @license A dummy license banner that goes at the top of the file.
- * This is version v1.2.3
+ * This is version <unknown>
  */
 
 (function (global, factory) {

packages/rollup/test/integration/golden.umd.sha256_

@@ -1 +1 @@
-d5b396f312920156e7a8ef03276e3447009d45e0e481573e3bf27e087f908b2b
+ce71b9fc99b95b713d01ac033ce5a7e47a9f3b3ec56017c4fc99c0fd848d1c58

packages/rollup/test/version_stamp/BUILD.bazel

@@ -1,10 +1,19 @@
 load("@build_bazel_rules_nodejs//internal/golden_file_test:golden_file_test.bzl", "golden_file_test")
 load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle")
+load("//internal/node:context.bzl", "node_context_data")
+
+# Force stamping behavior even in builds without --stamp config
+# by mocking out the config data
+node_context_data(
+    name = "force_stamp",
+    stamp = True,
+)
 
 rollup_bundle(
     name = "version_stamp",
     config_file = "rollup.config.js",
     entry_point = "input.js",
+    node_context_data = ":force_stamp",
     sourcemap = "false",
 )
 

providers.bzl

@@ -51,6 +51,12 @@ transitive_js_ecma_script_module_info = _transitive_js_ecma_script_module_info
 NpmPackageInfo = _NpmPackageInfo
 node_modules_aspect = _node_modules_aspect
 
+#Modelled after _GoContextData in rules_go/go/private/context.bzl
+NodeContextInfo = provider(
+    doc = "Provides data about the build context, like config_setting's",
+    fields = ["stamp"],
+)
+
 # TODO: remove transitive_js_ecma_script_module_info alias before 1.0 release
 NodeRuntimeDepsInfo = _NodeRuntimeDepsInfo
 run_node = _run_node