Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Remove outdated "output directory name" option." #16844

Closed

Conversation

torgil
Copy link
Contributor

@torgil torgil commented Nov 24, 2022

This reverts commit af0e20f.

@torgil
Copy link
Contributor Author

torgil commented Nov 24, 2022

See discussion in #14236

@sgowroji sgowroji added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Nov 28, 2022
@torgil torgil force-pushed the output-directory-name-option branch from 3b6df10 to 52d8cf4 Compare December 2, 2022 15:57
@gregestren
Copy link
Contributor

@sdtwigg triage / close?

@gregestren gregestren requested a review from sdtwigg June 26, 2023 15:15
@sdtwigg
Copy link
Contributor

sdtwigg commented Jun 28, 2023

Sorry for the delay here, an initial skim of the title missed the effective double negative so I was reading it as just "remove outdated output directory name option" and thus thought it was the initial PR for a change that went through and thus was an accidental artifact.

That said, we broadly cannot trust the user to override and set the output directory name. There is too much nuance in making sure action conflicts are avoided, especially as the situation gets more complex with deeper build graphs and more transitions. This is actually why I am doing a heavy push to have output directory be computed directly by looking at just the current set of options (and maybe comparing to a baseline) rather than relying on the transition or transition infrastructure to do the right thing immediately.

Instead, I think we should fix the underlying issue by more judicious use of EXPLICIT_IN_OUTPUT_PATH on options as well as named configurations/transitions and better unifying behaviors with the exec transition. Given all of the work in this space since the initial underlying issue was files, could you re-evaluate if it is still a problem (especially when using --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline) then re-submit a fresh issue?

@sdtwigg sdtwigg closed this Jun 28, 2023
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants