By defaults, stamp only under --stamp configuration.

This follows the example of rules_go and rules_nodejs. It means we don't produce cache-busting outputs unless the user explicitly requests non-deteriminism in the build with --stamp. BREAKING CHANGE: - To get stamped outputs, users must now call bazel with `--stamp` - the stamp attribute is removed from some providers - the stamp attribute is now trinary Fixes bazelbuild#1451
alexeagle · Nov 25, 2021 · cfcff81 · cfcff81
1 parent 0aacc53
commit cfcff81
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 87 deletions.
diff --git a/README.md b/README.md
@@ -253,14 +253,18 @@ to use `container_push` with custom docker authentication credentials.
 
 ## Varying image names
 
-A common request from folks using `container_push` or `container_bundle` is to
+A common request from folks using
+`container_push`, `container_bundle`, or `container_image` is to
 be able to vary the tag that is pushed or embedded.  There are two options
 at present for doing this.
 
 ### Stamping
 
-The first option is to use stamping. Stamping is enabled when a supported
-attribute contains a python format placeholder (e.g. `{BUILD_USER}`).
+The first option is to use stamping.
+Stamping is enabled when bazel is run with `--stamp`.
+This enables replacements in stamp-aware attributes.
+A python format placeholder (e.g. `{BUILD_USER}`)
+is replaced by the value of the corresponding workspace-status variable.
 
 ```python
 # A common pattern when users want to avoid trampling
@@ -272,31 +276,29 @@ container_push(
   # Any of these components may have variables.
   registry = "gcr.io",
   repository = "my-project/my-image",
+  # This will be replaced with the current user when built with --stamp
   tag = "{BUILD_USER}",
 )
 ```
 
+> Rules that are sensitive to stamping can also be forced to stamp or non-stamp mode
+> irrespective of the `--stamp` flag to Bazel. Use the `build_context_data` rule
+> to make a target that provides `StampSettingInfo`, and pass this to the
+> `build_context_data` attribute.
+
 The next natural question is: "Well what variables can I use?"  This
 option consumes the workspace-status variables Bazel defines in
-`stable-status.txt` and `volatile-status.txt`.  These files will appear
-in the target's runfiles:
-
-```shell
-$ bazel build //docker/testdata:push_stamp
-...
-
-$ cat bazel-bin/docker/testdata/push_stamp.runfiles/io_bazel_rules_docker/stable-status.txt
-BUILD_EMBED_LABEL
-BUILD_HOST bazel
-BUILD_USER mattmoor
+`bazel-out/stable-status.txt` and `bazel-out/volatile-status.txt`.
 
-$ cat bazel-bin/docker/testdata/push_stamp.runfiles/io_bazel_rules_docker/volatile-status.txt
-BUILD_TIMESTAMP 1498740967769
+> Note that changes to the stable-status file
+> cause a rebuild of the action, while volatile-status does not.
 
-```
+You can add more stamp variables via `--workspace_status_command`,
+see the [bazel docs](https://docs.bazel.build/versions/master/user-manual.html#workspace_status).
+A common example is to provide the current git SHA, with
+`--workspace_status_command="echo STABLE_GIT_SHA $(git rev-parse HEAD)"`
 
-You can augment these variables via `--workspace_status_command`,
-including through the use of [`.bazelrc`](https://github.com/kubernetes/kubernetes/blob/81ce94ae1d8f5d04058eeb214e9af498afe78ff2/build/root/.bazelrc#L6).
+That flag is typically passed in the `.bazelrc` file, see for example [`.bazelrc` in kubernetes](https://github.com/kubernetes/kubernetes/blob/81ce94ae1d8f5d04058eeb214e9af498afe78ff2/build/root/.bazelrc#L6).
 
 
 ### Make variables

diff --git a/container/bundle.bzl b/container/bundle.bzl
@@ -14,7 +14,7 @@
 """Rule for bundling Container images into a tarball."""
 
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
-load("@io_bazel_rules_docker//container:providers.bzl", "BundleInfo")
+load("@io_bazel_rules_docker//container:providers.bzl", "STAMP_ATTR", "StampSettingInfo", "BundleInfo")
 load(
     "//container:layer_tools.bzl",
     _assemble_image = "assemble",
@@ -40,18 +40,11 @@ def _container_bundle_impl(ctx):
 
     images = {}
     runfiles = []
-    if ctx.attr.stamp:
-        print("Attr 'stamp' is deprecated; it is now automatically inferred. Please remove it from %s" % ctx.label)
-    stamp = False
+    stamp = ctx.attr.stamp[StampSettingInfo].value
     for unresolved_tag in ctx.attr.images:
         # Allow users to put make variables into the tag name.
         tag = ctx.expand_make_variables("images", unresolved_tag, {})
 
-        # If any tag contains python format syntax (which is how users
-        # configure stamping), we enable stamping.
-        if "{" in tag:
-            stamp = True
-
         target = ctx.attr.images[unresolved_tag]
 
         layer = _get_layers(ctx, ctx.label.name, image_target_dict[target])
@@ -101,10 +94,7 @@ container_bundle_ = rule(
         # Implicit dependencies.
         "image_targets": attr.label_list(allow_files = True),
         "images": attr.string_dict(),
-        "stamp": attr.bool(
-            default = False,
-            mandatory = False,
-        ),
+        "stamp": STAMP_ATTR,
         "tar_output": attr.output(),
         "experimental_tarball_format": attr.string(
             values = [

diff --git a/container/image.bzl b/container/image.bzl
@@ -26,6 +26,8 @@ load(
 )
 load(
     "@io_bazel_rules_docker//container:providers.bzl",
+    "STAMP_ATTR",
+    "StampSettingInfo",
     "ImageInfo",
     "LayerInfo",
 )
@@ -105,14 +107,7 @@ def _add_create_image_config_args(
     args.add_all(ctx.attr.ports, before_each = "-ports")
     args.add_all(ctx.attr.volumes, before_each = "-volumes")
 
-    stamp = None
-
-    # If base image is having enabled stamping then it is propagated
-    # to child images.
-    if ctx.attr.stamp == True:
-        stamp = ctx.attr.stamp
-    elif ctx.attr.base and ImageInfo in ctx.attr.base:
-        stamp = ctx.attr.base[ImageInfo].stamp
+    stamp = ctx.attr.stamp[StampSettingInfo].value
 
     if creation_time:
         args.add("-creationTime", creation_time)
@@ -540,31 +535,11 @@ def _impl(
                 ([container_parts["legacy"]] if container_parts["legacy"] else []),
     )
 
-    # Stamp attribute needs to be propagated between definitions to enhance actions
-    # with ability to determine properly whether root image has activated stamping.
-    #
-    # This covers the following example case:
-    # container_image(
-    #     name = “base_image”,
-    #     base = “@base//image”,
-    #     stamp = True,
-    # )
-    #
-    # lang_image(
-    #     base = “:base_image”,
-    # )
-    stamp = None
-    if ctx.attr.stamp:
-        stamp = ctx.attr.stamp
-    elif ctx.attr.base and ImageInfo in ctx.attr.base:
-        stamp = ctx.attr.base[ImageInfo].stamp
-
     return [
         ImageInfo(
             container_parts = container_parts,
             legacy_run_behavior = ctx.attr.legacy_run_behavior,
             docker_run_flags = docker_run_flags,
-            stamp = stamp,
         ),
         DefaultInfo(
             executable = build_executable,
@@ -745,16 +720,7 @@ _attrs = dicts.add(_layer.attrs, {
         Setting this attribute to `gcr.io/dummy` would set the default tag to
         `gcr.io/dummy/package_name:target`.""",
     ),
-    "stamp": attr.bool(
-        default = False,
-        doc = """If true, enable use of workspace status variables
-        (e.g. `BUILD_USER`, `BUILD_EMBED_LABEL`,
-        and custom values set using `--workspace_status_command`)
-        in tags.
-
-        These fields are specified in attributes using Python format
-        syntax, e.g. `foo{BUILD_USER}bar`.""",
-    ),
+    "stamp": STAMP_ATTR,
     "user": attr.string(
         doc = """The user that the image should run as.
 

diff --git a/container/providers.bzl b/container/providers.bzl
@@ -27,7 +27,6 @@ ImageInfo = provider(fields = [
     "container_parts",
     "legacy_run_behavior",
     "docker_run_flags",
-    "stamp",
 ])
 
 # A provider containing information exposed by container_import rules
@@ -54,7 +53,6 @@ PushInfo = provider(fields = [
     "registry",
     "repository",
     "tag",
-    "stamp",
     "stamp_inputs",
     "digest",
 ])
@@ -73,3 +71,30 @@ FilterAspectInfo = provider(
         "depset": "a depset of struct(target=<target>, target_deps=<depset>)",
     },
 )
+
+#Modelled after _GoContextData in rules_go/go/private/context.bzl
+StampSettingInfo = provider(
+    fields = {
+        "value": "Whether stamping is enabled",
+    },
+)
+
+STAMP_ATTR = attr.label(
+    default = "@io_bazel_rules_docker//stamp:use_stamp_flag",
+    providers = [StampSettingInfo],
+    doc = """Whether to encode build information into the output. Possible values:
+
+    - `@io_bazel_rules_docker//stamp:always`:
+        Always stamp the build information into the output, even in [--nostamp][stamp] builds.
+        This setting should be avoided, since it potentially causes cache misses remote caching for
+        any downstream actions that depend on it.
+
+    - `@io_bazel_rules_docker//stamp:never`:
+        Always replace build information by constant values. This gives good build result caching.
+
+    - `@io_bazel_rules_docker//stamp:use_stamp_flag`:
+        Embedding of build information is controlled by the [--[no]stamp][stamp] flag.
+        Stamped binaries are not rebuilt unless their dependencies change.
+
+    [stamp]: https://docs.bazel.build/versions/main/user-manual.html#flag--stamp""",
+)
diff --git a/container/push.bzl b/container/push.bzl
@@ -17,7 +17,7 @@ Bazel rule for publishing images.
 """
 
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
-load("@io_bazel_rules_docker//container:providers.bzl", "PushInfo")
+load("@io_bazel_rules_docker//container:providers.bzl", "STAMP_ATTR", "StampSettingInfo", "PushInfo")
 load(
     "//container:layer_tools.bzl",
     _gen_img_args = "generate_args_for_image",
@@ -61,11 +61,7 @@ def _impl(ctx):
         tag = "$(cat {})".format(_get_runfile_path(ctx, ctx.file.tag_file))
         pusher_input.append(ctx.file.tag_file)
 
-    # If any stampable attr contains python format syntax (which is how users
-    # configure stamping), we enable stamping.
-    if ctx.attr.stamp:
-        print("Attr 'stamp' is deprecated; it is now automatically inferred. Please remove it from %s" % ctx.label)
-    stamp = "{" in tag or "{" in registry or "{" in repository
+    stamp = ctx.attr.stamp[StampSettingInfo].value
     stamp_inputs = [ctx.info_file, ctx.version_file] if stamp else []
     for f in stamp_inputs:
         pusher_args += ["-stamp-info-file", "%s" % _get_runfile_path(ctx, f)]
@@ -139,7 +135,6 @@ def _impl(ctx):
             registry = registry,
             repository = repository,
             tag = tag,
-            stamp = stamp,
             stamp_inputs = stamp_inputs,
             digest = ctx.outputs.digest,
         ),
@@ -179,10 +174,7 @@ container_push_ = rule(
             default = False,
             doc = "Only push images if the digest has changed, default to False",
         ),
-        "stamp": attr.bool(
-            default = False,
-            mandatory = False,
-        ),
+        "stamp": STAMP_ATTR,
         "tag": attr.string(
             default = "latest",
             doc = "The tag of the image.",

diff --git a/docs/BUILD b/docs/BUILD
@@ -1,5 +1,5 @@
-load("@bazel_skylib//rules:write_file.bzl", "write_file")
 load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
 load("@io_bazel_stardoc//stardoc:stardoc.bzl", "stardoc")
 
 # Workaround https://github.com/bazelbuild/stardoc/issues/25

diff --git a/stamp/BUILD.bazel b/stamp/BUILD.bazel
@@ -0,0 +1,27 @@
+"Helpers to determine when to stamp build outputs"
+load(":stamp.bzl", "stamp_setting")
+
+# Detect if the build is running under --stamp
+config_setting(
+    name = "stamp",
+    values = {"stamp": "true"},
+)
+
+# Enable stamping based on the --stamp flag
+stamp_setting(
+    name = "use_stamp_flag",
+    stamp = select({
+        "@io_bazel_rules_docker//stamp:stamp": True,
+        "//conditions:default": False,
+    }),
+)
+
+stamp_setting(
+    name = "always",
+    stamp = True,
+)
+
+stamp_setting(
+    name = "never",
+    stamp = False,
+)
diff --git a/stamp/stamp.bzl b/stamp/stamp.bzl
@@ -0,0 +1,18 @@
+"Helper for determining when to stamp build outputs"
+
+load("@io_bazel_rules_docker//container:providers.bzl", "StampSettingInfo")
+
+def _impl(ctx):
+    return [StampSettingInfo(value = ctx.attr.stamp)]
+
+# Modelled after go_context_data in rules_go
+# Works around github.com/bazelbuild/bazel/issues/1054
+stamp_setting = rule(
+    implementation = _impl,
+    attrs = {
+        "stamp": attr.bool(mandatory = True),
+    },
+    doc = """Determines whether build outputs should be stamped with version control info.
+    
+    Stamping causes outputs to be non-deterministic, resulting in cache misses.""",
+)
diff --git a/testdata/BUILD b/testdata/BUILD
@@ -334,19 +334,20 @@ container_image(
 container_image(
     name = "with_stamped_creation_time",
     base = ":base_with_volume",
+    stamp = "//stamp:always",
     creation_time = "{BUILD_TIMESTAMP}",
-    stamp = True,
 )
 
 container_image(
     name = "with_base_stamped_image",
     base = ":with_stamped_creation_time",
+    stamp = "//stamp:always",
 )
 
 container_image(
     name = "with_default_stamped_creation_time",
     base = ":base_with_volume",
-    stamp = True,
+    stamp = "//stamp:always",
 )
 
 container_image(
@@ -405,10 +406,10 @@ container_image(
 container_image(
     name = "with_stamp_label",
     base = ":base_with_volume",
+    stamp = "//stamp:always",
     labels = {
         "BUILDER": "{BUILD_USER}",
     },
-    stamp = True,
 )
 
 [genrule(
@@ -463,6 +464,7 @@ container_flatten(
 
 docker_push(
     name = "push_stamp",
+    stamp = "//stamp:always",
     image = ":link_with_files_base",
     registry = "gcr.io",
     repository = "convoy-adapter/bazel-oci-test",
@@ -471,6 +473,7 @@ docker_push(
 
 oci_push(
     name = "push_stamp_oci",
+    stamp = "//stamp:always",
     image = ":link_with_files_base",
     registry = "gcr.io",
     repository = "convoy-adapter/bazel-oci-test",
@@ -512,6 +515,7 @@ container_bundle(
 
 container_bundle(
     name = "stamped_bundle_test",
+    stamp = "//stamp:always",
     images = {
         # Pad username so that it is long enough.
         "example.com/aaaaa{BUILD_USER}:stamped": ":with_user",