From 6dc5906f17df1862e58c6998e214020128e6cacf Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 2 Jun 2021 12:39:03 -0700 Subject: [PATCH] Base stamping decisions on --stamp configuration, not attributes on the rule 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 was removed from some providers - the stamp attribute was deprecated on container_push and container_bundle, it is now removed - the stamp attribute was removed from container_image, there is no replacement Fixes #1451 --- README.md | 45 ++++++++++++----------------------------- container/BUILD | 5 ----- container/bundle.bzl | 17 +++------------- container/image.bzl | 43 ++++----------------------------------- container/providers.bzl | 24 ++++++++++++++++++++-- container/push.bzl | 15 +++----------- docker/BUILD | 17 ++++++++++++++++ docker/context.bzl | 19 +++++++++++++++++ testdata/BUILD | 28 +------------------------ 9 files changed, 82 insertions(+), 131 deletions(-) create mode 100644 docker/context.bzl diff --git a/README.md b/README.md index ccc0ccf69..65dfc49fb 100644 --- a/README.md +++ b/README.md @@ -256,8 +256,15 @@ 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 a bazel is run with `--stamp`. +This enables replacements in stamp-aware attributes, where a python format placeholder +(e.g. `{BUILD_USER}`) +is replaced by the corresponding key from the `bazel-out/stable-status.txt` or +`bazel-out/volatile-status.txt` file. +(Note that changes to the stable-status file +cause a rebuild of the action, while volatile-status does not. +) ```python # A common pattern when users want to avoid trampling @@ -293,7 +300,9 @@ BUILD_TIMESTAMP 1498740967769 ``` 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). +see the [bazel docs](https://docs.bazel.build/versions/master/user-manual.html#workspace_status). + +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 @@ -1546,20 +1555,6 @@ configuration. See [here](#container_push-custom-client-configuration) for detai

This field supports stamp variables.

- - stamp - -

Bool; optional

-

Deprecated: it is now automatically inferred.

-

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 the tag using Python format - syntax, e.g. - example.org/{BUILD_USER}/image:{BUILD_EMBED_LABEL}.

- - @@ -1794,26 +1789,12 @@ A rule that aliases and saves N images into a single `docker save` tarball.

A collection of the images to save into the tarball.

The keys are the tags with which to alias the image specified by the value. These tags may contain make variables ($FOO), - and if stamp is set to true, may also contain workspace + and may also contain workspace status variables ({BAR}).

The values may be the output of container_pull, container_image, or a docker save tarball.

- Toolchains - stamp - -

Bool; optional

-

Deprecated: it is now automatically inferred.

-

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 the tag using Python format - syntax, e.g. - example.org/{BUILD_USER}/image:{BUILD_EMBED_LABEL}.

- - diff --git a/container/BUILD b/container/BUILD index c4765b856..44d071be7 100644 --- a/container/BUILD +++ b/container/BUILD @@ -73,9 +73,6 @@ TEST_TARGETS = [ ":with_unix_epoch_creation_time", ":with_millisecond_unix_epoch_creation_time", ":with_rfc_3339_creation_time", - ":with_stamped_creation_time", - ":with_default_stamped_creation_time", - ":with_base_stamped_image", ":with_env", ":layers_with_env", ":with_double_env", @@ -100,7 +97,6 @@ TEST_TARGETS = [ # TODO(mattmoor): Re-enable once archive is visible # "extras_with_deb", ":bundle_test", - ":stamped_bundle_test", ":pause_based", ":dashdash_entrypoint", ":base_based_warning", @@ -126,7 +122,6 @@ TEST_DATA = [ ] + [ "//testdata:push_compression_gzip_fast", "//testdata:push_compression_gzip_normal", - "//testdata:stamped_bundle_test", "//testdata:stamp_info_file.txt", "//tests/container:basic_windows_image.tar", ] diff --git a/container/bundle.bzl b/container/bundle.bzl index 6f2d9d600..785f38a8d 100644 --- 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", "BUILD_CONTEXT_ATTRS", "BuildContextInfo", "BundleInfo") load( "//container:layer_tools.bzl", _assemble_image = "assemble", @@ -38,18 +38,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.build_context_data[BuildContextInfo].stamp 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]) @@ -96,15 +89,11 @@ def _container_bundle_impl(ctx): ] container_bundle_ = rule( - attrs = dicts.add({ + attrs = dicts.add(BUILD_CONTEXT_ATTRS, { "image_target_strings": attr.string_list(), # Implicit dependencies. "image_targets": attr.label_list(allow_files = True), "images": attr.string_dict(), - "stamp": attr.bool( - default = False, - mandatory = False, - ), "tar_output": attr.output(), }, _layer_tools), executable = True, diff --git a/container/image.bzl b/container/image.bzl index b40c07620..dca2297af 100644 --- a/container/image.bzl +++ b/container/image.bzl @@ -26,6 +26,8 @@ load( ) load( "@io_bazel_rules_docker//container:providers.bzl", + "BUILD_CONTEXT_ATTRS", + "BuildContextInfo", "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.build_context_data[BuildContextInfo].stamp 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, @@ -577,7 +552,7 @@ def _impl( ), ] -_attrs = dicts.add(_layer.attrs, { +_attrs = dicts.add(BUILD_CONTEXT_ATTRS, _layer.attrs, { "architecture": attr.string( doc = "The desired CPU architecture to be used as label in the container image.", default = "amd64", @@ -745,16 +720,6 @@ _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`.""", - ), "user": attr.string( doc = """The user that the image should run as. diff --git a/container/providers.bzl b/container/providers.bzl index c67dbedad..04fbe7d66 100644 --- 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,25 @@ FilterAspectInfo = provider( "depset": "a depset of struct(target=, target_deps=)", }, ) + +#Modelled after _GoContextData in rules_go/go/private/context.bzl +BuildContextInfo = provider( + doc = "Provides data about the build context, like config_setting's", + fields = { + "stamp": "If stamping is enabled", + }, +) + +BUILD_CONTEXT_ATTRS = { + "build_context_data": attr.label( + default = "@io_bazel_rules_docker//docker:build_context_data", + providers = [BuildContextInfo], + doc = """Provides info about the build context, such as stamping. + +By default it reads from the bazel command line, such as the `--stamp` argument. +Use this to override values for this target, such as enabling or disabling stamping. +You can use the `build_context_data` rule +to create a BuildContextInfo and override the --stamp config setting. +""", + ), +} diff --git a/container/push.bzl b/container/push.bzl index 032d1232d..62eb00419 100644 --- 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", "BUILD_CONTEXT_ATTRS", "BuildContextInfo", "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.build_context_data[BuildContextInfo].stamp 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,14 +135,13 @@ def _impl(ctx): registry = registry, repository = repository, tag = tag, - stamp = stamp, stamp_inputs = stamp_inputs, digest = ctx.outputs.digest, ), ] _container_push = rule( - attrs = dicts.add({ + attrs = dicts.add(BUILD_CONTEXT_ATTRS, { "extension": attr.string( doc = "(optional) The file extension for the push script.", ), @@ -179,10 +174,6 @@ _container_push = rule( default = False, doc = "Only push images if the digest has changed, default to False", ), - "stamp": attr.bool( - default = False, - mandatory = False, - ), "tag": attr.string( default = "latest", doc = "(optional) The tag of the image, default to 'latest'.", diff --git a/docker/BUILD b/docker/BUILD index a7f15b237..c8d9abec8 100644 --- a/docker/BUILD +++ b/docker/BUILD @@ -13,6 +13,7 @@ # limitations under the License. load("@bazel_skylib//:bzl_library.bzl", "bzl_library") +load(":context.bzl", "build_context_data") package(default_visibility = ["//visibility:public"]) @@ -26,3 +27,19 @@ bzl_library( "@io_bazel_rules_docker//toolchains/docker", ], ) + +# 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 +build_context_data( + name = "build_context_data", + stamp = select({ + "@io_bazel_rules_docker//docker:stamp": True, + "//conditions:default": False, + }), +) diff --git a/docker/context.bzl b/docker/context.bzl new file mode 100644 index 000000000..d6b5d239e --- /dev/null +++ b/docker/context.bzl @@ -0,0 +1,19 @@ +"build_context_data rule" + +load("@io_bazel_rules_docker//container:providers.bzl", "BuildContextInfo") + +_DOC = """build_context_data gathers information about the build configuration. +It is a common dependency of all targets that are sensitive to configuration.""" + +def _impl(ctx): + return [BuildContextInfo(stamp = ctx.attr.stamp)] + +# Modelled after go_context_data in rules_go +# Works around github.com/bazelbuild/bazel/issues/1054 +build_context_data = rule( + implementation = _impl, + attrs = { + "stamp": attr.bool(mandatory = True), + }, + doc = _DOC, +) diff --git a/testdata/BUILD b/testdata/BUILD index 091cd5a33..82ff3f3e0 100644 --- a/testdata/BUILD +++ b/testdata/BUILD @@ -331,24 +331,6 @@ container_image( creation_time = "1989-05-03T12:58:12.345Z", ) -container_image( - name = "with_stamped_creation_time", - base = ":base_with_volume", - creation_time = "{BUILD_TIMESTAMP}", - stamp = True, -) - -container_image( - name = "with_base_stamped_image", - base = ":with_stamped_creation_time", -) - -container_image( - name = "with_default_stamped_creation_time", - base = ":base_with_volume", - stamp = True, -) - container_image( name = "with_env", base = ":base_with_volume", @@ -406,9 +388,9 @@ container_image( name = "with_stamp_label", base = ":base_with_volume", labels = { + # Will be replaced when bazel is run with --stamp "BUILDER": "{BUILD_USER}", }, - stamp = True, ) [genrule( @@ -510,14 +492,6 @@ container_bundle( }, ) -container_bundle( - name = "stamped_bundle_test", - images = { - # Pad username so that it is long enough. - "example.com/aaaaa{BUILD_USER}:stamped": ":with_user", - }, -) - # Generate a dummy debian package with a test/ directory py_binary( name = "gen_deb",