Fix platform label context in rule transitions#3997
Fix platform label context in rule transitions#3997tamasvajk wants to merge 2 commits intobazelbuild:mainfrom
Conversation
When a platform label has no repo name (i.e. from the main repo), str(label) produces "//platforms:foo" instead of "@//platforms:foo". Bazel's platform setting requires the canonical form with @, causing the transition to fail silently or select the wrong platform. Extract a _resolve_platform helper and apply it consistently to all four rule transitions (static_library, shared_library, binary, test).
UebelAndre
left a comment
UebelAndre
left a comment
There was a problem hiding this comment.
Thanks! I had one question on this.
| When a label has no repo name (e.g. from the main repo), str(label) omits | ||
| the leading @, producing "//platforms:foo" instead of "@//platforms:foo". | ||
| Bazel's platform setting requires the canonical form with @. |
There was a problem hiding this comment.
Is there any upstream issue for this? That seems somewhat surprising.
There was a problem hiding this comment.
I'm not aware of a specific upstream Bazel issue for this. The problem is that str(attr.platform) inside a transition implementation omits the @ prefix for labels from the main repository. Since the transition runs in the context of @rules_rust, Bazel then interprets //platforms:foo as @rules_rust//platforms:foo instead of @//platforms:foo.
This was discovered when using the platform attribute on rust_binary with a label from the main repo — the platform was silently resolved against rules_rust instead of the root workspace.
There was a problem hiding this comment.
I tried to set up a minimal repro, but I could only repro this with our patched Bazel. So the problem is actually not in rules_rust, but in our Bazel patch. I'm closing this PR.
There was a problem hiding this comment.
Can you file an upstream issue for this? Because platform is a attr.label I would have expected it to be in the correct form for all contexts and that a simple str would result in the correct behavior. I'm happy to accept the fix but would love to hear from the Bazel team first 🙏
There was a problem hiding this comment.
Also @krasimirgg do you maybe have any context here?
There was a problem hiding this comment.
Reopening — I was able to reproduce this with stock Bazel (not our patched version). The trigger is --noincompatible_unambiguous_label_stringification, which some projects set for compatibility reasons.
Minimal repro:
# WORKSPACE
workspace(name = "repro")
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(name = "rules_rust", sha256 = "d861...", urls = ["...0.64.0..."])
load("@rules_rust//rust:repositories.bzl", "rules_rust_dependencies", "rust_register_toolchains")
rules_rust_dependencies()
rust_register_toolchains()
# BUILD.bazel
load("@rules_rust//rust:defs.bzl", "rust_binary")
rust_binary(name = "hello", srcs = ["main.rs"], edition = "2021", platform = "//platforms:my_linux_x86_64")
# platforms/BUILD.bazel
platform(name = "my_linux_x86_64", constraint_values = ["@platforms//os:linux", "@platforms//cpu:x86_64"], visibility = ["//visibility:public"])$ bazel build //:hello --noincompatible_unambiguous_label_stringification
ERROR: no such package '@@rules_rust//platforms': BUILD file not found in directory 'platforms' of external repository @@rules_rust.
Root cause: With --noincompatible_unambiguous_label_stringification (which was the default before Bazel 6.0, see bazelbuild/bazel#15916), str(attr.platform) inside the transition produces //platforms:my_linux_x86_64 without the @ prefix. Bazel then resolves this relative to @@rules_rust instead of the main repo.
The flag was introduced to fix exactly this class of ambiguity, but projects that disable it (via --noincompatible_unambiguous_label_stringification) hit this bug.
The previous check `not attr.platform.repo_name` was incorrect because repo_name is empty for main-repo labels regardless of the stringification flag. With --incompatible_unambiguous_label_stringification (the default), str(label) already includes the @ prefix, so prepending another @ produced "@@//foo:bar" which is an invalid repository name. Check the string itself instead: only add @ when str() doesn't already start with one.
|
@UebelAndre What's the best way to rerun the CI jobs? There are some transient failures in the logs. |
2 participants
Summary
Fix platform label resolution in rule transitions for
rust_binary,rust_shared_library,rust_static_library, andrust_test.Problem
When a platform label comes from the main repository (no repo name),
str(label)produces//platforms:fooinstead of@//platforms:foo. Bazel's//command_line_option:platformssetting requires the canonical form with@, so the transition silently selects the wrong platform or fails.This affects anyone using the
platformattribute on Rust rules with a label defined in the main repository.Fix
Extract a
_resolve_platformhelper that adds the@prefix whenattr.platform.repo_nameis empty, and apply it consistently to all four rule transitions. The original patch only fixedrust_binary; this PR fixes all four for completeness.