CC'ing @serynth who reviewed the proposal.

Given that this function covers functionality in Bazel that I'm not intimately familiar with and @c-parsons has already discussed parts of the design with you (and because he's the owner of Skylib now), I prefer to defer to him on the review.

and because he's the owner of Skylib now

Oh, my bad. Hello new owner!

@c-parsons - given the state of bazelbuild/bazel#6237, do you think I can start integrating this with that now?

If by "integrating this with that" you mean: testing your Pull Request using the analysis-testing framework proposed in that issue?

If so, yes! Please do!
There are analysis tests using this framework which exist in the repository now today. See https://github.com/bazelbuild/bazel-skylib/blob/master/tests/unittest_tests.bzl for some examples (in truth, they are testing the analysis-test framework itself, but these are good canonical examples to get you started)

Is there something preventing this to be merged ? Thanks!

See the previous two comments. @c-parsons was writing some Starlark testing infrastructure that this change needed to get proper testing. That wasn't ready at first but now it is. So that's what I need to integrate.

I want to tackle this in O(weeks) but it's not something I'll practically be able to have done tomorrow.

No worries, I'm running off the patch from the PR. Thanks for the clarification!

Update: I'm revisiting this PR.

@c-parsons - I added a bunch of tests. It was fun using the new analysis-aware infrastructure. I would have liked to hide each test behind a single call, but that doesn't seem possible. It would also be nice to have test suite support.

I have two commented out tests which require checking that a macro fail()s. Is there any way to test that?

Otherwise, PTAL.

@gregestren FYI, i've had some issues when ANDing with config_setting that had constraint_values where bazel would fail. It seems it had to do with the alias.

I seems it has to do with AND'ing on a platform and a config_setting:

    native.config_setting(
        name = "something",
        define_values = {
            "something": "1",
        },
        visibility = ["//visibility:public"],
    )
    selects.config_setting_group(
        name = "mymegarule",
        match_all = [
            "@io_bazel_rules_go//go/toolchain:android_arm64",
            ":something",
        ],
    )

It fails with:

//:mymegarule is not a valid configuration key for //XXXX

Oddly enough, if I invert the match_all, then it works

    selects.config_setting_group(
        name = "mymegarule",
        match_all = [
            ":something",
            "@io_bazel_rules_go//go/toolchain:android_arm64",
        ],
    )

All tests on bazel 0.25.3, for the record

Oddly enough, if I invert the match_all, then it works

    selects.config_setting_group(
        name = "mymegarule",
        match_all = [
            ":something",
            "@io_bazel_rules_go//go/toolchain:android_arm64",
        ],
    )

So it's a TODO to be able to put constraint_value directly in select() (or by extension in config_setting_group). See bazelbuild/bazel#4167 (comment). So that's a valid error: all entries in config_setting_group have to be config_setting.

You can fix that in the interim by putting your constraint_value in a config_setting:

config_setting(
    name = "android_arm64",
    constraint_values = ["@io_bazel_rules_go//go/toolchain:android_arm64"]
)

As for inverting the order, an AND fails fast if the first condition already fails. :)

What are you looking for in test suite support? It looks like you somewhat do this by having all targets created under
Is it that you'd like to be able to test specifically your whole "suite" given a single target?
If so, I'll give it some thought :) Thanks for the feedback!

Yes, exactly. I just see a huge explosion of the number of tests at the top level so I imagine at some point of scale that could be a problem. No major issue as things stand now though.