bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Option to disallow rules from including stable-status/volatile-status as action inputs

Open alexeagle opened this issue 2 years ago • 7 comments

In working on rules_docker, I found that regardless of any configuration of the build, actions were non-deterministic because the presence of a { character in certain attributes caused rules like container_image to always stamp outputs, and since the stable-status.txt file is an input to the action, it was non-deterministic and caused cache misses.

Since there's no real contract between Bazel and rulesets regarding the meaning of --stamp and how rules should present a non-deterministic stamping facility to users, there's a lot of inconsistency. (For starters, ctx.info_file and ctx.status_file are undocumented) I think we'd benefit from an --incompatible flag on Bazel itself.

As a strawman, call it --incompatible_prevent_status_files_without_stamp which, unless --stamp is also present, would either prevent the *-status.txt files from being passed as inputs to actions, or would error the build and point to the rule implementation that is including those files as inputs.

Related:

  • https://github.com/bazelbuild/bazel/issues/11164
  • https://github.com/bazelbuild/bazel/issues/9363
  • https://github.com/bazelbuild/bazel/issues/10177

alexeagle avatar Nov 29 '21 04:11 alexeagle

+@comius, I don't really know much about stamp so maybe you can comment?

brandjon avatar Jan 11 '22 22:01 brandjon

Ping @comius for triage.

brandjon avatar Feb 10 '22 16:02 brandjon

You mention --stamp, but --embed_label should also be supported, right?

brentleyjones avatar Apr 14 '22 20:04 brentleyjones

Yes, I think anything that produces non-deterministic outputs has to be user-controlled rather than rule-author-controlled.

alexeagle avatar Apr 18 '22 14:04 alexeagle

cc @buildbreaker2021

Trying to keep things simple, I'd prefer if it's possible not to introduce additional flag. That would mean if --stamp is False, ctx.info_file and ctx.version_file should just return a "constant" file. Returning a constant file wouldn't break rules that are already using the functions and would still generate hermetic builds.

comius avatar Apr 19 '22 13:04 comius

Assigning to @buildbreaker2021, because he's handling BuildInfo in Java and C++ Starlark rules atm.

comius avatar Apr 19 '22 13:04 comius

Returning a constant file wouldn't break rules that are already using the functions and would still generate hermetic builds.

I don't think that's true, since it wouldn't be possible to know what a valid constant file would be in the presence of user defined keys, so the constant file could be missing keys that user defined rules expect, and break the rules.

The built in workspace status keys already come with a redacted value, (though there's a comment saying they should be removed https://github.com/bazelbuild/bazel/blob/037368017b5cde7bf99187271f3d139b9fca7d14/src/main/java/com/google/devtools/build/lib/rules/cpp/CppBuildInfo.java#L42-L43), but supporting redacted values for user defined keys would require changing the format of the output of the workspace status script, for example instead of being KEY1<space>VALUE1[\nKEYN<space>VALUEN...] it could be KEY1<space>VALUE1<space>REDACTED_VALUE1[\nKEYN<space>VALUEN<space>REDACTED_VALUEN...]. That would allow users to upgrade bazel while keeping their build compatible with the current version of rule sets, until they upgrade their rule sets to versions that support the new constant files.

uri-canva avatar Aug 02 '22 03:08 uri-canva

I agree with this request. It's a pain now for Starlark to even read the --stamp value (#11164).


Returning a constant file wouldn't break rules that are already using the functions and would still generate hermetic builds.

Unfortunately, there are some caveats with that approach:

  1. --stamp can be altered through transitions
  2. Some rules/scripts (e.g. genrule commands) have hardcoded the bazel-out paths.

(How I have used stamp transitions: I can get both a stamped artifact for deployment, and the digest of the unstamped artifact to detect if a deployment is actually necessary.)

pauldraper avatar Apr 13 '23 19:04 pauldraper

Just got bit by this this week.

AustinSchuh avatar Aug 02 '23 21:08 AustinSchuh