Skip to content

Use output directory naming scheme diff_against_dynamic_baseline#18561

Closed
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:diff-against-dynamic-baseline
Closed

Use output directory naming scheme diff_against_dynamic_baseline#18561
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:diff-against-dynamic-baseline

Conversation

@fmeum
Copy link
Copy Markdown
Collaborator

@fmeum fmeum commented Jun 2, 2023

Compared to diff_against_baseline, this mode improves caching when top-level flags change that are reset in the exec configuration.

Fixes #18480

@fmeum fmeum requested a review from a team as a code owner June 2, 2023 09:36
@fmeum fmeum requested review from a team, gregestren and sdtwigg and removed request for a team June 2, 2023 09:36
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Jun 2, 2023
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Jun 2, 2023 •
edited
Loading

@linzhp Could you test the change? I did and it seemed to resolve the issue.

@fmeum fmeum force-pushed the diff-against-dynamic-baseline branch from 7e527b9 to 0e72491 Compare June 2, 2023 10:48
linzhp
linzhp approved these changes Jun 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked. Thanks!

@gregestren gregestren removed their request for review June 6, 2023 18:15
@fmeum fmeum mentioned this pull request Jun 6, 2023
Closed
@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Jun 26, 2023

@sdtwigg for triage - do we still need this PR?

@sdtwigg
Copy link
Copy Markdown
Contributor

sdtwigg commented Jun 28, 2023

Okay, it has been about a month since my change was submitted so users have had a chance to test and reported no issues (at least compared to the old diff_against_baseline).

I think it might be reasonable to consider setting this as the new default. Any thoughts?

PS: What is with the objc thing? Can you use the helper-function in CoreOptions instead of the raw equality check?

@aiuto
Copy link
Copy Markdown

aiuto commented Jun 29, 2023

Do you need a global blazerc change before this? We can discuss in the import

@fmeum
Use output directory naming scheme diff_against_dynamic_baseline
f0d06ba 
Compared to `diff_against_baseline`, this mode improves caching when
top-level flags change that are reset in the exec configuration.
@fmeum fmeum force-pushed the diff-against-dynamic-baseline branch from 0e72491 to f0d06ba Compare July 8, 2023 13:20
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Jul 8, 2023 •
edited
Loading

PS: What is with the objc thing? Can you use the helper-function in CoreOptions instead of the raw equality check?

Thanks for the pointer, I wasn't aware of the helper function. Changed.

Do you need a global blazerc change before this? We can discuss in the import

As far as I know google3 already uses a different value than Bazel. If that's still the case this wouldn't be needed.

@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Jul 11, 2023

Do you need a global blazerc change before this? We can discuss in the import

As far as I know google3 already uses a different value than Bazel. If that's still the case this wouldn't be needed.

This is correct.

@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Aug 1, 2023

@sdtwigg Friendly ping, it would be great if we could get this into a rolling release soon so that it can enjoy testing "in the wild" well before the 7.0.0 branch cut.

@gregestren gregestren mentioned this pull request Aug 15, 2023
Closed
@gregestren gregestren removed the request for review from a team August 16, 2023 20:18
@linzhp linzhp mentioned this pull request Aug 19, 2023
Closed
@sdtwigg sdtwigg 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 Aug 25, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 26, 2023
@fmeum fmeum deleted the diff-against-dynamic-baseline branch August 31, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@linzhp linzhp linzhp approved these changes

@sdtwigg sdtwigg sdtwigg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sdtwigg sdtwigg

Labels

team-Configurability platforms, toolchains, cquery, select(), config transitions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

--experimental_output_directory_naming_scheme=diff_against_baseline caused Bazel to rebuild whenever --run_under or --test_env change

5 participants

@fmeum @gregestren @sdtwigg @aiuto @linzhp