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

Ban "--platforms" from config_setting #8896

Closed
gregestren opened this issue Jul 15, 2019 · 6 comments
Closed

Ban "--platforms" from config_setting #8896

gregestren opened this issue Jul 15, 2019 · 6 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@gregestren
Copy link
Contributor

gregestren commented Jul 15, 2019

Description of the problem / feature request:

Spawned from this comment thread: https://docs.google.com/a/google.com/document/d/1UZaVcL08wePB41ATZHcxQV4Pu1YfA1RvvWm8FbZHuW8/edit?disco=AAAADQko1J8

We want to discourage select()ing on platforms, in preference for constraint_settings. In other words, it's far preferable to have:

config_setting(
    ...,
    constraint_values = ["@platforms//os:linux"])

than separate config_settings for every specific Linux-based platform definition.

Feature requests: what underlying problem are you trying to solve with this feature?

Sane, maintainable, generic multi-platform support.

More rationale: https://github.com/hlopko/bazel_platforms_examples/blob/4019120264181e491bc148a5e874e1de55b01f58/examples/05_select_on_platform/README.md

Have you found anything relevant by searching the web?

#8583 - would allow embedding this directly in select (no need for an intermediate config_setting)

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Jul 15, 2019
@gregestren gregestren self-assigned this Jul 15, 2019
@gregestren
Copy link
Contributor Author

@katre , @hlopko , @aragos for input. I'm happy to implement if it makes sense.

We could take this even farther and ban everything in PlatformOptions.

This is also an opportunity to conservatively roll out platform support + best practices, then relax these restrictions only when/if we see clear need to.

@gregestren
Copy link
Contributor Author

Also comment from @katre on the original thread:

It would be nice to either forbid it, or at least show an error saying something like "This doesn't work like you think it does". Maybe a similar error for "--host_cpu" and other host flags would also be useful?

@katre
Copy link
Member

katre commented Jul 15, 2019

I like the concept. Ideally it'd be more flexible than just a blacklist in config_setting: possibly a new setting/metadata on individual flags?

@c-parsons
Copy link
Contributor

I like the concept. Ideally it'd be more flexible than just a blacklist in config_setting: possibly a new setting/metadata on individual flags?

+1

I've also had this thought with respect to flags labeled "experimental".

@gregestren
Copy link
Contributor Author

gregestren commented Jul 15, 2019

I like the concept. Ideally it'd be more flexible than just a blacklist in config_setting: possibly a new setting/metadata on individual flags?

Looks like we already have precedent:

* Returns a map from options defined by this fragment to restrictions on whether the option may
* appear in a {@code config_setting}. If an option defined by this fragment is not a key of this
* map, then it has no restriction.
*
* <p>In addition to making options unconditionally non-selectable, this can also be used to gate
* selectability based on the value of other flags in the same fragment -- for instance,
* experimental or incompatible change flags.
*
* <p>The intended usage pattern is to define, for each flag {@code foo} to have a restriction, a
* field
*
* <pre>{@code
* private static final OptionDefinition FOO_DEFINITION =
* OptionsParser.getOptionDefinitionByName(ThisClass.class, "foo");
* }</pre>
*
* This way, if the option is ever renamed (especially common for an experimental flag), if the
* definition is not updated at the same time it will fail-fast during static initialization.
*/
public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {

@gregestren
Copy link
Contributor Author

While there's a broader theme this highlights, this bug should remain focused specifically on --platforms, as that has its own, and wide-ranging, reasons for exclusion. And it's a smaller practical scope.

I'm going to close this since there's no urgency for this yet and I don't think that's happening soon. We can re-open when/if we sense this is truly something to worry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

No branches or pull requests

3 participants