Platform mappings: support setting Starlark flags #18288

gregestren · 2023-05-03T01:07:14Z

In other words, support:

platforms:
  //:my_platform
    --//mypkg:my_custom_flag=value

Non-goals:

Refactor the relationship between the Starlark and native option parsers
Support Starlark flag -> platform mappings (we could add this if needed but platform mappings isn't a feature we want to expand more than necessary)
Parse with simplified ad hoc logic instead of directly using StarlarkOptionsParser as the source of truth

Approach:

The basic problem (described here) is StarlarkOptionsParser needs to convert flag names to Targets. PlatformMappingValue calls the parsers but can't Skyframe-load Targets.
So we add support in PlatformMappingFunction to Skyframe-load the needed Targets
But StarlarkOptionParser already has code for doing that, outside of a Skyfunction environment, through SkyframeExecutor.loadTargetPatternsWithoutFilters.
So factor out StarlarkOptionParsers Target loading into an interface the caller can implement
The original callers (BlazeOptionHandler and CanonicalizeCommand) provide the same implementation as before
PlatformMappingFunction provides a Skyframe retriever through PackageFunction (which is a cleaner way to get Targets within a Skyfunction)

I also had to factor out StarlarkOptionsParser.java into its own java_library to prevent dependency cycles between :runtime and Skyframe: :runtime already depends on Skyframe, and by calling StarlarkOptionsParser from a Skyfunction that adds a dependency the other way.

Thanks @katre for initial investigation.

Fixes #18199.

PiperOrigin-RevId: 528888600
Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2

@katre

In other words, support: ``` platforms: //:my_platform --//mypkg:my_custom_flag=value ``` ### Non-goals: - Refactor the relationship between the Starlark and native option parsers - Support Starlark flag -> platform mappings (we could add this if needed but platform mappings isn't a feature we want to expand more than necessary) - Parse with simplified ad hoc logic instead of directly using `StarlarkOptionsParser` as the source of truth ### Approach: 1. The basic problem (described [here](bazelbuild#18199 (comment))) is `StarlarkOptionsParser` needs to convert flag names to `Target`s, `PlatformMappingValue` parses the flags but can't Skyframe-load `Target`s. 1. So we add support in `PlatformMappingFunction` to Skyframe-load the needed `Target`s 1. But `StarlarkOptionParser` already has code for doing that, outside of a `Skyfunction` environment, through `skyframeExecutor.loadTargetPatternsWithoutFilters`. 1. So factor out `StarlarkOptionParser`s `Target` loading into an interface the the caller can implement 1. The original callers (`BlazeOptionHandler` and `CanonicalizeCommand`) provide the same implementation as before 1. `PlatformMappingFunction provides a Skyframe retriever through `PackageFunction` (which is a cleaner way to get `Target`s within a `Skyfunction`) I also had to factor out `StarlarkOptionsParser` into its own `java_library` to prevent dependency cycles between `:runtime` and Skyframe: `:runtime` already depends on Skyframe, and by calling `StarlarkOptionsParser` from a Skyfunction that adds a dependency the other way. Thanks @katre for initial investigation. Fixes bazelbuild#18199. PiperOrigin-RevId: 528888600 Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2

gregestren · 2023-05-03T01:08:28Z

https://github.com/Colecf @jin

gregestren · 2023-05-03T01:09:37Z

I only included a basic test in this first commit. Happy to add more (e.g. bad label checking).

gregestren · 2023-05-03T01:11:10Z

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

+   * <p>Hence this class. By dropping those events, we restrict all info and error reporting logic
+   * to the options parsing pipeline.
+   */
+  private static class NonPostingEventHandler implements ExtendedEventHandler {


Note this is just ported out of StarlarkOptionsParser.

gregestren · 2023-05-03T01:11:29Z

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

+    @Override
+    public Target loadBuildSetting(String targetLabel)
+        throws InterruptedException, TargetParsingException {
+      TargetPatternPhaseValue result =


This implementation code is ported out of StarlarkOptionsParser.

jin · 2023-05-03T05:44:12Z

Thanks @gregestren! Do you mind adding a more comprehensive test that changes the platform in a transition, and verifies that the associated build flag with the platform also changes value?

katre

Overall LGTM.

I would have preferred this split into a few PRs:

Move StarlarkOptionsParser to a new target and introduce the BuildSettingsLoader interface (and existing usage).
This would have introduced no new behavior, making testing simpler.
Add new usage of StarlarkOptions in platform mapping.
New behavior, so new tests.

But this is written so it's fine as-is.

katre · 2023-05-03T13:14:25Z

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

+   *
+   * <p>This is used for top-level flag parsing, outside any {@link SkyFunction}.
+   */
+  public static class SkyframeExecutorTargetLoader


Do we need a new inner class? Can the BlazeOptionHandler implement BuildSettingLoader directly, or just define loadBuildSetting in BlazeOptionHandler and pass in a method reference?

CanonicalizeCommand also references this, which is why I made it its own entity:

bazel/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java

Line 201 in f2ccd3c

new SkyframeExecutorTargetLoader(env), parser);

gregestren · 2023-05-03T23:08:50Z

Overall LGTM.

I would have preferred this split into a few PRs:

I was thinking something similar but wanted to at least start with the full picture to show how it all fits together.

This was on the verge of being complicated enough to merit a design doc. But IMO not quite that complicated (since it's not user-facing aside from one very specific difference) and we really don't want to publicize platform mapping upgrades any more than necessary.

gregestren · 2023-05-03T23:17:05Z

Thanks @gregestren! Do you mind adding a more comprehensive test that changes the platform in a transition, and verifies that the associated build flag with the platform also changes value?

On it...

Also, I'm not sure how this handles labels outside the main repo. I did my best to use canonical label references but all that logic is so subtle and you never know.

This is complicated enough where I'd like to not worry about that for this PR. If you think you'd need that let me know and let's explore as a followup patch (or even better if no patch is needed at all).

gregestren · 2023-05-03T23:18:03Z

@brentleyjones out of curiosity, what's your interest in this change?

brentleyjones · 2023-05-03T23:20:43Z

Not out if need (that I know of).

I'm just a fan of Starlark build settings/flags, so I like changes like this that make them work in more places.

PiperOrigin-RevId: 528888600 Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2

gregestren · 2023-05-04T01:03:58Z

Thanks @gregestren! Do you mind adding a more comprehensive test that changes the platform in a transition, and verifies that the associated build flag with the platform also changes value?

Added tests.

jin

Thank you for the test case! @Colecf FYI

I'll leave the review of the impl details to John.

katre · 2023-05-04T13:20:15Z

LGTM for me.

Colecf · 2023-05-05T17:25:43Z

Thanks Greg! I'll try and pull this into android and test it out.

@katre

In other words, support: ``` platforms: //:my_platform --//mypkg:my_custom_flag=value ``` #### Non-goals: - Refactor the relationship between the Starlark and native option parsers - Support Starlark flag -> platform mappings (we could add this if needed but platform mappings isn't a feature we want to expand more than necessary) - Parse with simplified ad hoc logic instead of directly using `StarlarkOptionsParser` as the source of truth #### Approach: 1. The basic problem (described [here](bazelbuild#18199 (comment))) is `StarlarkOptionsParser` needs to convert flag names to `Target`s. `PlatformMappingValue` calls the parsers but can't Skyframe-load `Target`s. 1. So we add support in `PlatformMappingFunction` to Skyframe-load the needed `Target`s 1. But `StarlarkOptionParser` already has code for doing that, outside of a `Skyfunction` environment, through `SkyframeExecutor.loadTargetPatternsWithoutFilters`. 1. So factor out `StarlarkOptionParser`s `Target` loading into an interface the caller can implement 1. The original callers (`BlazeOptionHandler` and `CanonicalizeCommand`) provide the same implementation as before 1. `PlatformMappingFunction` provides a Skyframe retriever through `PackageFunction` (which is a cleaner way to get `Target`s within a `Skyfunction`) I also had to factor out `StarlarkOptionsParser.java` into its own `java_library` to prevent dependency cycles between `:runtime` and Skyframe: `:runtime` already depends on Skyframe, and by calling `StarlarkOptionsParser` from a Skyfunction that adds a dependency the other way. Thanks @katre for initial investigation. Fixes bazelbuild#18199. PiperOrigin-RevId: 528888600 Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2 Closes bazelbuild#18288. Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2 PiperOrigin-RevId: 529479790

gregestren requested a review from katre May 3, 2023 01:07

gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label May 3, 2023

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels May 3, 2023

gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label May 3, 2023

gregestren commented May 3, 2023

View reviewed changes

katre approved these changes May 3, 2023

View reviewed changes

Add tests.

153f233

PiperOrigin-RevId: 528888600 Change-Id: Ie3597f4a1d406c6918fadbdcdb208d23db6a2aa2

gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 4, 2023

jin approved these changes May 4, 2023

View reviewed changes

copybara-service bot closed this in 8a7f5e2 May 4, 2023

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform mappings: support setting Starlark flags #18288

Platform mappings: support setting Starlark flags #18288

gregestren commented May 3, 2023

gregestren commented May 3, 2023

gregestren commented May 3, 2023

gregestren May 3, 2023

gregestren May 3, 2023

jin commented May 3, 2023

katre left a comment

katre May 3, 2023

gregestren May 3, 2023

gregestren commented May 3, 2023 •

edited

Loading

gregestren commented May 3, 2023

gregestren commented May 3, 2023

brentleyjones commented May 3, 2023

gregestren commented May 4, 2023

jin left a comment

katre commented May 4, 2023

Colecf commented May 5, 2023

Platform mappings: support setting Starlark flags #18288

Platform mappings: support setting Starlark flags #18288

Conversation

gregestren commented May 3, 2023

Non-goals:

Approach:

gregestren commented May 3, 2023

gregestren commented May 3, 2023

gregestren May 3, 2023

Choose a reason for hiding this comment

gregestren May 3, 2023

Choose a reason for hiding this comment

jin commented May 3, 2023

katre left a comment

Choose a reason for hiding this comment

katre May 3, 2023

Choose a reason for hiding this comment

gregestren May 3, 2023

Choose a reason for hiding this comment

gregestren commented May 3, 2023 • edited Loading

gregestren commented May 3, 2023

gregestren commented May 3, 2023

brentleyjones commented May 3, 2023

gregestren commented May 4, 2023

jin left a comment

Choose a reason for hiding this comment

katre commented May 4, 2023

Colecf commented May 5, 2023

gregestren commented May 3, 2023 •

edited

Loading