Skip to content

Base stamping decisions on --stamp configuration, not attributes on t… - #1878

Merged
alexeagle merged 1 commit into
bazelbuild:masterfrom
alexeagle:stamping
Nov 26, 2021
Merged

Base stamping decisions on --stamp configuration, not attributes on t…#1878
alexeagle merged 1 commit into
bazelbuild:masterfrom
alexeagle:stamping

Conversation

@alexeagle

Copy link
Copy Markdown
Contributor

…he 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

@google-cla google-cla Bot added the cla: yes label Jun 2, 2021

@gravypod gravypod left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an update to README.md that explains this behavior?

@alexeagle
alexeagle force-pushed the stamping branch 7 times, most recently from e9ff310 to 4ccc7a5 Compare June 4, 2021 13:59
@alexeagle
alexeagle marked this pull request as ready for review June 4, 2021 14:02
@alexeagle
alexeagle requested review from pcj and smukherj1 as code owners June 4, 2021 14:02

@gravypod gravypod left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good but as we discussed we should wait for a major version change whenever we're ready.

Comment thread README.md
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`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be configured with .bazelrc? I know some users want to always stamp containers so showing them an easy to use setup to make that always happen is probably a good idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to still have a stamp attribute on our three stamp-aware rules, and that attribute is now a trinary. So you can still use stamp=always behavior like before.

@alexeagle

Copy link
Copy Markdown
Contributor Author

Okay so what's our cadence for major releases?
Since we haven't released a 1.0 semver dictates that breaking changes are in every minor. So we don't have a way to signal this beyond either just a 0.18.0 including this change.
I think we should have a 1.0. Could discuss doing that now? holding this change for that doesn't make a difference for semver though

@gravypod

gravypod commented Jun 5, 2021 via email

Copy link
Copy Markdown
Collaborator

@alexeagle

Copy link
Copy Markdown
Contributor Author

If we do 1.0 we need to say what that means and how we'll version in the future. Can discuss at our meeting Monday but I'll have to duck out early due to conflict

@alexeagle alexeagle added this to the 1.0 milestone Jun 7, 2021
@alexeagle
alexeagle force-pushed the stamping branch 2 times, most recently from 5eb0478 to cfcff81 Compare November 25, 2021 04:58
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

Copy link
Copy Markdown
Contributor Author

fyi @aptenodytes-forsteri rebased and landable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stamping results in different digest with remote cache

2 participants