fix: expand_location binary target inputs

[f0rmiga]

The previous behaviour was that location expansion on native rules would accept binary targets as inputs and resolve to a single location. This was not an option on custom rules. This patch aligns the behaviour of Starlark's ctx.expand_location to the native rules. E.g. a py_binary can be expanded using location/execpath/rootpath instead of previously only locations/execpaths/rootpaths.

For more context, see bazelbuild/rules_python#846. To be able to add the multi-toolchain feature to rules_python, I had to trick ctx.expand_location using the tools attribute while this is not fixed.

[sgowroji]

Hello @f0rmiga, Can you please fix the failed buildkite checks. Thanks!

[f0rmiga]

That's a funny failure since the same test passed on all other platforms, including other centos. Any chance it flaked?

[rickeylev]

For context: formiga came across this difference in behavior when trying to put a py_binary in the args of (i think it was) an sh_test. A py_binary has two default outputs (the executable and the main .py file) (why I don't entirely know; undesirable and historical).

After a big of digging, we were found the difference is how StarlarkRuleContext and native rules create their LocationExpander.

StarlarkRuleContext (through StarlarkRuleContext.expandLocation) creates an expander that doesn't look at the data attribute and passes in a label map with the default outputs of the targets list (StarlarkRuleContext.makeLabelMap). The net effect is when an e.g. py_binary is specified, the map has {"//foo:bar": [executable, other]} in it.

In comparison, a native rule usually creates the location expanded through RunfilesSupport (RunfilesSupport.computeActionEnvironment). It creates an expander that does look at the data attribute and passes in a empty/null label map. LocationExpander has hard-coded logic to look at the deps, data, and tools attributes and only add targets from them that have FilesToRunProvider; these are then added to the label map it builds before processing. The net effect is the map has {"//foo:bar": [executable]}

HTH

[rickeylev]

@sgowroji I've removed the team-Rules-Python tag -- this isn't Python specific and this code isn't part of the Python implementation. I'm not sure offhand who the domain owner is for this

[rickeylev]

@comius it looks like you've been reviewing changes to StarlarkRuleContext and expand_location recently? I think this PR is ready for review. If the behavior change here isn't acceptable, then see #16382 for some other ideas/discussion about how we might let Starlark code better act here.

In the mean time, I've imported this internally and started a TGP.

[rickeylev]

@rickeylev (tagging myself to make this findable)

[rickeylev]

The global regression tests finally finished, but it's unclear if there was an infrastructure failure, this broke many things, or just broke a handful of very low-level dependencies. I'm having a look.

[rickeylev]

Ok, I think i figured out what's going on.

The basic code in question is:

thing = ctx.attr.thing
print(thing.files_to_run.executable) # <generated file bla/thing_executable>
print(thing.files) # [<generated file bla/thing>]
arg = ctx.expand("$(location {})".format(thing.label))
ctx.actions.run(
  inputs = ctx.files.thing # expands to bla/thing
  args = [arg]
)

Without this PR, the expansion is bla/thing. With this PR, it expands to bla/thing_executable

As a concrete example, this seems to happen when the wasm rules apply a transition to a cc_test. The cc_test puts the original test executable in the default outputs, but then the wasm transition somehow causes it to put a wasm-specific executable as the test's executable, but it doesn't show up in default outputs. I think wasm changing the name is WAI; I see some code that seems to indicate the cc toolchain (wasm in this case) can affect the output name.

So the general case is when a rule doesn't put its own executable in the default outputs. That seems very strange -- why would one do that on purpose? This seems like a bug in the cc_test rules. Actually, wait, I thought the executable was always added? Aha, yes, Executable rules docs say:

That executable is added to the default outputs of the rule (so you don't need to pass that to both executable and files)

So this definitely sounds like a bug somewhere.

[rickeylev]

Quick update: I've found the owner of the code that fails and they're going to have a look. At a glance, it looks more like a bug in their code. After they assess, we'll figure out next steps (maybe it's an easy fix, or medium and need to wait, or hard and adding a flag somewhere might be needed, etc).

[trybka]

I've fixed the particular internal rule that was failing, and the related tests pass with this import.

[f0rmiga]

Thank you @trybka.