bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23k stars 4.03k forks source link

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

Open alexeagle opened 2 years ago

alexeagle commented 2 years ago

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:

brandjon commented 2 years ago

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

brandjon commented 2 years ago

Ping @comius for triage.

brentleyjones commented 2 years ago

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

alexeagle commented 2 years ago

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

comius commented 2 years ago

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 commented 2 years ago

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

uri-canva commented 2 years ago

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.

pauldraper commented 1 year ago

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.)

AustinSchuh commented 1 year ago

Just got bit by this this week.