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

The platforms flag should be single-valued #19807

Open
katre opened this issue Oct 12, 2023 · 8 comments
Open

The platforms flag should be single-valued #19807

katre opened this issue Oct 12, 2023 · 8 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@katre
Copy link
Member

katre commented Oct 12, 2023

Description of the feature request:

Right now, the --platforms flag accepts multiple values but only the first is actually used.

Instead, the flag should be single-valued and give an error if multiple values are passed.

Further, the flag should actually be --platform and uses should be migrated, but that may be harder to manage.

Which category does this issue belong to?

Configurability

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

Poor decision decisions at the start of the platforms project

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

@katre katre added P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions and removed untriaged labels Oct 12, 2023
@katre
Copy link
Member Author

katre commented Oct 12, 2023

@sdtwigg We discussed this recently

@chaselatta
Copy link

I put together a quick repro case which shows the issue we discussed. https://github.com/chaselatta/platform_bug_repro

@fmeum
Copy link
Collaborator

fmeum commented Oct 12, 2023

I remember a design doc by @brentleyjones that (if I recall correctly) gave meaning to a multi-valued --platforms flag: A target is built for the first platform in the list that it is target_compatible_with, which allows for pretty clean flagless multi-platform builds. I am noting this here to make sure that if we migrate, we are very sure that we aren't going to migrate back eventually.

@brentleyjones
Copy link
Contributor

Yes, I would like to confirm if this means we will never adopt something like my proposal. And if so, if there are any future plans at all to support multi-platform builds in a nice way?

@katre
Copy link
Member Author

katre commented Oct 13, 2023

@brentleyjones: Your proposal was very interesting, and we may decide to take it up in the future. At this point, however, I think the best approach is the one from https://github.com/bazelbuild/proposals/blob/main/designs/2023-06-08-standard-platform-transitions.md, where the controls are in BUILD files and Starlark rules.

The reason to handle this now are to reduce confusion, both for Bazel users (who frequently ask about what happens when they pass multiple values to --platforms) and for Bazel developers (who keep having to add defensive code around PlatformOptions.platforms).

If we decide to implement either @brentleyjones's proposal or something similar, we can change the flag semantics again to match whatever we decide to implement.

@katre
Copy link
Member Author

katre commented Nov 27, 2023

Copying parts of my comment from #6519:

Steps for cleaning up --platforms:

  1. Make the existing multi-valued flag single-valued, but leave the name.
  2. Actually rename the flag to --platform (flag migrations are hard, sorry), leaving --platforms as a deprecated legacy name.
  3. Actually remove --platforms.

@fmeum
Copy link
Collaborator

fmeum commented Nov 27, 2023

@katre Would step 1 include making it single-valued within transition implementation functions as well? I could see that being very breaking.

Could we keep //command_line_option:platforms list-valued until it is removed and introduce //command_line_option:platform as the separate single-valued alternative?

@katre
Copy link
Member Author

katre commented Nov 27, 2023

Yes, that's part of why we're not moving on this faster.

Having two different flags also seems confusing. When we pick this up we'll consider which is less bad.

I consider multi-valued --platforms as one of the two biggest mistakes I made when starting the platforms project (the other was a separate toolchains parameter to rule instead of a new attr.toolchain attribute type).

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

No branches or pull requests

7 participants
@brentleyjones @katre @chaselatta @fmeum @Pavank1992 @iancha1992 and others