"tags" attribute should be usable with "select"

[philwo]

Reported by @gorset in #2669 (comment):

I tried falling back to exclusive using something like:
tags = select({":darwin": ["exclusive"], "//conditions:default": ["block-network"]}),
but this is unfortunately not supported:
attribute "tags" is not configurable.

Do we want the "tags" attribute to be configurable via "select"? If not, why not? (Would be good to document this decision here.)

Ping @ahumesky who replied on related #1904 earlier.

[ahumesky]

I don't know much past what I posted on #1904, perhaps @gregestren can comment?

[gregestren]

"tags" is nonconfigurable for practical reasons, not principled ones. In short, multiple places in Bazel's codebase expect to know the value of "tags" before the build's configuration is known. This means they can't get their answer from a select. We can conceivably refactor that code away and make tags selectable. But that would be a mini-project for whoever wants to take up the effort.

I'm glad to review anyone's proposed cleanup, but I'm not prioritizing this work myself.

Examples:

bazel/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

Line 161 in 1575652

if (tag.startsWith("block-")

bazel/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

Line 148 in 1575652

return NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)

bazel/src/main/java/com/google/devtools/build/lib/packages/Package.java

Line 1301 in b13457b

if (TargetUtils.isTestRule(rule) && !TargetUtils.hasManualTag(rule)) {

I'd expect the cleanup to be relatively straightforward but not entirely trivial.

[gregestren]

As mentioned on #7911 (comment), the challenge to make this viable is to figure out how to make

bazel/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java

Lines 113 to 114 in 1f684e1

	List<String> tagsAttribute =
	NonconfigurableAttributeMapper.of(testSuite).get("tags", Type.STRING_LIST);

work without a NonconfigurableAttributeMapper.

This code unconditionally iterates over all values of tags. So we can't exempt "known safe" values like exclusive and make then select-friendly unless we know how to either exempt them here too or lift the general NonconfigurableAttributeMapper limitation.

[gregestren]

Closing for the below reasons:

This can't be practically prioritized without a focused effort by a community contributor. This requires vetting how much logical complexity has to be considered due to the dependencies described in previous comments and evaluating the feature's benefit vs. the new complexity's maintenance cost.

It's also possible (maybe even preferable?) to explore alternative approaches that reduce the need to use tags in the first place, vs. other better-fitting attributes.

So basically, it's a design effort. And one that needs to evaluate the specific needs for this flag vs. alternatives, which varies from case to case. It's still a good design question but let's only keep this open when/if someone wants to own that.

[tpudlik]

On Pigweed we ran into the exact same use case: we want to use "block-network", but that only works on Linux, and there's no way to tell Bazel to only enable "block-network" when running on Linux. As far as I can tell this means you can't benefit from network sandboxing at all (even while building on Linux) if you want your project to build on both Linux and MacOS. That's a bummer: it's a great feature!

It's also possible (maybe even preferable?) to explore alternative approaches that reduce the need to use tags in the first place, vs. other better-fitting attributes.

I agree with this sentiment. But it seems like there's a more fundamental problem here than tags being the wrong attribute. Bazel expects to extract some execution info about tests (linked by you above, in #2971 (comment)) before the target configuration is known. The real issue is whether extracting this information can be deferred until after the target configuration is determined. Moving the information from tags to another attribute will not help, since then that attribute would not be selectable. (Maybe addressing this is the "mini-project" you referred to in your first comment.)

[gregestren]

On quick glance I didn't see anything obvious that required execution info that early (most consumes of getExecutionInfo look like they're in analysis? all?) Maybe we can defer those classes of tags?

The only other concern I'd have is if CI expects to know the execution info tags. Google's CI cares about knowing execution machine requirements with nothing more informed than a bazel query. We could always check with them on this point.

[philwo]

we want to use "block-network", but that only works on Linux

I implemented support for block-network in the macOS sandbox more than 6 years ago - does it no longer work?

See:

bazel/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

Lines 303 to 308 in e393673

	if (!allowNetwork) {
	out.println("(deny network*)");
	out.println("(allow network-inbound (local ip \"localhost:*\"))");
	out.println("(allow network* (remote ip \"localhost:*\"))");
	out.println("(allow network* (remote unix-socket))");
	}

[tpudlik]

It does not appear to work for us: see https://issues.pigweed.dev/issues/312452379. (Should have linked to it in the original comment, sorry!)

[pcjanzen]

One useful-but-slightly-gross hack:
Write a repository rule that detects the host OS (or reads some environment variable) and then writes a .bzl function that returns either ["block-network"] or [], and then call that function from your BUILD file.