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.

Sign up for GitHub

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

Jump to bottom

Implement Platform-Based Flags #19409

Closed
6 tasks
katre opened this issue Sep 5, 2023 · 2 comments
Closed
6 tasks

Implement Platform-Based Flags #19409

katre opened this issue Sep 5, 2023 · 2 comments
Assignees
@katre
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@katre
Copy link
Member

katre commented Sep 5, 2023

This is a tracking issue for the implementation of Platform-Based Flags.

Incomplete Task list:

  • Add a flags attribute to platform
  • Decide if this needs to be in the PlatformInfo provider
  • Prototype reading target attributes from inside BuildConfigurationValue
  • Add a lot of tests
  • Add more tests
@katre katre self-assigned this Sep 5, 2023
@katre katre added type: feature request P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Sep 5, 2023
copybara-service bot pushed a commit that referenced this issue Sep 15, 2023
@katre @copybara-github
Re-arrange PlatformInfo and clean up some code.
d8d3593 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 565770775
Change-Id: Ibb75e66f6a73aad565ade28e5cda99ebaa280121
copybara-service bot pushed a commit that referenced this issue Sep 20, 2023
@katre @copybara-github
Make the platform rule non-configurable.
87fb462 
Also remove the unneeded PlatformBaseRule.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 567075741
Change-Id: I5ba99d3b52d17b5786de49561a497b5802001c00
copybara-service bot pushed a commit that referenced this issue Sep 25, 2023
@katre @copybara-github
Remove the unused PlatformInfo starlark constructor.
e5fe228 
Also remove the exec property fields from the Starlark API.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 568265920
Change-Id: I5a4146f93167901455c2f4d3856505840aeb64f1
copybara-service bot pushed a commit that referenced this issue Sep 27, 2023
@katre @copybara-github
Extract execProperty handling to a dedicated class.
612bf5c 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 568853694
Change-Id: I468d33b9eb0138e35274f7fd188c82ac851fbef4
copybara-service bot pushed a commit that referenced this issue Sep 29, 2023
@katre @copybara-github
Clean up uses of an "empty" BuildOptions.
0626570 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 569576336
Change-Id: I7c7f37c833987aed83b04aaddac234ef3b17de2e
copybara-service bot pushed a commit that referenced this issue Sep 29, 2023
@katre @copybara-github
Update PlatformInfoProducer to not use a configuration key.
d6254dc 
All platforms are non-configured (since 87fb462), so we can use an empty configuration and de-duplicate requests.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 569577077
Change-Id: Iea70d2c1484df17bc12925317415fc32cbd90ccc
copybara-service bot pushed a commit that referenced this issue Oct 3, 2023
@katre @copybara-github
Update platform mapping to work on BuildOptions.
4647633 
Remove BuildConfigurationKey from platform mapping calculations. This enables locking down access to BCK contents from other locations.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 570430837
Change-Id: I5645a6782015455ebf3513cbac60fa78b0696e57
copybara-service bot pushed a commit that referenced this issue Oct 4, 2023
@katre @copybara-github
Make PlatformMappingApplier its own class and rename it.
c24b847 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 570779049
Change-Id: I142c44db575bd3f4b77d369285a9e97f57227271
copybara-service bot pushed a commit that referenced this issue Oct 4, 2023
@katre @copybara-github
Centralize SkyframeExecutor's creation of BuildConfigurationKey insta…
a30de30 
…nces.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 570816210
Change-Id: I2cd28c20e05cf1fd2401632bb856e9cf93955a7e
copybara-service bot pushed a commit that referenced this issue Oct 6, 2023
@katre @copybara-github
Use BuildConfigurationKeyProducer from BaselineOptionsFunction.
ba436cc 
This further centralizes BuildConfigurationKey creation.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 571362276
Change-Id: I09736443ec18cab51feddc153d3a5a7c49761a09
copybara-service bot pushed a commit that referenced this issue Oct 6, 2023
@katre @copybara-github
Inline and remove BuildConfigurationKey.withPlatformMapping.
860612c 
Next will be to rename `withoutPlatformMapping`.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 571363118
Change-Id: I170d83c8cc6550f33abeaa6b11cb58447104b134
copybara-service bot pushed a commit that referenced this issue Oct 6, 2023
@katre @copybara-github
Rename BuildConfigurationKey.withoutPlatformMapping.
8d6d2ec 
The name `create` is much clearer and doesn't expose internal implementation details.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 571376423
Change-Id: I4f8c29d22a0e16dd331749c07dba37685caf3382
@blorente blorente mentioned this issue Oct 10, 2023
Closed
copybara-service bot pushed a commit that referenced this issue Nov 7, 2023
@katre @copybara-github
Simplify cloning BuildOptions.
6a08b2f 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 580317682
Change-Id: Ia84894632c8344b983f73e41070a1a24ac43bc2b
copybara-service bot pushed a commit that referenced this issue Nov 8, 2023
@katre @copybara-github
Streamline code to cache configurations.
926b2c4 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 580452068
Change-Id: If94e0fbd9a815bfe3decca21ef1fd58003b4c366
copybara-service bot pushed a commit that referenced this issue Nov 8, 2023
@katre @copybara-github
Move OptionsDiff to a separate class.
ff0c35c 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 580499529
Change-Id: Icbcd46b25643ce7271bbec9eb1d4b0fedc2acef7
copybara-service bot pushed a commit that referenced this issue Dec 4, 2023
@katre @copybara-github
Clean up Starlark transition processing.
9b16f62 
The flags being read by a transition are by definition a subset of all flags, and typically are a much smaller subset, so let's process the smaller set instead of every flag in the configuration.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 587681561
Change-Id: I8b329ef3e9447db08a0cedb9bb1b5ef60fe6458f
copybara-service bot pushed a commit that referenced this issue Jan 9, 2024
@katre @copybara-github
Allow some native options to be marked as IMMUTABLE.
576e61a 
This means they cannot be changed during a transition.

Currently only handles starlark transitions. The exec transition is specifically exempted, because it is a core part of configuration handling.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 596917609
Change-Id: Icae25fd907dd1a16e2c83cefe1a14a162438dc99
copybara-service bot pushed a commit that referenced this issue Jan 9, 2024
@katre @copybara-github
Make the --platform_mappings flag immutable.
1d4ab4d 
If it cannot be changed in transitions, all errors will be caught in the first usage (in `SkyframeExecutor.createBuildConfigurationKey`), and so no further errors need to be considered.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 596918285
Change-Id: I68a1808d836716893491fd212eb2b5bc9352f541
copybara-service bot pushed a commit that referenced this issue Jan 9, 2024
@katre @copybara-github
Remove handling for multiple platform mappings in BuildConfigurationK…
c55011e 
…eyProducer.

This situation cannot happen.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 596958087
Change-Id: I8870973bc0f940375ff9829cb3da30dd5c12c2dc
copybara-service bot pushed a commit that referenced this issue Jan 10, 2024
@katre @copybara-github
Remove SkyframeExecutor.getConfigurationForTesting.
4603a3a 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 597221978
Change-Id: I9740eea9d91b384548b45260fa9c69b2a8302197
copybara-service bot pushed a commit that referenced this issue Jan 16, 2024
@katre @copybara-github
Add better exception support to PlatformMappingFunction.
4a6f896 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 598953443
Change-Id: I55cd314ee644934032a39d7d23dd9a742fe2c0f7
copybara-service bot pushed a commit that referenced this issue Jan 16, 2024
@katre @copybara-github
Handle platform mapping errors in BuildConfigurationKeyProducer.
10082f3 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 598954569
Change-Id: I29056b09ebdb898fee4a1ec1621fb01b8d8d8f09
copybara-service bot pushed a commit that referenced this issue Jan 19, 2024
@katre @copybara-github
Add new skyfunction to create build configuration keys.
4ddc4a1 
This essentially exposes BuildConfigurationKeyProducer as a SkyFunction.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 599838440
Change-Id: Id6c0b00f43fe813f77dbed8c7a6fee0f989649a6
copybara-service bot pushed a commit that referenced this issue Jan 19, 2024
@katre @copybara-github
Use BuildConfigurationKeyFunction to create BuildConfigurationKey ins…
c411fa9 
…tances.

This removes duplication of logic.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 599862509
Change-Id: Ib0c17a90ad9fd475d758dff3be68994202bb8d98
copybara-service bot pushed a commit that referenced this issue Jan 19, 2024
@katre @copybara-github
Use BuildConfigurationKeyFunction to create BuildConfigurationKey ins…
8897473 
…tances.

This removes duplication of logic.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 599870463
Change-Id: I86f08697ed264b9de939bceb7a544d1c3d5fe0da
copybara-service bot pushed a commit that referenced this issue Apr 4, 2024
@katre @copybara-github
Relax test for ProtoOutputFormatterCallbackTest.
8550cb8 
In some cases platofrm based flags cause extra targets to be present.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 621930695
Change-Id: I5b72bb07e8385e2fc1c4c478504385ababca3415
copybara-service bot pushed a commit that referenced this issue Apr 4, 2024
@katre @copybara-github
Correct dependencies to ignore platforms in tests.
6a397db 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 621936486
Change-Id: Ib5bf2fed705a1c1bd68de3e08d38a8cdf9be9986
copybara-service bot pushed a commit that referenced this issue Apr 9, 2024
@katre @copybara-github
Update shouldDeleteOnAnalysisInvalidatingChange to avoid invalidati…
fb37774 
…ng the special empty configuration and related actions.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 623208595
Change-Id: I2af23afe893599e5ac7a338e5e09680495d1faa2
copybara-service bot pushed a commit that referenced this issue Apr 11, 2024
@katre @copybara-github
Change PlatformFlagsProducer to also include the requested platform l…
b4f9cfd 
…abel in the results.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 623820511
Change-Id: Iaa7bd29654667c8c1ec12828ac7f54043915ed04
copybara-service bot pushed a commit that referenced this issue May 2, 2024
@katre @copybara-github
More platform mapping test cleanups.
36b812b 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 630030455
Change-Id: I91933097a31ad5fe1ff5955e58261bde357362d0
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Update tests of coverage_common.instrumented_file_info to not need …
c117ae5 
…to override builtins.

Overriding builtins is flaky with upcoming work when the configuration changes.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621472041
Change-Id: Ib61f14dee822bc0e1fc24eba24865c9d5b3dc0b9
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Show duplicates more easily in failing skyframe tests.
2985649 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621476576
Change-Id: Ic4b88c051b79d13f0ac7fc12be605836fe22411d
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Correctly order creating the platforms and using them in tests.
39c016d 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621555402
Change-Id: Id3c70a34844f0ec4ba0ca690827d228acc3493b0
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
When overriding the platform repo, define the host platform.
9c60278 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621567588
Change-Id: I4a833e1458d7c4f6f78ce2069b3104acfddd211f
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Use a new package for the new platform in tests.
36dc249 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621568874
Change-Id: Id880b5fbf3085ee2dbef966aba8ccda9b442c75d
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Increase the number of shards for toolchain_test.
d618972 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621865160
Change-Id: Id9ef8a342dcdf9dde44b367a8c74088dde7ae9b6
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Relax test for ProtoOutputFormatterCallbackTest.
506a7bc 
In some cases platofrm based flags cause extra targets to be present.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621930695
Change-Id: I5b72bb07e8385e2fc1c4c478504385ababca3415
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Correct dependencies to ignore platforms in tests.
35d1db9 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 621936486
Change-Id: Ib5bf2fed705a1c1bd68de3e08d38a8cdf9be9986
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Update shouldDeleteOnAnalysisInvalidatingChange to avoid invalidati…
f4f26b7 
…ng the special empty configuration and related actions.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 623208595
Change-Id: I2af23afe893599e5ac7a338e5e09680495d1faa2
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
Change PlatformFlagsProducer to also include the requested platform l…
df385bf 
…abel in the results.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 623820511
Change-Id: Iaa7bd29654667c8c1ec12828ac7f54043915ed04
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@katre @Kila2
More platform mapping test cleanups.
4a63e85 
Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 630030455
Change-Id: I91933097a31ad5fe1ff5955e58261bde357362d0
@katre katre mentioned this issue May 14, 2024
Open
katre added a commit to katre/bazel that referenced this issue May 15, 2024
@katre @copybara-github
Add tests for more types of flag merging.
14688f8 
Flag merging is used in platform mappings, and the in progress platform-based flags.

Issues:
- Implicit options are droppped
- Repeating native and Starlark flags overwrite previous values instead of appending.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 634039493
Change-Id: I65006ed00bef94e7f944afc447315c6e9504b50b
@katre katre mentioned this issue May 15, 2024
Draft
@katre
Copy link
Member Author

katre commented May 15, 2024

Current status: I have a working implementation, except that there's an issue with repeatable flags (ie, --copt, --feature, any Starlark flag that uses config.string_list(repeatable = True)).

The issue (which also exists in platform mappings) is that setting a repeatable flag in the platform.flags attribute (or in a platform->flag mapping) will completely overwrite the previous values, instead of appending.

I have some example tests that show the error: #22386 (see errors: https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/21626)

The current goal is to try and detect and warn when this happens (for native flags: not doable for Starlark flags), submit platform based flags, then fix the merging (which requires fixing the Starlark options parser, thus #22365).

copybara-service bot pushed a commit that referenced this issue May 20, 2024
@katre @copybara-github
Have ConfigurationTestCase use the same set of precomputed values as …
5cafcc4 
…AnalysisMock.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 635457036
Change-Id: I53d1429ca580ef7f0c933243e4b4f953d3c9394a
@katre katre mentioned this issue May 20, 2024
Open
copybara-service bot pushed a commit that referenced this issue May 20, 2024
@katre @copybara-github
Test overriding the platform module by first copying the existing ver…
077bb40 
…sion.

Otherwise, anything that depends on `@platform//cpu` will fail.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 635502289
Change-Id: I2431a7bc849d2d0b0c6b4573de6a4838ffe9478e
copybara-service bot pushed a commit that referenced this issue May 31, 2024
@katre @copybara-github
Refactor build options merging to better handle all options.
219f118 
Also properly handles implicit options.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 639053307
Change-Id: Iceb376d81fdc874358ad0d5b88923803b229d784
copybara-service bot pushed a commit that referenced this issue Jun 5, 2024
@katre @copybara-github
Allow BzlLoadFunction to fully reset the cache.
25a87d9 
Some tests (specifically those that use `usesInliningBzlLoadFunction`) mutate the builtins, and so need to be able to reset the cache, making the comment at [`BzlLoadFunction$InlineCacheManager.reset`](https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java;drc=2c0faf93deae61bfc0bb1c42becbca4c49c988e6;l=1551) invalid.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 640607616
Change-Id: I3c30b2888c99b46084ea2519870dc6d61c5a2a79
copybara-service bot pushed a commit that referenced this issue Jun 6, 2024
@katre @copybara-github
Ensure that packages are invalidated any time package or build langua…
08ef9b2 
…ge options are changed.

This prevents issues where incomplete skyframe nodes are still cached when
tests change state.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 641037026
Change-Id: Ibb2ba4b1413b516d2f2214244d3f536fb5583ca7
copybara-service bot pushed a commit that referenced this issue Jun 7, 2024
@katre @copybara-github
When creating a BuildConfigurationKey, check the target platform for …
ac05b29 
…flags.

Fixes platform-based flags: #19409.

PiperOrigin-RevId: 641217049
Change-Id: I0a873eb708f8919a5bd5d62ca35dba5e51b80888
copybara-service bot pushed a commit that referenced this issue Jun 7, 2024
@katre @copybara-github
Add further documentation about platform-based flags.
04a7685 
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 641224216
Change-Id: I84e28a526c06c098dfeafb049373e1a8b5a3e067
@katre
Copy link
Member Author

katre commented Jun 7, 2024

The main functionality is now in (and documented). The only work remaining is to clean up how we handle merging repeated flags, but that's tracked in #22453, so I am closing this as complete.

@katre katre closed this as completed Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@katre katre

Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@katre