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

Fixing OutputFile comment: path is relative to working directory. #26

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

ola-rozenfeld
Copy link
Contributor

Fixes #25.

@googlebot googlebot added the cla: yes Pull requests whose authors are covered by a CLA with Google. label Aug 22, 2018
@sstriker
Copy link
Collaborator

This would actually be a spec change. I don't think we can chalk this up as a comment fix.

@ola-rozenfeld
Copy link
Contributor Author

It's not a spec change. You can see it being documented correctly in line https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/remote_execution.proto#L431, and the current line just straight-up contradicts that one, because I forgot to change it.

@sstriker
Copy link
Collaborator

I clearly overlooked something about the proposal on Paths + Whole directory special case. I assumed wrongly that absolute pats would refer to the input_root.
For the BuildStream client we have a use case where we do want to retrieve files relative to the input_root. Shall I file a new issue and proposal for that?

@ola-rozenfeld
Copy link
Contributor Author

ola-rozenfeld commented Aug 23, 2018 via email

@sstriker
Copy link
Collaborator

@ola-rozenfeld that sounds about right. Now, to refer to the input_root for output_*, should I open up a new proposal that allows passing of absolute paths that are rooted in input_root? Or should I pass in ../../../subdir_of_input_root for output_directory? The latter is legit I think according to spec, but a bit awkward.

@ola-rozenfeld
Copy link
Contributor Author

You are talking about the case where the working directory is a subdir of input root, right? For that one, you pass a ../other_subdir_of_input_root/myoutput path. I think when we discussed "Paths + Whole directory special case" we decided that using absolute paths to refer to something else, like input root, was too confusing.

@ola-rozenfeld ola-rozenfeld merged commit 6130f7e into bazelbuild:master Aug 30, 2018
santigl pushed a commit to santigl/remote-apis that referenced this pull request Aug 26, 2020
…azelbuild#26)

* A Chunker to support memory-bounded buffered uploads with lazy file
reads.

* Addressing offline comments

* Addressing comments

* Making the Digest public instead of just the Size

* Reconsidered, I don't need to have Digest as part of Chunk.

* Adding the post-Next-reliability contraint into the unit tests. Enabling more tests.

* Stupid Go won't print an int into a %s...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Pull requests whose authors are covered by a CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants