Feature request: allow nested selects.

[ahyangyi]

Currently, if one writes a helper function that wraps a select(), he must clearly document where he used that select. For example:

def foo(x, src1, src2):
    native.cc_library(name = x, srcs = select("//tools/toolchain:arm": src1, "//tools/toolchain:x86": src2))

where one can use foo("bar", ["bar_arm.cc"], ["bar_x86.cc"]), but cannot use

foo("bar", select(
     "//tools/toolchain:arm_android": ["bar_arm_and.cc"],
     "//tools/toolchain:arm_linux": ["bar_arm_lin.cc"],
     ), 
     ["bar_x86.cc"]
)

, since select() cannot nest.

Thus, in this feature request, I propose allowing nested selects. Since selects work on user inputs, in almost all practical situations, even nested selects would be pretty simple and in small scale. Allowing selects to nest would only provide flexibility to the user, and would not cause more slowdown than implemented otherwise.

Therefore, I believe allowing nested selects does not contradict with any existing Bazel philosophy, and could provide additional productivity.

[hermione521]

@lberki following from last issue

[lberki]

I don't think there is a philosophical objection against this, it just needs to be implemented.

[jia-kai]

A workaround in some cases is to add a new target.

Assume you want:

cc_library(
    name='foo',
    defines=if_a(if_b(['-DAB=1']))
)

Then a virtual target foo_a can be added to provide the defines:

cc_library(
    name='foo_a',
    defines=if_b(['-DAB=1'])
)
cc_library(
    name='foo',
    deps=if_a(['foo_a']),
)

[gregestren]

Right, this is similar to AND chaining of selects (select({config_setting1 && config_setting2: value}). The intermediate library is the best current workaround. You can also define another Skylark macro that automatically creates that intermediate library. These approaches don't always work well for non-dep-oriented attributes (like genrule cmd).

The only philosophical caution I would mention about nested selects is the possibility of freezes or OOMs under certain obscure circumstances. Long story short is that certain build phases - most notably Bazel query - don't know which select paths will get chosen. So sometimes they say "give me every possible value". Nested selects can make that answer exponential. Most code paths optimize this problem away. But there are still certain code paths that can trigger it, and it has caused freezing bugs before.

As for implementation challenge, I think a simple version of this wouldn't be too hard. The biggest concern is that select values are designed to match the type of the original attribute (label list, string list, etc). Breaking that assumption is much harder than keeping it. So

# Two selects of different types:
some_string_list_attr = select({fooCond: ["foo" + select({barCond: "bar"})]})

would be a lot harder than

# Two selects of the same type:
some_string_list_attr = select({fooCond: ["foo"] + select({barCond: ["bar"]})]})

[gregestren]

So I proof-of-concepted nested selects: check out https://bazel-review.googlesource.com/c/bazel/+/44230/1#message-c2bbb6b8973380ea2eb65483e2364f600c07f295

Short story is the basic change is surprisingly easy. But there are a bunch of corner cases that substantially complicate it. So I put up this change as a starting point for further discussion and consensus.

If anyone really wants nested selects, this is a good time to speak up.

[chpatton013]

If anyone really wants nested selects, this is a good time to speak up.

Present and accounted for!
I'm not sure how many other people have use-cases for this, but I know I sure do.

[gregestren]

Christopher,

Thanks so much for your input. We had some good conversation on the review thread linked from two comments ago, so I think there's some comfort about the concern of this making build files less readable. And it looks like your post is pretty thumbs up popular.

Would you mind giving me more detail on what you'd like to do with this feature?

[chpatton013]

Would you mind giving me more detail on what you'd like to do with this feature?

Sure thing!

We are primarily a Linux dev house. We've got some third party C++ libraries that make heavy use of #ifdef to include specific features, change memory layout, etc. We've described these features as config_settings, which we are able to use in select blocks to change the set of flags that are used in various targets. This is great because it means we don't have to modify the third-party libraries' source, which makes updating to new releases much easier.

Now we would like to support Windows as a native development platform. We're using config_settings to describe the Linux and Windows platforms, and can use them in select statements to control compiler-specific copts and the like. Unfortunately, there are a few situations where the flags we need to provide to these third-party libraries differ based on your development platform.

If we had nested select support, we would be able to describe these compound conditions appropriately. However, without that feature we have to use less-savory workarounds.

[hlopko]

@gunan could you chime in with your requirements? I remember TF asking for this a year ago, and also recently.

[gunan]

For TF, in our BUILD rules we manage a lot of configurations that are orthogonal to each other, such as CPU architecture, operating system, GPU, additional library support.
For these, we currently need to create many config settings such as this:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/BUILD#L247

Having nested selects will greatly improve readability and managability of our BUILD configs.

[gregestren]

While not quite the same as the prototype described in #1623 (comment), I just added a draft proposal to support AND and OR chaining for config_setting: Config Setting Chaining. Hopefully that can help at least some of these uses cases?

Please do check it out and let me know where it helps and where it falls short.

The main advantage of that vs. the nested select prototype is all the code is already written for the chaining. :) Proper nested select support has been blocked on the simple fact that it's a more concerted development effort. I'd still love to finish that out to production quality but it's hard for me to provide an ETA for that. Nevertheless, this remains on my radar. If anyone else has spare cycles earlier and wanted to make it happen I'd be happy to advise / review as well.

[emusand]

If anyone really wants nested selects, this is a good time to speak up.

We need support for nested selects.

Our products are written in C and run on our proprietary hardware, which is very resource constrained. To get good build and test performance, we want to build minimal unit test binaries that only contain a production .c file, a test .c file, and mocks of all used functions. We only want to use public .h files and defines from cc_library dependencies, to avoid compiling all source files in the dependencies and link them into the test binary.

We use a library macro that wraps cc_library. Our idea was to add a select(...) in this library macro, that sets srcs = [] when building unit tests. But if the srcs parameter already contains a select(...), they will be nested.

[gregestren]

We use a library macro that wraps cc_library. Our idea was to add a select(...) in this library macro, that sets srcs = [] when building unit tests. But if the srcs parameter already contains a select(...), they will be nested.

If all the macro is doing is emptying out srcs, does it matter whether the original srcs has a select? Can't the macro just ignore the original srcs?

[emusand]

If all the macro is doing is emptying out srcs, does it matter whether the original srcs has a select? Can't the macro just ignore the original srcs?

The same macro is used both in production and unit test builds. In production builds, srcs should be used. In unit test builds, srcs should be an empty list. We set a --define in unit test builds, to distinguish between them.

However, there might be other ways to solve our use case.

[emusand]

We have investigated alternatives to other use case, but it looks like we need support for nested select(...) to solve it.

Here is a summary of our use case, if my previous posts were hard to follow: We use a macro around cc_library which adds a select(...) for parameter srcs, but srcs might already contain a select(...).

[gregestren]

@emusand what about this?

def emptyable_cc_library(**kwargs):
    name = kwargs["name"]

    args_with_srcs = dict(kwargs)
    args_with_srcs["name"] = name + "_with_srcs"

    args_without_srcs = dict(kwargs)
    args_without_srcs["name"] = name + "_without_srcs"
    args_without_srcs["srcs"] = []

    native.alias(
	name = name,
	actual = select({
	   ":foo": ":" + args_with_srcs["name"],
	   ":bar": ":" + args_without_srcs["name"]
	   }))
    native.cc_library(**args_with_srcs)
    native.cc_library(**args_without_srcs)

I tried this in a sample repro and it worked fine (a bazel query --output=build //mypkg:all helps see what ultimately gets produced).

Sorry to keep pushing back. In spite of my earlier enthusiasm I'm less convinced there'll be appetite to natively support nested selects. The reasons being

1. it adds some real complexity to Starlark's structure (which I think the Starlark devs would push back against)
2. I think we can get reasonable workarounds for most cases with skylib's selects library (which provides boolean chaining), stuff like I laid out in this example, and another pending FR to let macros see inside a select's dict.

I think that empowers most cases. While it's true that's more complex than native direct support, we can abstract this away with predefined libraries and best practice patterns (like in Skylib).

If you think of this stuff as features on a product that feels like unnecessary complexity. But if you think of it as a programming language (which it is), this is the standard way to extend and support new patterns, for a system that's already powerful enough to model new functionality with its existing feature set. So it becomes a challenge of designing clear, extensible patterns vs. trying to stuff all the complexity directly into the core.

[emusand]

@gregestren thank you for the idea, and thank you for pushing back :) We also found another solution to our use case. So my previous posts were immature - we can avoid using nested select(...) for this use case.

[gregestren]

For all the reasons discussed here, I'm going to close this. While technically possible, from a design and UI perspective we need a very high bar for considering direct support of nested selects.

We have enough alternative approaches now that heavily mitigate the problem. I think future bugs should carefully demonstrate the deficiencies of other approaches and consider what enhancements to them are possible before coming back to this theme.

[jlebar]

Hi. :)

We have enough alternative approaches now that heavily mitigate the problem.

Are there others other than #1623 (comment)?

This solution seems fine, except that bazel build :all will build both cc_libraries, and one might not even compile (that may be the purpose of the select). Is there a way to disable one of them?

(My thought was to empty the "bad" one's srcs, but it doesn't work if srcs might have any selects inside it. Turtles all the way down.)

[gregestren]

Current recommended practice is to try using https://github.com/bazelbuild/bazel-skylib/blob/master/docs/selects_doc.md.

If you need both cc_libraries to exist, then I agree $ bazel build :all can be problematic.

One option could be #3780, which @philsc is diligently working on in a PR (and might be a presentation at this year's BazelCon). This will fulfill a long-needed ability to skip some targets for :all-style builds.

Another option could be to write the libraries in different packages so $bazel build :all only triggers one of them.

Why doesn't emptying the srcs work in your case?

[jlebar]

I'll keep an eye on #3780; thank you for the link.

Why doesn't emptying the srcs work in your case?

If srcs itself contains a select, then doing select(condition, srcs, []) to empty the srcs would use a nested select. :)