Allow string_list flags to be set via repeated flag uses #14911

fmeum · 2022-02-25T08:15:17Z

For all parts of Bazel other than option parsing, string build setting
flags with allow_multiple = True should behave just like string_list
settings, even though they are fundamentally of a different type. This
requires a lot of special casing all over the code base and also causes
confusing user-facing behavior when transitioning on such settings.

This commit deprecates the allow_multiple named parameter of
config.string and as a replacement introduces a new named parameter
repeatable on config.string_list. Settings with the latter set to True
behave exactly like string settings with allow_multiple = True with
respect to command-line option parsing and exactly like ordinary
string_list flags in all other situations.

Fixes #13817

gregestren · 2022-03-04T22:04:06Z

I like what you're doing here but this is complicated enough (from an API perspective) where I'd like to see a proper design doc. Would you be up for that? I'm not asking anything complex: just a small doc that clearly states the issue and why this approach solves it effectively. And ensures everyone's collaboratively considering all angles.

For example, there are important nuances (which you acknowledge in the PR) I'd like to be upfront clear: a repeated string_list with --//foo=a,b is different than a singleton string_list with --//foo=a,b.

I'm asking this because build_setting is basically the "new" API for builtin Bazel flags. The idea is we can eventually replace all builtin flags that users - not core devs - should control. While that goal alone is great, there's also an opportunity to avoid the complicated typing API that powers native flags. Those grew over a long time, somewhat ad hoc in response to isolated use cases, and is now unfortunately complex.

I'd like to avoid the same outcome with build_setting. We're already running into nuanced complications and API tweaks. So I really want to make sure API changes are cautiously vetted with long-term, generic sustainability in mind. Keep the APIs as simple as possible while still powerful (in particular push as much complication as we can into Starlark, which is built for that). And try to minimize long-term API churn, migrations, and deprecations.

This isn't a knock at your proposal (which is super-promising!). It's general caution I have for any API changes in this area. But I agree there are real API issues to address, and the status quo is insufficient.

CC @brandjon @comius who are also thinking about general API curation.

gregestren · 2022-03-04T22:05:45Z

I'm also curious how or if this applies to other types. Like a theoretical int_list, or any other novel data type. Would this API generalize that way? Should it? (there's a good argument that it shouldn't). Would love to hear some design brainstorming about that too.

fmeum · 2022-03-06T16:43:09Z

I will try to get a short doc started. I'm now leaning towards using a more descriptive argument, e.g. something like flag_style = "repeated" vs. flag_style = "comma", and a doc would be a great way to get feedback on this.

I'm also curious how or if this applies to other types. Like a theoretical int_list, or any other novel data type. Would this API generalize that way? Should it? (there's a good argument that it shouldn't). Would love to hear some design brainstorming about that too.

It would not require any additional code apart from adding the parameter to intSetting. Given that there isn't even an int_list setting yet, I'm not sure it would make sense to add at this point though.

fmeum · 2022-03-07T09:46:40Z

@gregestren I submitted bazelbuild/proposals#248.

gregestren · 2022-03-07T23:09:37Z

To be clear, I'm hesitant to support int_list. Once we're combining arbitrary data types I figure it's cleaner for the API to use string_list and have implementers parse that into whatever they need in Starlark.

hlopko · 2022-06-15T07:31:19Z

(this is a super-useful feature and we from rules_rust are looking forward to using it :)

gregestren · 2022-06-15T18:50:16Z

Sorry we're not faster on this, @hlopko and everyone else. Still on our radar. @fmeum and I also have an open communication channel so we'll get this properly worked out at some point.

gregestren · 2022-06-23T23:14:09Z

FYI @fmeum , @aranguyen and I have a sync next week to try to move this forward.

fmeum · 2022-06-30T17:21:56Z

@aranguyen I updated the PR to reflect the update to the proposal I submitted in bazelbuild/proposals#266

aranguyen · 2022-07-13T01:11:48Z

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java

-                    + " implementation function will be a list of strings. Insertion order and"
-                    + " repeated values are both maintained. This list can be post-processed in the"
-                    + " build setting implementation function if different behavior is desired.",
+                "Deprecated, use a <code>string_list</code> setting with"


Does this actually do anything beside doc? I was thinking it would be nice to emit a WARNING from now on if allowMultiple is being used. Thoughts?

Outside Google, such a warning probably wouldn't work that well: The ones seeing the warning (ruleset users on a new Bazel version) would not be the ones in a position to fix it (ruleset authors). In the past, I think that this has been handled via an --incompatible_* flag, which is flipped at some point with a full downstream pipeline running against the popular rulesets before the flip.

aranguyen · 2022-07-13T01:17:21Z

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java

@@ -3025,6 +3025,51 @@ public void testBuildSettingValue_allowMultipleSetting() throws Exception {
        .containsExactly("some-other-value", "some-other-other-value");
  }

+  @SuppressWarnings("unchecked")
+  @Test


all of these test cases are great. Thank you for adding them. Could we also add test case for this example

--copt=a,b,c --> ["a,b,c"] --copt=a --copt=b,c --> ["a", "b,c"]

Great point, added.

For all parts of Bazel other than option parsing, string build setting flags with `allow_multiple = True` should behave just like `string_list` settings, even though they are fundamentally of a different type. This requires a lot of special casing all over the code base and also causes confusing user-facing behavior when transitioning on such settings. This commit deprecates the `allow_multiple` named parameter of `config.string` and as a replacement introduces a new `repeatable` parameter on `config.string_list`. Settings with the latter set to True behave exactly like `string` settings with `allow_multiple = True` with respect to command-line option parsing and exactly like ordinary `string_list` flags in all other situations.

aranguyen

This looks good to me. I'm trying out a new internal review process. If all goes well, this should be merged soon. Thanks for your patience.

aranguyen · 2022-07-15T12:25:22Z

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java

@@ -3025,6 +3025,51 @@ public void testBuildSettingValue_allowMultipleSetting() throws Exception {
        .containsExactly("some-other-value", "some-other-other-value");
  }

+  @SuppressWarnings("unchecked")
+  @Test


fmeum · 2022-07-20T17:29:19Z

@bazel-io flag

fmeum · 2022-07-20T17:52:09Z

@gregestren Do you think we should add an incompatible flag disabling the old allow_multiple parameter?

ShreeM01 · 2022-07-20T18:07:42Z

@bazel-io fork 5.3.0

For all parts of Bazel other than option parsing, string build setting flags with `allow_multiple = True` should behave just like `string_list` settings, even though they are fundamentally of a different type. This requires a lot of special casing all over the code base and also causes confusing user-facing behavior when transitioning on such settings. This commit deprecates the `allow_multiple` named parameter of `config.string` and as a replacement introduces a new named parameter `repeatable` on `config.string_list`. Settings with the latter set to True behave exactly like `string` settings with `allow_multiple = True` with respect to command-line option parsing and exactly like ordinary `string_list` flags in all other situations. Fixes bazelbuild#13817 Closes bazelbuild#14911. PiperOrigin-RevId: 462146752 Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56

fmeum · 2022-07-21T08:22:39Z

@kshyanashree The bazel-io fork command doesn't seem to have worked.

sgowroji · 2022-07-21T08:23:36Z

@bazel-io fork 5.3.0

For all parts of Bazel other than option parsing, string build setting flags with `allow_multiple = True` should behave just like `string_list` settings, even though they are fundamentally of a different type. This requires a lot of special casing all over the code base and also causes confusing user-facing behavior when transitioning on such settings. This commit deprecates the `allow_multiple` named parameter of `config.string` and as a replacement introduces a new named parameter `repeatable` on `config.string_list`. Settings with the latter set to True behave exactly like `string` settings with `allow_multiple = True` with respect to command-line option parsing and exactly like ordinary `string_list` flags in all other situations. Fixes bazelbuild#13817 Closes bazelbuild#14911. PiperOrigin-RevId: 462146752 Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56

For all parts of Bazel other than option parsing, string build setting flags with `allow_multiple = True` should behave just like `string_list` settings, even though they are fundamentally of a different type. This requires a lot of special casing all over the code base and also causes confusing user-facing behavior when transitioning on such settings. This commit deprecates the `allow_multiple` named parameter of `config.string` and as a replacement introduces a new named parameter `repeatable` on `config.string_list`. Settings with the latter set to True behave exactly like `string` settings with `allow_multiple = True` with respect to command-line option parsing and exactly like ordinary `string_list` flags in all other situations. Fixes #13817 Closes #14911. PiperOrigin-RevId: 462146752 Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56 Co-authored-by: Fabian Meumertzheim <[email protected]>

fmeum mentioned this pull request Feb 25, 2022

config.string with allow_multiple=True should use list for build_setting_default #13817

Closed

aiuto requested a review from gregestren February 26, 2022 01:50

gregestren mentioned this pull request Mar 4, 2022

Allow literal default for allow_multiple build setting in transitions #14908

Closed

gregestren self-assigned this Mar 4, 2022

gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 4, 2022

fmeum force-pushed the 13817-allow-multiple-list-default branch from 70a4290 to e33a5e4 Compare March 6, 2022 16:35

fmeum force-pushed the 13817-allow-multiple-list-default branch from e33a5e4 to 70a4290 Compare March 6, 2022 22:17

fmeum mentioned this pull request Mar 7, 2022

Add "Representing repeatable Starlark flags as config.string_list" bazelbuild/proposals#248

Merged

sgowroji added the awaiting-user-response Awaiting a response from the author label May 5, 2022

fmeum mentioned this pull request Jun 10, 2022

Pass singleton list to transition for allow_multiple setting default #15655

Closed

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jun 13, 2022

mypmc mentioned this pull request Jun 14, 2022

Allow multiple uses of extra_rustc_flags bazelbuild/rules_rust#1407

Closed

fmeum force-pushed the 13817-allow-multiple-list-default branch from 70a4290 to 72a44a3 Compare June 30, 2022 17:19

fmeum force-pushed the 13817-allow-multiple-list-default branch 2 times, most recently from 90b0660 to e04ea41 Compare June 30, 2022 18:13

aranguyen self-assigned this Jul 8, 2022

aranguyen self-requested a review July 8, 2022 19:30

aranguyen reviewed Jul 13, 2022

View reviewed changes

fmeum force-pushed the 13817-allow-multiple-list-default branch from e04ea41 to 3b4b3a8 Compare July 13, 2022 11:08

aranguyen approved these changes Jul 15, 2022

View reviewed changes

aranguyen 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 Jul 15, 2022

aranguyen removed the request for review from gregestren July 15, 2022 12:59

copybara-service bot closed this in cdf01a3 Jul 20, 2022

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 20, 2022

fmeum deleted the 13817-allow-multiple-list-default branch July 20, 2022 17:29

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 20, 2022

bazel-io mentioned this pull request Jul 21, 2022

[5.3.0] Allow string_list flags to be set via repeated flag uses #15937

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 21, 2022

ckolli5 mentioned this pull request Jul 21, 2022

Allow string_list flags to be set via repeated flag uses #15943

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow string_list flags to be set via repeated flag uses #14911

Allow string_list flags to be set via repeated flag uses #14911

fmeum commented Feb 25, 2022 •

edited

Loading

gregestren commented Mar 4, 2022 •

edited

Loading

gregestren commented Mar 4, 2022 •

edited

Loading

fmeum commented Mar 6, 2022 •

edited

Loading

fmeum commented Mar 7, 2022

gregestren commented Mar 7, 2022

hlopko commented Jun 15, 2022

gregestren commented Jun 15, 2022

gregestren commented Jun 23, 2022

fmeum commented Jun 30, 2022

aranguyen Jul 13, 2022

fmeum Jul 13, 2022

aranguyen Jul 13, 2022

fmeum Jul 13, 2022

aranguyen Jul 15, 2022

aranguyen left a comment

aranguyen Jul 15, 2022

fmeum commented Jul 20, 2022

fmeum commented Jul 20, 2022

ShreeM01 commented Jul 20, 2022

fmeum commented Jul 21, 2022

sgowroji commented Jul 21, 2022

Allow string_list flags to be set via repeated flag uses #14911

Allow string_list flags to be set via repeated flag uses #14911

Conversation

fmeum commented Feb 25, 2022 • edited Loading

gregestren commented Mar 4, 2022 • edited Loading

gregestren commented Mar 4, 2022 • edited Loading

fmeum commented Mar 6, 2022 • edited Loading

fmeum commented Mar 7, 2022

gregestren commented Mar 7, 2022

hlopko commented Jun 15, 2022

gregestren commented Jun 15, 2022

gregestren commented Jun 23, 2022

fmeum commented Jun 30, 2022

aranguyen Jul 13, 2022

Choose a reason for hiding this comment

fmeum Jul 13, 2022

Choose a reason for hiding this comment

aranguyen Jul 13, 2022

Choose a reason for hiding this comment

fmeum Jul 13, 2022

Choose a reason for hiding this comment

aranguyen Jul 15, 2022

Choose a reason for hiding this comment

aranguyen left a comment

Choose a reason for hiding this comment

aranguyen Jul 15, 2022

Choose a reason for hiding this comment

fmeum commented Jul 20, 2022

fmeum commented Jul 20, 2022

ShreeM01 commented Jul 20, 2022

fmeum commented Jul 21, 2022

sgowroji commented Jul 21, 2022

fmeum commented Feb 25, 2022 •

edited

Loading

gregestren commented Mar 4, 2022 •

edited

Loading

gregestren commented Mar 4, 2022 •

edited

Loading

fmeum commented Mar 6, 2022 •

edited

Loading