-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Normalize C++ action config tool paths based on exec platform OS #22218
Conversation
9e03d9d
to
484d245
Compare
RELNOTES: Tool paths specified in `cc_toolchain` action configs are now normalized based on the current execution platform's OS rather than the host OS. In particular, Windows-style absolute paths are now treated as absolute paths when building on a Windows executor from a non-Windows host.
484d245
to
d119829
Compare
@lberki I implemented the solution you mentioned as your favorite one in #19208 (comment). |
As a follow-up, I would also use this new logic in CC @sluongng |
@lberki Gentle ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much rather @comius make the final call since he owns the C++ rules, but since he's quite overloaded, I tried to give this change a honest review,
// We *should* use a case-insensitive policy for OS.DARWIN, but we currently don't handle this. | ||
OsPathPolicy HOST_POLICY = | ||
OS.getCurrent() == OS.WINDOWS ? WindowsOsPathPolicy.INSTANCE : UnixOsPathPolicy.INSTANCE; | ||
OsPathPolicy HOST_POLICY = getFilePathOs(OS.getCurrent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: SERVER_POLICY
? ("host" is ambiguous, because it's also the old name for "exec")
For this reason, a comment would also be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding (per https://bazel.build/extending/platforms) is that we've retroactively redefined "host" to mean "the platform Bazel runs on" and introduced "exec" to mean "the platform where an action executes". The only reason it's ambiguous is that we haven't followed through in replacing "host" with "exec" everywhere it matters (notably, --host_*
flags which should really be called --exec_*
).
A comment would be welcome, yes, but IMO we should stick to "host" or "exec" instead of introducing yet another term.
* normalized on the current host OS. | ||
*/ | ||
final class UncheckedPathUnsupportedOnThisOsException | ||
extends UnsupportedOperationException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be an unchecked exception? It looks like it's only thrown from ShortPathResolver.resolve()
, which itself is only called through OsPathPolicy
from PathFragment()
so it should not be too bad to either use its checked version or turn this into an erroneous return value instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is also reachable via PathFragment#getRelative
, which has ~100 callsites in prod code in Bazel.
But as PathFragment#getRelative
can validly throw the exception (e.g. C:\foobar
is joined with foo~1.txt
on Linux for Windows), this could also result in the unchecked exception being thrown when C++ toolchain logic internally uses this function. I'm not sure what a good solution to this would look like.
@@ -1690,7 +1693,12 @@ static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct) throws | |||
throw infoError(toolStruct, "\"tool\" and \"path\" cannot be set at the same time."); | |||
} | |||
|
|||
toolPath = PathFragment.create(toolPathString); | |||
try { | |||
toolPath = PathFragment.createForOs(toolPathString, execOs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do understand correctly that this will raise an error where there was none, but that's OK because if Bazel encountered C:\WINDOWS32\SYSTEM\
style path on Linux, the situation would be unsalvageable anyway?
Actually: what about the case where Bazel runs on Linux, there is a toolchain definition for Windows somewhere but it's not used? Then it looks like Bazel would raise an error where there were none. I'm not sure if this scenario happens a lot because it would require someone to depend on the toolchain, which they don't if it's just a toolchain that's never selected.
@comius I have mostly given up on this for now. There are too many callers of |
Work towards #19208
RELNOTES: Tool paths specified in
cc_toolchain
action configs are now normalized based on the current execution platform's OS rather than the host OS. In particular, Windows-style absolute paths are now treated as absolute paths when building on a Windows executor from a non-Windows host.