experimental_remap_main_repo and register_execution_platforms issue

[ittaiz]

Description of the problem / feature request:

build breaks when using --experimental_remap_main_repo=true and register_execution_platforms refers to current workspace in fully qualified manner.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

https://github.com/ittaiz/bazel_remap_platforms_issue

Error:

➜  bazel_remap_platforms_issue git:(master) bazel build //...                 
INFO: Invocation ID: b8f78378-5b97-42bc-a982-ae9aa9c23824
ERROR: While resolving toolchains for target //code:foo: Unable to find an execution platform for target platform @bazel_tools//platforms:target_platform from available execution platforms []
ERROR: Analysis of target '//code:foo' failed; build aborted: Unable to find an execution platform for target platform @bazel_tools//platforms:target_platform from available execution platforms []
INFO: Elapsed time: 0.188s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded, 3 targets configured)

You can also check the remap_off_passes and no_qualify_in_ws_passes branches which show the build passes when remap is off or when you don't qualify the execution platform in the WORKSPACE.

What operating system are you running Bazel on?

OS X

What's the output of bazel info release?

0.23.1
I also tried to verify that 2398d85 doesn't solve the issue. It wasn't easy since it was rollbacked, I can't seem to build bazel HEAD locally and I had a really hard time finding a bazel nightly release.
I ended up downloading the mac binary from this build https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/862#36abbed3-6d50-4137-a779-4a6c989bc557 as I think it should contain the above change.
In any case the build still fails there.
Also happens with 0.24.0rc7

Have you found anything relevant by searching the web?

#7654 sounds related, which is why I tried the above commit, but we have a bit of a different error.

Also I'll share that in our internal workspace we did see ERROR: While resolving toolchains for target //some-target: no matching toolchains found for types @io_bazel_rules_scala//scala:toolchain_type but when I worked on the repro and worked on a minimized example I actually got the error from above.
Also the two workarounds above also workaround it for our internal cases.

[ittaiz]

@iirina any update?

[katre]

@dkelmer and I were just discussing this (and #7654) and realized the problem and how to fix it.

Here is a brief explanation of how the code works currently:

1. The register_toolchains and register_execution_platforms functions merely save their arguments, as strings, into the Package.Builder instance for //external.
2. RegisteredToolchainsFunction and RegisteredExecutionPlatformsFunction then call into TargetPatternUtil.expandTargetPatterns to get a list of Labels.
3. Those Labels are then used directly.

This was fine before repository remapping existed. Now, however, we need to move the target pattern expansion into the workspace functions and out of the skyfunctions.

So, to fix these issues:

1. Change Package.registeredToolchains and Package.registeredExecutionPlatform (and the associated Builder methods) to take Label instead of String
2. Add target pattern expansion into register_toolchains and register_execution_platforms (including error handling). This needs to use the current repository map, based on the currently loaded repository.
3. Remove target pattern expansion from RegisteredToolchainsFunction and RegisteredExecutionPlatformsFunction.

Of these, the hard part is adding target pattern expansion to the workspace functions, since that requires access to TargetPatternUtil or something similar, which needs a SkyFunction.Environment to be available. Note that the Environment in the BuiltinFunction.invoke signature is a syntax.Environment, not a SkyFunction.Environment.

[ittaiz]

Thanks @katre and @dkelmer!
Priority and complexity wise is there a chance this will get in the 0.25 rc next week? (I assume chances are slim but am keeping my hopes up).

[dkelmer]

Priority wise: definitely. Complexity wise: I'm doing my best

[ittaiz]

@dkelmer will this make 0.26rc?

[ittaiz]

@dkelmer any update? We're nearing three months on it. I have a workaround but I'm not sure when it will break

[katre]

Unfortunately @dkelmer is no longer working on Bazel. I'm going to try and figure out who can take over this work.

[katre]

Re-assigning to @dslomov to find a new owner.

[laurentlb]

Klaus, can you take a look at what needs to be done here (and how much work it is)?

[aehlig]

Hi @katre,

@dkelmer and I were just discussing this (and #7654) and realized the problem and how to fix it.

Here is a brief explanation of how the code works currently: [...]
This was fine before repository remapping existed.

thanks for the detailed description of how things work at the moment. However, I have a question on your conclusion.

Now, however, we need to move the target pattern expansion into the workspace functions and out of the skyfunctions.

Why is it necessary, to move the pattern expansion out of the skyfunctions? Wouldn't it be an alternative to perform the repository mapping (with the current map based on the currently loaded repository) at the level of target patterns? As far as I can see, pattern expansion is pretty orthogonal to repository remapping, as we do not do any form of expansion in the repository part of a pattern.

In this way, we would also get rid of the problem...

Of these, the hard part is adding target pattern expansion to the workspace functions, since that requires access to TargetPatternUtil or something similar, which needs a SkyFunction.Environment to be available. Note that the Environment in the BuiltinFunction.invoke signature is a syntax.Environment, not a SkyFunction.Environment.

...of getting a full skyframe access (through a SkyFunction.Environment) with the side effect of reading BUILD files as part of the WORKSPACE semantics, disallowing the currently possible forward-referencing toolchain declarations, etc.

Am I missing something, or is this a way forward worth investigating further?

[katre]

Wouldn't it be an alternative to perform the repository mapping (with the current map based on the currently loaded repository) at the level of target patterns? As far as I can see, pattern expansion is pretty orthogonal to repository remapping, as we do not do any form of expansion in the repository part of a pattern.

Hmm, I think this would work. You're entirely correct that target patterns can't change the repository, only the "local" package and targets selected.

I believe @Dannark was working on something similar: she had several changes which added the concept of repo mapping to target pattern evaluation (771cb7a, which was rolled back in dac2135). I don't think this was ever finished and rolled forward again, but may prove a basis for starting.

[ittaiz]

@aehlig can you explain what will be the implications of disallowing the currently possible forward-referencing toolchain declarations, etc.? I'm not sure I fully understand

[aehlig]

@aehlig can you explain what will be the implications of disallowing the currently possible forward-referencing toolchain declarations, etc.? I'm not sure I fully understand

Well, my proposal is to not disallow it. What it's all about is the following. Currently, you can have a WORKSPACE file like the following.

register_execution_platforms("@foo//platforms/...")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
  name = "foo",
  ...
)

This works, because we do not need to expand the pattern in the registration of an execution platform at time of registration. And with the proposal to do the renaming of repositories at the level of pattern strings this continues to work; however, if we followed the proposal of immediately expanding and keeping the labels instead, the example would stop working.

[aehlig]

My proposed fix for that issue is the following sequence of commits.

• https://bazel-review.googlesource.com/c/bazel/+/105150
• https://bazel-review.googlesource.com/c/bazel/+/105151
• https://bazel-review.googlesource.com/c/bazel/+/105152

@katre @ittaiz can you have a look if this is a reasonable implementation or whether I missed something? Thanks.

[ittaiz]

I skimmed it but I don’t feel I have enough understanding of that area to weigh in.

[katre]

This looks fine, I'll save detailed review comments for the CLs as they arrive.

…

On Fri, Jul 5, 2019 at 11:15 AM Ittai Zeidman ***@***.***> wrote: I skimmed it but I don’t feel I have enough understanding of that area to weigh in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7773?email_source=notifications&email_token=AACPW7ZIU3UVIWIPNS4N4Z3P55QSHA5CNFSM4G724TV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZJYPHQ#issuecomment-508790686>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACPW73FXKYV6CLBLMPL2CDP55QSHANCNFSM4G724TVQ> .

[aehlig]

Reopening, as the change was rolled back. It seems, the renaming is missing in the toolchain_type attribute of the toolchain rule (which is in BUILD files, so in a later phase).

[aehlig]

Unfortunately, the commit was rolled back again, due to bazelbuild/bazel-skylib#173 which I, unfortunately, cannot reproduce locally.