fix: r_binary should respect --stamp flag #37
Conversation
|
This one is a little tricky because I don't think the stamp flag in bazel is a good one to depend on. See I will look at the change in more detail and see if I can give you what you need without depending on |
|
Sorry for my prolonged absence. I have started a discussion on https://groups.google.com/forum/#!topic/bazel-discuss/e4T_XMwn9aE. In the meanwhile, I can give you a per-target stamp attribute. In your builds, to emulate the --nostamp flag, you can use --workspace_status_command=/bin/true to not generate any status in the first place. That will also get you away from subtleties like host configuration targets are not stamped, etc. At GRAIL, in our deployment environments, we set --workspace_status_command to something that generates STABLE_ variables which rebuilds our binaries with the latest stamping information. But in our CI and dev machines, we use a command that does not generate STABLE_ variables and so we continue to get good cache hits while getting stamped binaries, but with stamping information from the original build from which the cache was populated. |
|
cff2986 for excluding R_SESSION_TMPDIR. |
|
No worries. I have temporarily forked to exclude stamping altogether but will give your suggestions a try. Thanks for getting back to this and thoughtful discussion! |
|
Thank you for understanding. I will keep you posted. |
This stamp attribute will determine whether we depend on the stable status file in the runfiles of the binary/test. The volatile status file is always available. This will help people get better cache hits across builds when their workspace status command is outputting STABLE_ vars but an individual test does not really need these vars. NOTE: This is a breaking change for people who were depending on the stable status file in their r_test or r_markdown target. This file will not be available by default because the attribute value is False by default. Volatile status files will continue to be available. See #37 for more context. We expect the usage of this attribute to be limited, and we expect cache hits in stamping to be governed mostly through the management of the content of stable status file. The logic extends to r_markdown. Co-authored-by: forestfang-stripe <[email protected]>
|
Committed 8a4ab37 for the stamp attribute. Note that this changes the default behavior and stable status file is now not included as a runfile by default. The volatile status file continues to be included. Further changes will await discussion on the Google group thread. |
siddharthab
force-pushed
the
master
branch
13 times, most recently
from
18f91df to
d94a12e
Compare
|
I did not get any response on the Google Group thread. I am going to close this PR in favor of the solution currently in place, i.e. always include volatile status file, and include stable status when the stamp attribute is set True on the label. |
|
I did not notice that one of the main issues in this thread was that bazel's remote cache protocol does not treat volatile status file as volatile. As with the stable status variables, my recommendation is to actually make the volatile status files also constant based on an environment variable in your workspace-status command. This solution does not have to rely on which rules respect the See cc9a406 for an example. |
Currently,
r_testalways include stamp files which change over each build/host. This makes test incompatible with bazel remote cache due to changing inputs.Apply a similar fix as bazel-contrib/rules_nodejs#1441 where we respect
--[no]stampflag. Furthermore, adopt similar behavior ascc_binarywherestampcan be overridden on a target by target basis. This is mostly for backward compatible reasons in case users have been consuming{stable,volatile}-status.txton these rules.Added a
r_testtarget in example for easier debugging in the future.Note This is a breaking change where stamp files are no longer available for
r_binary/r_markdown/r_testby default. However, this seems to be the expected behavior for--[no]stamp. I don't expect status files are actually depended on for majority of users.Lastly, exclude
R_SESSION_TMPDIRenvironment variable inr_toolchain_generic_statewhich changes on each toolchain registry for different hosts and sessions. This was also breaking remote caching and sometimes resulted flaky local disk caches as well.