Design review for Platform-based Flags #18672
Replies: 19 comments 40 replies
-
I like the general direction of this, moving flags into This brings up a few more questions about flag precedence: If a transition changes both |
Beta Was this translation helpful? Give feedback.
-
cc @Colecf This is great! |
Beta Was this translation helpful? Give feedback.
-
First of all, I think this is a great suggestion. In general, I have been thinking about something like
in hope to get more structure than So here are my questions:
|
Beta Was this translation helpful? Give feedback.
-
Could you please be a bit more specific on how |
Beta Was this translation helpful? Give feedback.
-
Have we considered restricting this to just build settings instead of arbitrary flags? I still think it would be better to move to a world where transitioning on flags is really deprecated. |
Beta Was this translation helpful? Give feedback.
-
Accumulating flags are an interesting problem. Let's say we have a rule that transitions to platform A, and then transitively depends on something that transitions to A again. Does the no-op change of platform do nothing? Now, lets say A has --copt = [-X]. What I don't want is for the second transition to A to result in --copt = [-X,. -X]. That would potentially be a different configuration that the user didn't intend. |
Beta Was this translation helpful? Give feedback.
-
Accumulating flags are an interesting problem even without no-op transitions. Let's say we have It seems like in order to make this work sensibly, we'd have to maintain a special bucket of platform-added flags, and unwind the entire old platform flag bucket each time the platform changes. (Naively I would think that the platform flag bucket would override the command line options, but I haven't thought too deeply about it). |
Beta Was this translation helpful? Give feedback.
-
Acknowledging reviewer request. My main feedback is the same as in the other proposal: I really want to sit down and explore more concretely how this fits with the other new concepts we're exploring: where they overlap, where they could conflict, what they do uniquely, how they'd fit most elegantly. I need some more time to get momentum on that (such as sharing my named config / flag set proposal here) so we can broadly contextualize the conversation. I think the right thing is that I whip up that proposal and share and get that thread going. Then we write a holistic "how everything fits together" doc that references all these proposals. That doc would rationalize the past, present, and future of the combination of: Each is a puzzle piece and we want to be as absolutely clear as possible as what the completed puzzle looks like. As a bonus we might get a refreshed long-term vision doc for free. :) I think this is also healthy for the community to see, since it's really hard to see the forest through the trees when we're looking at individual focused proposals. |
Beta Was this translation helpful? Give feedback.
-
After talking with @gregestren and looking at my calendar, I'm going to pause the discussion on this proposal: @gregestren has a related proposal to finish up and I will be out of the country for most of July. I'd like to pick this back up when I'm back in the last week of July. |
Beta Was this translation helpful? Give feedback.
-
I have some concerns, especially from the implementation complexity side where we will likely have to start making finer distinctions on when things happen in options parsing and handling compared to before. I think it would be reasonable to first consider what happens during the initial options parse and ignore the potential impact of transitions. Notably, only here do we have to consider blazerc and the actual commandline arguments. Will platforms-based flags have higher or lower priority than blazerc settings? (I think regardless we will have to add a new entry to OptionPriority.PriorityCategory to get this to work cleanly and allow us to more seamlessly tune the ordering behavior.) If choosing the behavior where the order of setting Will the flags attribute of platforms be non-configurable? (It essentially has to be in order for the feature to be sanely usable as part of the initial options parse but I did not see a mention in the proposal.) Can platforms itself be set with platform-based flags? (This sounds silly but isn't inconceivable as users may try to use platforms+platform-based-flags to define a transition and config-in-one.) Now, considering transitions (but no longer blazerc or commandline arguments): Will the platform-based flags only be applied once, when platforms is transitioned? (I presume they are not applied 'continuously', like with platform_mappings as then they would effectively always have highest priority.) What if there are multiple platforms defined in platforms? (This is really just to push for platforms becoming single-valued) What if the transitions sets other flags, besides just platforms? (set platform from transition -> apply platform-based-flags -> apply all other flags from transition -> apply platform_mappings) What if the transition setting the flag is the exec transition? (Wherein it is conceivable that you want the platform-based flags to take precedence) What if the transition explicitly sets the platform to the same value? (Should platform-based flags be re-applied even though, technically, the platform change was seemingly a no-op.) What if the transition clears the platform and thus it is platform_mappings setting the platform? (We may need to double apply here so that the platform is determined correctly: apply all flags from transition -> determine platform via platform_mappings -> apply platform-based-flags -> apply all non-platform flags from transition -> apply platform_mappings) |
Beta Was this translation helpful? Give feedback.
-
The proposal now has a Semantics section based on @sdtwigg's suggestions. Thanks all! |
Beta Was this translation helpful? Give feedback.
-
Thoughts: Syntax:I don't fully understand the syntax concerns. The proposal describes: flags = [
"--custom_malloc=//label",
"--other_flag",
"--//example/starlark:flag=23",
], This is clearly intuitive. It matches what the user's used to at the command line. But we have two other BUILD constructs that also itemize flags: def _transition_impl(settings, attr):
_ignore = (settings, attr)
return {
"//command_line_option:cpu": "arm",
"//some:starlark_flag": "foo",
} config_setting(
name = "flag_blob",
values = {
"//command_line_option:cpu": "arm",
"//some:starlark_flag": "foo",
}) Never mind the bazelrc syntax. Is this further forking the syntax, if the current norm people are used to in BUILD files is Re: Rejected Alternative: Starlark Dict type, the transition logic works with mixed build types. But I think that's not translatable from pure .bzl code to a rule attribute like you're doing here. So I think that objection stands. Re: Parsing already exists, doesn't The main reason I'm bringing this up is the principle of consistent experience. If the reasons are good enough to diverge here that's fair. But let's be super-conscientious so users down the road don't complain "Why does Bazel do the same thing in two different ways?" Other:
I didn't catch how the double application fits? I saw #18672 (comment) but didn't fully follow.
|
Beta Was this translation helpful? Give feedback.
-
From https://bazel.build/run/bazelrc#config:
That means for
we'd have to first scan to find the |
Beta Was this translation helpful? Give feedback.
-
Oh, I didn't mean to advocate for forced label syntax for native flag names. Happy to drop that as not in consideration here.
You're right, I got the example wrong. Maybe we should consider the reverse. Move config_setting(
name = "flag_blob",
values = [
"--cpu=arm",
"--//some:starlark_flag=foo"
]
)
Ah yes, just as you stated. I'd really love to not need so many attributes on Anyway. my main point is for us to stay conscientious of consistent syntax. It makes sense to properly focus on that when/if we get to #18672 (comment). So no more in scope for this conversation.
Got it. For reference the below comment (#18672 (comment)) clarifies the prioritization logic.
Makes sense, thanks.
What happens today if a transition sets Isn't the current semantics that the platform mapping exclusively applies after the transition? Which already raises the possibility of conflicts? This all sounds pretty hypothetical: the exact mix of transitions, flags, platforms-with-flags, and platform mappings meant for some effect. If the above is right, why don't we just keep it simple: transition first, then platform-map, then we're done. That would still require steps 1-4 but not 5. Slightly simpler implementation and IMO simpler API contract. i don't think we know for a fact yet that fails actual user expectations? And if it did we could always adjust? We could go even farther and decide to not yet support transitions that clear |
Beta Was this translation helpful? Give feedback.
-
LGTM frome me |
Beta Was this translation helpful? Give feedback.
-
Thanks for the comments, everyone! Now that this is accepted, I am locking the discussion to keep it archived, but feel free to reach out or file further feature requests if needed. |
Beta Was this translation helpful? Give feedback.
-
Update on platform-based flagsI have an implementation mostly ready to start submitting. Unfortunately, the original planned semantics aren't feasible to implement. What we wantedIdeally, platform-based flags would have fixed a number of problems with platform mappings, the biggest being that platform mappings are effectively highest priority, it's not possible to override the flags from the mapping to a different value. The original ideal for platform-based flags was that they would only be applied when the target platform changes, thus allowing flags to be overridden, and that we'd augment the flag parsing so that the values could be more flexible. What is possibleUnfortunately, those semantics aren't possible at this time without a major overhaul of Bazel's flag parsing library (specifically, we'd need to merge the currently split functionality in What we can getInstead, the current version of platform-based flags that I plan to submit will have the same semantics as platform mappings: the flags from the platform will be applied to every configuration, and will not be able to be overridden by other flags (either in transitions or at the command line). This is unfortunate, but will still allow us to use platform-based flags in a reduced manner. Hopefully at some point we can come back and add the missing functionality to the option parsing and improve this. Please feel free to comment or message me to discuss this further if you have questions. |
Beta Was this translation helpful? Give feedback.
-
Thanks for the update.
What happens if you transition to a platform that specifies different values for the same flags? Or will it not be possible at all to transition to a platform that specifies flag values? (That would be truly unfortunate, as it's the major way we envisioned using them!) |
Beta Was this translation helpful? Give feedback.
-
Correct, they'll use the same system as execution property inheritance. The only nuance is that there's no easy way to say "reset this flag to the default, whatever that was": if a parent platform sets a flag, the child platform can either accept that, or set it to a different value, but can't just remove it. |
Beta Was this translation helpful? Give feedback.
-
Another design from Platform Features API, this is for platform--based flags, which aims to obsolete half of the platform mappings file, and unlock new capabilities, by adding a new
flags
attribute to the existingplatform
rule, listing flags and values to enable when the platform is the target platform.See the full design and let me know what you think. Specifically, I'm curious about what is the least confusing behavior when the same flag is present in the platform and on the command line, and whether that changes if the platform is also set on the command line, or via a transition.
Beta Was this translation helpful? Give feedback.
All reactions