Fix genrule autostamping

[AlessandroPatti]

Genrule does not support conditional stamping based on the value of the --stamp flag. This is because genrule treats the stamp attribute as boolean, while all other rules use the tristate type. After this change, the stamp attribute in genrule can be set to -1 to request conditional stamping.

[AlessandroPatti]

Also noticed that the documentation says:

Bazel never stamps binaries that are built for the exec configuration, regardless of this option or the stamp attribute

This is not true for genrule. Wonder if that is intentional since there's is a test verifying that.

[cdkrot]

This is awesome, thank you @AlessandroPatti.

The documentation on stamp functionality is not exactly complete/consistent, I can confirm

[justinhorvitz]

Thanks for the fix. I'm going to patch this and modify it slightly to better converge with the google-internal genrule implementation.

Bazel never stamps binaries that are built for the exec configuration, regardless of this option or the stamp attribute

This is not true for genrule. Wonder if that is intentional since there's is a test verifying that.

Which test are you referring to? This is a bit of a concern for breaking users that rely on stamped genrules in the exec config, but that is not working as intended.

Edit: after further research, it does appear that this is WAI. Genrules are treated differently than binaries, see following comment.

[justinhorvitz]

Also note that the linked documentation refers to binaries when talking about the exec configuration, not genrules. Internally, genrules are also currently allowed to be stamped (with stamp = 1) in the exec configuration. It will take some effort to see whether disallowing this would break anything (I guess that it will).

[AlessandroPatti]

Which test are you referring to?

bazel/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java

Line 562 in 7f7017a

assertStamped(getExecConfiguredTarget("//u:foo_stamp"));

bazel/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java

Line 573 in 7f7017a

assertStamped(getExecConfiguredTarget("//u:foo_stamp"));

This is a bit of a concern for breaking users that rely on stamped genrules in the exec config, but that is not working as intended.

@justinhorvitz indeed this would be a breaking change and would likely require an incompatible flag, that is why I left it out of this pr, but wanted to call it out.

Also note that the linked documentation refers to binaries when talking about the exec configuration, not genrules

True, it mentions binaries only. Still, I find it odd that binaries and genrule behave differently and it seems desirable for tools to not be stamped to avoid massive cache invalidations. However, if this is the intended behaviour, maybe worth calling out explicitly in the documentation that this is not the case for genrules (what about starlark rules?)?

[justinhorvitz]

Thanks, it took me a bit to realize that you left the exec configuration change out of this PR, but we're on the same page now. I have it out for review.

Most likely it was initially an oversight to allow stamping in the exec (back then host) configuration for genrules. In general, there are a lot of legacy issues with the stamp feature, and if we could redesign it from scratch it would look totally different (no per-rule attributes, only a per-build flag).

Starlark actions are also a mess with stamping. They allow unconditional access to the stamp files via ctx.info_file and ctx.version_file, no attribute necessary. It's probably a good thing this isn't documented, because it encourages stamp-based volatility regardless of the build configuration. I'm trying to get a cleanup effort prioritized to restrict access to the stamp files in starlark.

[brentleyjones]

@bazel-io flag

[keertk]

@bazel-io fork 7.1.0

[iancha1992]

The changes in this PR have been included in Bazel 7.1.0 RC2. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.1.0rc2. Thanks!