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

Allow transitions to optionally output settings #18086

Closed
pauldraper opened this issue Apr 13, 2023 · 5 comments
Closed

Allow transitions to optionally output settings #18086

pauldraper opened this issue Apr 13, 2023 · 5 comments
Assignees
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request untriaged

Comments

@pauldraper
Copy link
Contributor

Description of the feature request:

Allow transition functions to optionally outputs settings.

Currently, a transition must output all of its declared outputs, or none of this.

What underlying problem are you trying to solve with this feature?

Optionally returning values is helpful to sometimes workaround #14023 in case settings have not changed.

The current all-or-nothing behavior doesn't prevent the workaround, but it is odd and unclear from the error messages that it is possible to not return some settings, so long as all are excluded. It's a weird middle-ground.

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

No response

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@ShreeM01 ShreeM01 added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Apr 13, 2023
@gregestren
Copy link
Contributor

@sdtwigg for triage.

@sdtwigg
Copy link
Contributor

sdtwigg commented Jun 14, 2023

Sorry for the delay on this one, initially it slipped through the cracks on my end.

This is a good suggestion but also tricky. Requiring all of the settings be specified was an intentional design choice since the initial inception of transitions (and allowing returning None was added later). The thought was that this would avoid error more than not where outputs were mistakingly specified but never used.

That said, we have certainly run into the case where a transition may want to skip some output occasionally based on input settings or attributes. They have to then take those outputs as inputs just to output their current values (while changing others). But, for some native options, they cannot actually be read as inputs yet.

Other team members wanted to ponder this a little further (as the latter case alone is sufficiently compelling to loosen this requirement) so I cannot give a final answer quite yet.

PS: Can you clarify when this was helpful for #14023? Even in the legacy mode, it should skip adding to affected by Starlark transition when the new value is the same as the old. Then, in the newer diff_against_baseline or diff_against_dynamic_baseline modes, it will of course avoid ST-hash when the new options match that baseline.

@pauldraper
Copy link
Contributor Author

Yes, you are correct. I have confirmed this is the case. I didn't realize that when reading #14023 .

While the ergonomics here could probably be improved, there isn't a functional limitation imposed. I am satisfied and am closing this.

@pcjanzen
Copy link
Contributor

We are also affected by the problem that @sdtwigg describes as

But, for some native options, they cannot actually be read as inputs yet.

Is there another issue for that specifically? Otherwise maybe we can keep this open?

@pauldraper
Copy link
Contributor Author

I would suggest opening that as a new issue.

Not being able to read native options is related, but somewhat different problem.

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 type: feature request untriaged
Projects
None yet
Development

No branches or pull requests

6 participants