Enable revised output directory hash suffix computation #16910

fmeum · 2022-12-02T21:28:15Z

Flips --experimental_output_directory_naming_scheme and --experimental_exec_configuration_distinguisher to diff_against_baseline and off, respectively, which makes it so that Starlark transitions can return to previous configurations, thus improving cache hit rates.

Also changes the fixed platform suffix in the exec configuration from an empty string to "exec" for compatibility with logic that looks for the substring -exec- in paths to determine whether the current configuration is the exec configuration.

As a result of this commit, output directory paths can change in the following ways:

-ST-<hash> suffixes can change or disappear and, in rare cases, appear where they previously didn't
for exec configuration directories, k8-opt-exec-2B5CBBC6 becomes k8-opt-exec-ST-011b9bdc32ed

This may result in tool paths that are seven characters longer, which can cause builds on Windows to fail that were already very close to the MAX_PATH limit. A follow-up commit will reduce the length of the hash by three characters, bringing that increase down to four.

Work towards #14023

fmeum · 2022-12-02T21:28:35Z

@gregestren

(There is a very unexpected integration test failure on Windows that I still have to debug)

fmeum · 2022-12-03T21:42:09Z

Stacked on #16916, which fixes the pipeline. The change made a path to an Android tool slightly longer, which triggered a dormant bug in the Java launcher fixed by the linked PR.

fmeum · 2022-12-04T11:57:49Z

@meteorcloudy As explained in #14023 (comment), this change would be rather important for rules_go in order to support Go 1.20 (due February 2023). It also generally improves build times and reduces target graph sizes for transition-heavy builds.

This flag only affects the -ST-<hash> part of output paths, with the hash being an intentionally undocumented implementation detail. @gregestren arrived at the conclusion that this would also be safe to cherry-pick in a minor release in #14023 (comment). I will flag it for cherry-picking and let you decide whether and if so which version to cherry-pick this into.

Would it be possible to get a downstream pipeline for this change? It is currently stacked on #16916, which is a fix for an unrelated bug that caused Java tools to fail if their paths get too long (the flag flip can make paths slightly longer or shorter).

fmeum · 2022-12-04T11:59:51Z

@bazel-io flag

meteorcloudy · 2022-12-05T12:01:09Z

Like #16916, I think we should cherry pick them in 6.1

meteorcloudy · 2022-12-05T12:09:45Z

A downstream test on this change is initiated here: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2765

Note that there are already some failures with Bazel@HEAD: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2764 (we're looking into fixing them)

keertk · 2022-12-07T16:04:35Z

@bazel-io fork 6.1.0

fmeum · 2022-12-09T14:21:04Z

Downstream test failures on Windows such as this one may be a result of paths becoming longer as there are now two hash fragment in the exec configuration.

@sdtwigg Given that the platform is already part of the ST hash (please correct me if I'm wrong here!), could we also flip --experimental_exec_configuration_distinguisher to off (or some other value) to mitigate this? If not, could we merge the hashes into a single one?

fmeum · 2023-03-08T16:40:06Z

@sdtwigg I have also thought about this more and now feel that we should not cherry-pick this change. We haven't heard from rules_go users that they have seen their build times regress dramatically with Go 1.20 and as time passes, Bazel 7.0 comes closer and with it this commit.

sdtwigg · 2023-03-08T16:47:14Z

If we do not add this to 6.X, I do think we should submit this to 7.0.

(I'm a bit lost as to whether that means this is fine as-is being merged into bazelbuild:master or if another branch is more appropriate.)

fmeum · 2023-03-08T16:48:38Z

(I'm a bit lost as to whether that means this is fine as-is being merged into bazelbuild:master or if another branch is more appropriate.)

It's fine to merge to master, Bazel 6.X releases are handled via the bazel-6.X.Y branches.

Flips `--experimental_output_directory_naming_scheme` and `--experimental_exec_configuration_distinguisher` to `diff_against_baseline` and `off`, respectively, which makes it so that Starlark transitions can return to previous configurations, thus improving cache hit rates. Also changes the fixed platform suffix in the exec configuration from an empty string to `"exec"` for compatibility with logic that looks for the substring `-exec-` in paths to determine whether the current configuration is the exec configuration. As a result of this commit, output directory paths can change in the following ways: * `-ST-<hash>` suffixes can change or disappear and, in rare cases, appear where they previously didn't * for exec configuration directories, `k8-opt-exec-2B5CBBC6` becomes `k8-opt-exec-ST-011b9bdc32ed` This may result in tool paths that are seven characters longer, which can cause builds on Windows to fail that were already very close to the `MAX_PATH` limit. A follow-up commit will reduce the length of the hash by three characters, bringing that increase down to four. Work towards bazelbuild#14023

fmeum · 2023-03-09T10:30:46Z

@sdtwigg Just to make sure this isn't lost: If you want this PR to be merged, I think that you need to the awaiting-merge label.

com6056 · 2023-03-10T18:58:22Z

@sdtwigg I have also thought about this more and now feel that we should not cherry-pick this change. We haven't heard from rules_go users that they have seen their build times regress dramatically with Go 1.20 and as time passes, Bazel 7.0 comes closer and with it this commit.

@sdtwigg this is impacting us pretty badly, the recompiling of GoStdlib every time is pretty painful for us, any chance we can reconsider getting this cherry-picked instead into a Bazel 6 release? 🙏

gregestren · 2023-03-10T20:38:50Z

@com6056 what team do you represent?

Is it possible to manually set these flags for your own builds? Did rules_go need any other updates to its own code in response to these changes?

com6056 · 2023-03-10T21:15:38Z

@com6056 what team do you represent?

Is it possible to manually set these flags for your own builds? Did rules_go need any other updates to its own code in response to these changes?

I'm at Mux (https://github.com/muxinc), and what flags would we have to set? Is it just these?

--experimental_output_directory_naming_scheme=diff_against_baseline
--experimental_exec_configuration_distinguisher=off

If so, ya we can totally just set those for now in our .bazelrc if it is that easy 🎉

fmeum · 2023-03-10T23:14:13Z

I would recommend setting only the first one for now. The second one may break some rulesets that rely on specific output path patterns to detect the exec configuration, which is fixed by the current PR.

fmeum · 2023-03-17T10:31:40Z

@gregestren @sdtwigg Friendly ping, is this in the process of being imported? Just want to make sure it hasn't gotten lost.

gregestren · 2023-03-17T22:25:39Z

@sdtwigg is still handling the import process. I last saw a code update from him earlier today.

com6056 · 2023-04-05T17:24:22Z

Any updates on this? And is there any chance we could get the fix for using --experimental_exec_configuration_distinguisher=off cherry-picked into Bazel 6? 🙏

com6056 · 2023-04-05T17:54:22Z

I would recommend setting only the first one for now. The second one may break some rulesets that rely on specific output path patterns to detect the exec configuration, which is fixed by the current PR.

Do you know which rulesets are known to break with the second one as it is currently implemented in Bazel 6.1.1?

gregestren · 2023-04-05T18:11:03Z

@sdtwigg is the import ready?

sdtwigg · 2023-04-05T18:14:20Z

I had approved the import internally but it looks like it breaks some tests and so was not actually submitted. I was not re-added to the attention set when this happened so I had just been assuming everything was proceeding fine. Planning to look into those test breakages ASAP. I speculate it is due to those tests hard-coding lookups to intermediate artifacts.

…

On Wed, Apr 5, 2023 at 2:11 PM Greg ***@***.***> wrote: @sdtwigg <https://github.com/sdtwigg> is the import ready? — Reply to this email directly, view it on GitHub <#16910 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHX76DI5ZI53SNZ3FNXYADW7WYUHANCNFSM6AAAAAASSJ7BCQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

fmeum · 2023-04-05T18:27:40Z

@sdtwigg #17474 may be required to prevent downstream breakages in Bazel CI on Windows due to path length increases. That one would end up changing paths again.

gregestren · 2023-04-07T14:14:43Z

@sdtwigg have been having some back and forth on the internal tests. I think he's pretty close to unblocking them.

One of them is a shell test for path stripping. It assumed bazel-out/k8-fastbuild/bin/... on an Android binary. But these changes add a new hash to Android split transition that impacts Android binaries. So that needed a small update.

@fmeum we should export that test to GitHub. The only reason it's Google-internal is because only the Google executor has API support for path stripping before you changes, as you know.

Flips `--experimental_output_directory_naming_scheme` and `--experimental_exec_configuration_distinguisher` to `diff_against_baseline` and `off`, respectively, which makes it so that Starlark transitions can return to previous configurations, thus improving cache hit rates. Also changes the fixed platform suffix in the exec configuration from an empty string to `"exec"` for compatibility with logic that looks for the substring `-exec-` in paths to determine whether the current configuration is the exec configuration. As a result of this commit, output directory paths can change in the following ways: * `-ST-<hash>` suffixes can change or disappear and, in rare cases, appear where they previously didn't * for exec configuration directories, `k8-opt-exec-2B5CBBC6` becomes `k8-opt-exec-ST-011b9bdc32ed` This may result in tool paths that are seven characters longer, which can cause builds on Windows to fail that were already very close to the `MAX_PATH` limit. A follow-up commit will reduce the length of the hash by three characters, bringing that increase down to four. Work towards bazelbuild#14023 Closes bazelbuild#16910. PiperOrigin-RevId: 523999034 Change-Id: Id5548a00b62ebfe7889cd40177995210ecc224c1

sgowroji added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Dec 3, 2022

fmeum marked this pull request as draft December 3, 2022 09:58

fmeum force-pushed the flip_output_directory_naming_scheme branch 11 times, most recently from d9136dd to a40b6a0 Compare December 3, 2022 21:41

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 4, 2022

bazel-io mentioned this pull request Dec 7, 2022

[6.2.0] Default experimental_output_directory_naming_scheme to diff_against_baseline #16948

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 7, 2022

fmeum force-pushed the flip_output_directory_naming_scheme branch from a40b6a0 to d9d640f Compare December 9, 2022 14:10

fmeum marked this pull request as ready for review December 9, 2022 14:10

fmeum force-pushed the flip_output_directory_naming_scheme branch 3 times, most recently from 0854b1c to 5a56183 Compare January 2, 2023 19:51

fmeum force-pushed the flip_output_directory_naming_scheme branch from 16f3ecd to 84f7911 Compare March 8, 2023 17:08

sdtwigg approved these changes Mar 9, 2023

View reviewed changes

meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 9, 2023

fmeum mentioned this pull request Mar 10, 2023

switching from bazel test to coverage causes GoStdlib to be rebuilt with go1.20 bazel-contrib/rules_go#3472

Closed

fmeum mentioned this pull request Apr 6, 2023

Diff against a dedicated exec version of the baseline configuration #18002

Closed

copybara-service bot closed this in 94d8bd3 Apr 13, 2023

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 13, 2023

fmeum deleted the flip_output_directory_naming_scheme branch April 13, 2023 14:53

ShreeM01 mentioned this pull request Apr 13, 2023

[6.2.0]Enable revised output directory hash suffix computation #18088

Closed

jadenPete mentioned this pull request Nov 13, 2024

Implement a toolchain lucidsoftware/rules_scala#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable revised output directory hash suffix computation #16910

Enable revised output directory hash suffix computation #16910

fmeum commented Dec 2, 2022 •

edited

Loading

fmeum commented Dec 2, 2022 •

edited

Loading

fmeum commented Dec 3, 2022 •

edited

Loading

fmeum commented Dec 4, 2022 •

edited

Loading

fmeum commented Dec 4, 2022

meteorcloudy commented Dec 5, 2022

meteorcloudy commented Dec 5, 2022

keertk commented Dec 7, 2022

fmeum commented Dec 9, 2022 •

edited

Loading

fmeum commented Mar 8, 2023

sdtwigg commented Mar 8, 2023

fmeum commented Mar 8, 2023

fmeum commented Mar 9, 2023

com6056 commented Mar 10, 2023

gregestren commented Mar 10, 2023

com6056 commented Mar 10, 2023 •

edited

Loading

fmeum commented Mar 10, 2023 •

edited

Loading

fmeum commented Mar 17, 2023

gregestren commented Mar 17, 2023

com6056 commented Apr 5, 2023

com6056 commented Apr 5, 2023 •

edited

Loading

gregestren commented Apr 5, 2023

sdtwigg commented Apr 5, 2023 via email

fmeum commented Apr 5, 2023 •

edited

Loading

gregestren commented Apr 7, 2023

Enable revised output directory hash suffix computation #16910

Enable revised output directory hash suffix computation #16910

Conversation

fmeum commented Dec 2, 2022 • edited Loading

fmeum commented Dec 2, 2022 • edited Loading

fmeum commented Dec 3, 2022 • edited Loading

fmeum commented Dec 4, 2022 • edited Loading

fmeum commented Dec 4, 2022

meteorcloudy commented Dec 5, 2022

meteorcloudy commented Dec 5, 2022

keertk commented Dec 7, 2022

fmeum commented Dec 9, 2022 • edited Loading

fmeum commented Mar 8, 2023

sdtwigg commented Mar 8, 2023

fmeum commented Mar 8, 2023

fmeum commented Mar 9, 2023

com6056 commented Mar 10, 2023

gregestren commented Mar 10, 2023

com6056 commented Mar 10, 2023 • edited Loading

fmeum commented Mar 10, 2023 • edited Loading

fmeum commented Mar 17, 2023

gregestren commented Mar 17, 2023

com6056 commented Apr 5, 2023

com6056 commented Apr 5, 2023 • edited Loading

gregestren commented Apr 5, 2023

sdtwigg commented Apr 5, 2023 via email

fmeum commented Apr 5, 2023 • edited Loading

gregestren commented Apr 7, 2023

fmeum commented Dec 2, 2022 •

edited

Loading

fmeum commented Dec 2, 2022 •

edited

Loading

fmeum commented Dec 3, 2022 •

edited

Loading

fmeum commented Dec 4, 2022 •

edited

Loading

fmeum commented Dec 9, 2022 •

edited

Loading

com6056 commented Mar 10, 2023 •

edited

Loading

fmeum commented Mar 10, 2023 •

edited

Loading

com6056 commented Apr 5, 2023 •

edited

Loading

fmeum commented Apr 5, 2023 •

edited

Loading