-
Notifications
You must be signed in to change notification settings - Fork 117
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
build.bazel.remote.execution.v2.Command.arguments[0] should specify a path separator. #187
Comments
I think that solution 2 is the only logical one. Solution 1 would only make sense if you only call into programs inside the input root. It’s perfectly valid to also call into programs installed on the system: C:\Program Files\foo. Using forward slashes there would be really weird. Furthermore, this problem also applies to successive arguments. Those most likely need backslashes as well. It would be inconsistent if argv[0]’s conventions differ from the rest. |
+1 to Ed - we could standardize arg0 in some way perhaps, but we cannot
safely standardize anything about the remainder - we don't actually know if
an argument is a path (to be transformed) or a string that happens to
contain slashes (which servers should not touch in any way). And different
tools will have different support for paths, especially on Windows. (UNC
paths, etc). And at that point, it seems best to just say that the client
is responsible for providing OS-specific and tool-understandable
paths/arguments for all arguments, including arg0.
One wrinkle to that might be if cmd.exe adds any specific constraints that
aren't "Windows standard" - my preference for documentation would be
something along the lines of "*args must be formatted correctly for the OS
and for the tool they'll be passed to*", but if there are ways to comply
with that but also not be amenable to passing to cmd.exe directly, I guess
that's on our server to wrap/transform as necessary anyways to bridge the
gap.
…On Tue, Mar 9, 2021 at 1:22 AM Ed Schouten ***@***.***> wrote:
I think that solution 2 is the only logical one. Solution 1 would only
make sense if you only call into programs inside the input root. It’s
perfectly valid to also call into programs installed on the system:
C:\Program Files\foo. Using forward slashes there would be really weird.
Furthermore, this problem also applies to successive arguments. Those most
likely need backslashes as well. It would be inconsistent if argv[0]’s
conventions differ from the rest.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#187 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABREW4HJZE2UWHBKXFCF23TCW5A5ANCNFSM4Y22JH4A>
.
|
+1 to option 2. The client already has some knowledge (or makes
assumptions) about what the remote environment looks like--for example,
that executable /foo/bar/bas will exist in the remote environment--and this
feels similar. I also agree that the problem of universally processing
argv[n] is not solvable by the server, and that consistency is valuable
here.
On Tue, Mar 9, 2021 at 11:44 AM Eric Burnett <[email protected]>
wrote:
… +1 to Ed - we could standardize arg0 in some way perhaps, but we cannot
safely standardize anything about the remainder - we don't actually know if
an argument is a path (to be transformed) or a string that happens to
contain slashes (which servers should not touch in any way). And different
tools will have different support for paths, especially on Windows. (UNC
paths, etc). And at that point, it seems best to just say that the client
is responsible for providing OS-specific and tool-understandable
paths/arguments for all arguments, including arg0.
One wrinkle to that might be if cmd.exe adds any specific constraints that
aren't "Windows standard" - my preference for documentation would be
something along the lines of "*args must be formatted correctly for the OS
and for the tool they'll be passed to*", but if there are ways to comply
with that but also not be amenable to passing to cmd.exe directly, I guess
that's on our server to wrap/transform as necessary anyways to bridge the
gap.
On Tue, Mar 9, 2021 at 1:22 AM Ed Schouten ***@***.***>
wrote:
> I think that solution 2 is the only logical one. Solution 1 would only
> make sense if you only call into programs inside the input root. It’s
> perfectly valid to also call into programs installed on the system:
> C:\Program Files\foo. Using forward slashes there would be really weird.
>
> Furthermore, this problem also applies to successive arguments. Those
most
> likely need backslashes as well. It would be inconsistent if argv[0]’s
> conventions differ from the rest.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <
#187 (comment)
>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AABREW4HJZE2UWHBKXFCF23TCW5A5ANCNFSM4Y22JH4A
>
> .
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#187 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADMU23ZH7I5K62FQVUHZ52TTCZF7JANCNFSM4Y22JH4A>
.
|
I intended option 1 to apply exclusively to argv[0], since it should always (?) be a non-path separated function or command in the $PATH, or be the canonical path to an executable. Maybe it also makes sense to define some reasonable entry point on the server side? The user or the server could have weird setups (eg a user specified docker container with a cygwin bash entrypoint) that might be incompatible with the OS default (eg cygwin). It'd be unreasonable for bazel as a client tool to make those assumptions? |
The client also specifies the docker container, and that should allow it to make assumptions about the contents thereof. |
Motivation: bazelbuild/bazel#11636 (comment)
Throughout the API, both for inputs (in
SymlinkNodes
) and in outputs, we specify that separators should be single forward slashes (/
). However, we're not explicit on the Command arguments, which currently reads:The issue here comes from Windows
cmd.exe
not being able to naively recognize relative paths using/
separators as executables, eg:While there are many ways for the server to go around that restrictions (eg converting
/
->\
, wrapping the command in quotes, etc), we should it explicit whose responsibility it is to deal with this:/
or\
are valid, though I find little benefit to this approach, as it's a more ambiguous than 1 but forces the server to apply processing anyway (and maybe there exists a bizarre scenario where both are valid and yield different results?)I'm personally prefer 1. as the server may be already doing other processing on the command and figuring out the platform specific separator shouldn't be challenging, but I don't have a strong preference here as the approach is explicitly stated.
The text was updated successfully, but these errors were encountered: