Tidy up the semantics of Command.arguments[0] #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
google-cla
bot
added
the
cla: yes
EdSchouten
force-pushed
the
eschouten/20211112-argv0
branch
from
8fedefa to
3773a16
Compare
mostynb
reviewed
Nov 13, 2021
EdSchouten
force-pushed
the
eschouten/20211112-argv0
branch
from
3773a16 to
b6c86e1
Compare
mostynb
reviewed
Jan 11, 2022
EdSchouten
force-pushed
the
eschouten/20211112-argv0
branch
2 times, most recently
from
8b41954 to
e81f7e3
Compare
bergsieker
approved these changes
Feb 7, 2022
ulfjack
reviewed
Feb 8, 2022
Bazel Buildfarm, Buildgrid and Buildbarn all perform resolution relative to the working directory of an action; not the input root directory. Instead of requiring that all implementations are updated, we should consider just altering the spec. Performing resolution relative to the input root directory can also be very tricky, as it means that argv[0] as visible to the calling process must also be rewritten. Applications may get confused otherwise. For example, consider the case where the working directory is "foo" and argv[0] is "bar/baz". In that case argv[0] as visible to the calling process must become "../bar/baz" or be made absolute. Making it absolute is inconsistent with what Bazel does right now. Attempting to keep it relative can be complex when symbolic links are involved. Furthermore, the specification doesn't mention what kind of path separators are used for argv[0]. The only reasonable solution here is to use path separators that are native to the host, as successive arguments also need to be provided in that form.
EdSchouten
force-pushed
the
eschouten/20211112-argv0
branch
from
e81f7e3 to
3d0b812
Compare
|
Hey @ulfjack, could you approve this PR if you're fine with the latest version? |
ulfjack
approved these changes
Apr 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bazel Buildfarm, Buildgrid and Buildbarn all perform resolution relative
to the working directory of an action; not the input root directory.
Instead of requiring that all implementations are updated, we should
consider just altering the spec.
Performing resolution relative to the input root directory can also be
very tricky, as it means that argv[0] as visible to the calling process
must also be rewritten. Applications may get confused otherwise. For
example, consider the case where the working directory is "foo" and
argv[0] is "bar/baz". In that case argv[0] as visible to the calling
process must become "../bar/baz" or be made absolute. Making it absolute
is inconsistent with what Bazel does right now. Attempting to keep it
relative can be complex when symbolic links are involved.
Furthermore, the specification doesn't mention what kind of path
separators are used for argv[0]. The only reasonable solution here is to
use path separators that are native to the host, as successive arguments
also need to be provided in that form.
EDIT: As discussed on the mailing list, let's re-add PATH resolution. RBE, Buildbarn and Bazel Buildfarm all do that right now.