Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toolchain resolution fails for non-workspace prefixed toolchain_types if workspace name used --extra_toolchains #13381

Open
shahms opened this issue Apr 20, 2021 · 11 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug

Comments

@shahms
Copy link

shahms commented Apr 20, 2021

Description of the problem / feature request:

When specifying a toolchain via --extra_toolchains which includes a workspace name, toolchain resolution fails to include that toolchain if it toolchain doesn't include a workspace prefix in the toolchain_type attribute.

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

Given a trivial build file and rule implementation (in dummy_toolchain.bzl):

def _impl(ctx):
    return [platform_common.ToolchainInfo()]

dummy_toolchain = rule(implementation = _impl)

def _rule(ctx):
    print(ctx.label)

dummy_rule = rule(
    implementation = _rule,
   # qualification here doesn't appear to matter
    toolchains = ["//:dummy_toolchain_type"],
)

And corresponding BUILD file:

load(":dummy_toolchain.bzl", "dummy_rule", "dummy_toolchain")
package(default_visibility=["//visibility:public"])

dummy_toolchain(name = "dumdumdum")

toolchain_type(
    name = "dummy_toolchain_type",
)

toolchain(
    name = "dummy_toolchain_relative",
    toolchain = ":dumdumdum",
    toolchain_type = ":dummy_toolchain_type",
)

toolchain(
    name = "dummy_toolchain_absolute",
    toolchain = ":dumdumdum",
    toolchain_type = "//:dummy_toolchain_type",
)

toolchain(
    name = "dummy_toolchain_prefix",
    toolchain = ":dumdumdum",
    toolchain_type = "@//:dummy_toolchain_type",
)

toolchain(
    name = "dummy_toolchain_full_prefix",
    toolchain = ":dumdumdum",
    toolchain_type = "@//:dummy_toolchain_type",
)
dummy_rule(name = "dummy")

If the workspace name is included when specifying a toolchain via --extra_toolchains, only toolchains which include a workspace prefix will work. For example, given the above:

for workspace in "@" "@io_kythe" ""; do
  for toolchain in "relative" "absolute" "prefix" "full_prefix"; do
    bazel build   --extra_toolchains="$workspace//:dummy_toolchain_$toolchain"  //:dummy
  done
done

Gives the results:

--extra_toolchains prefix toolchain_type result
@ relative succeeds
@ absolute succeeds
@ prefix succeeds
@ full_prefix succeeds
@io_kythe relative fails
@io_kythe absolute fails
@io_kythe prefix succeeds
@io_kythe full_prefix succeeds
relative succeeds
absolute succeeds
prefix succeeds
full_prefix succeeds

What operating system are you running Bazel on?

Ubuntu Linux

What's the output of bazel info release?

release 4.0.0

What's the output of git remote get-url origin ; git rev-parse master ; git rev-parse HEAD ?

git@github.com:shahms/kythe.git
160b28af24a477fd75458e033523cf9ce81733ee
160b28af24a477fd75458e033523cf9ce81733ee

Have you found anything relevant by searching the web?

No, although I found #10927 which may be related.

@katre
Copy link
Member

katre commented Apr 20, 2021

Good news! This was recently fixed in d23e0a1, and in a long chain of preceeding changes which, unfortunately, are captured in an internal-only issue. I should have copied it into an externally visible one.

For reference, the changes are:

  • 68effbe - Create a new interface to allow Starlark objects to get a thread when getIndex is called
  • e35aedf - Renamed ExecGroupCollection
  • b9519f9 - Make StarlarkExecGroupContext use AutoValue
  • 627c16e - Extract a separate StarlarkToolchainContext
  • b120d4f - Fix toolchains to support type lookup
  • 95fe204 - Move label conversion into the LabelConversionContext object
  • f46b29c - Testing bazel modules should use non-relative labels
  • d23e0a1 - Fix relative label parsing with Starlark toolchain APIs
  • 04ab8d0 - Remove unused repoMapping data in ResolvedToolchainContext
  • dfe2a31 - Update the no-op ToolchainContext to throw an EvalException

Please try this with bazel build from head (or the latest green build with bazelisk) and it should be fixed.

@shahms
Copy link
Author

shahms commented Apr 20, 2021

It doesn't look like this is fixed at last_green:

$ USE_BAZEL_VERSION=last_green bazelisk build --extra_toolchains=@io_kythe//:dummy_toolchain_relative //:dummy
2021/04/20 14:00:08 Using unreleased version at commit 3d33d7b2dea8b4ea92162cc18b1ed2c7c31ad6e6
...
ERROR: While resolving toolchains for target //:dummy: no matching toolchains found for types //:dummy_toolchain_type
ERROR: Analysis of target '//:dummy' failed; build aborted: no matching toolchains found for types //:dummy_toolchain_type
INFO: Elapsed time: 0.174s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 227 targets configured)

It's easy enough to workaround, though.

@katre
Copy link
Member

katre commented Apr 20, 2021

I'll take a look, thanks.

@katre
Copy link
Member

katre commented Apr 21, 2021

Debugging this, the two failing cases have the same behavior:

  • The toolchain is registered as @io_kythe//:dummy_toolchain_type
  • The lookup is looking for //:dummy_toolchain_type

These don't match, and so there's a failure in resolution.

@c-parsons: You were telling me recently that you wanted to change repo mapping, would this be handled by the replacement?

@c-parsons
Copy link
Contributor

This is indeed the same issue I was running into with regards to cc toolchain resolution if I recall correctly. (whether or not the toolchain type was under @rules_cc or "current repository" (This is no longer blocking us because we hacked a workaround).

@katre
Copy link
Member

katre commented Apr 21, 2021

@c-parsons Is there a way for toolchain resolution to know that //:dummy_toolchain_type and @io_kythe//:dummy_toolchain_type are two labels for the same target?

@c-parsons
Copy link
Contributor

I don't recall off the top of my head, apologies. My memory and understanding of general toolchain resolution logic has faded over the last several months. Some of the toolchain-checking logic specific to rules_cc is inflexible with regards to repo renaming (example), and this is what we had to hack around.

@katre
Copy link
Member

katre commented Apr 21, 2021

Sorry, I wasn't clear. I'm not worried about the toolchain resolution part, I can handle that. But, given two labels that are in different repositories, what do I need to do to discover that they are actually pointing to the same target?

@philwo philwo added team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug untriaged labels Apr 28, 2021
@katre katre added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels May 3, 2021
@katre
Copy link
Member

katre commented May 3, 2021

I talked to @c-parsons some more, he's not sure if there's a helpful API.

I'm going to investigate further.

@katre katre self-assigned this May 3, 2021
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 10, 2023
@fmeum
Copy link
Collaborator

fmeum commented May 10, 2023

This should be fixed with Bazel 6, which applies the repo mapping to all command line options.

@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

No branches or pull requests

5 participants