Add non-specific output_paths to Command to replace output_files and …

[ola-rozenfeld]

…output_directories.

Fixes #75.

[peterebden]

Looks great, thanks - this will be ideal for us!

[ola-rozenfeld]

• @EricBurnett

[EricBurnett]

What does the expected transition for this look like - is the idea that clients should still specify both (if known; for compatibility), and servers will then use output_paths if they support it and the split fields otherwise?

Is it worth adding a "Clients SHOULD populate both ..." message to the comments on the deprecated fields to that effect?

[ola-rozenfeld]

The naive transition plan is: first thing, the clients will populate both fields, and the servers will support both fields. *This will cause a cache key change #1!* Then, as the relevant servers support both fields, the clients gradually stop populating the old fields, which causes a *cache key change #2*. There is a way around this using the Capabilities API, which I like better and propose to do instead: we cut a formal 2.1.0 release after this change is in. As servers switch to supporting both fields, they also begin reporting that they support a high_api_version of 2.1.0 in Capabilities API. Then Bazel (and remote-apis-sdk) will decide which fields to use based on server capabilities: use the new field if high_api_version>=2.1.0, and old fields otherwise. It is a little more complicated, but I like this plan because is causes *a single cache key change*. Also, it may be submitted independently of server changes. Given that, maybe the comment should be more like "During the transition period from output_files/directories to output_paths, the clients should make sure that the fields they populate are supported by their server, either populating both fields or using the Capabilities service." WDYT? +George Gensure <ggensure@uberatc.com> for thoughts.

…

On Fri, Sep 27, 2019 at 10:08 PM Eric Burnett ***@***.***> wrote: What does the expected transition for this look like - is the idea that clients should still specify both (if known; for compatibility), and servers will then use output_paths if they support it and the split fields otherwise? Is it worth adding a "Clients SHOULD populate both ..." message to the comments on the deprecated fields to that effect? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#96?email_source=notifications&email_token=AGFAVOCLSVWF6FR6HHGZMWDQL24DRA5CNFSM4I3GJXR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD72OMJI#issuecomment-536143397>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFAVOBAEHQX4VP4YKXZ5QDQL24DRANCNFSM4I3GJXRQ> .

[EricBurnett]

I like the idea of adding it to the capabilities API, so a server can declare support for one or both in an unambiguous way, and a client can give clear error messages if it's not going to be compatible.

On the client side though I'm less certain - turning two cache key changes into one is somewhat valuable, though certainly not critical for most parties, especially as tool version changes...e.g. bazel releases often cause implicit cache key changes anyways due to toolchain changes. Which might make it easier for some clients to simply set both fields still anyways, rather than worry about switching on the capabilities-returned version at the appropriate point in code, until such a time as they choose to drop the old version and the restriction it imposes.

At any rate, +1 for tying this change to API version 2.1. If you go that route, please make it clear in the field comments: "DEPRECATED as of 2.1"; "New in 2.1: ...", etc.

[juergbi]

Would it make sense to also deprecate output_file_symlinks and output_directory_symlinks, and replace them with the non-specific output_symlinks? If I remember correctly, the only reason we split symlink outputs into two fields was because of the corresponding split in Command.

I think this would simplify handling of symlink outputs as the server wouldn't have to dereference symlinks at all anymore (which can be a concern for code running outside the sandbox) and dangling symlinks would also not cause any issues.

[ola-rozenfeld]

Would it make sense to also deprecate output_file_symlinks and output_directory_symlinks, and replace them with the non-specific output_symlinks? If I remember correctly, the only reason we split symlink outputs into two fields was because of the corresponding split in Command.

I think this would simplify handling of symlink outputs as the server wouldn't have to dereference symlinks at all anymore (which can be a concern for code running outside the sandbox) and dangling symlinks would also not cause any issues.

I like this idea! There is one problem that I can think of: unlike with the input Command fields, the ActionResult fields are persistent. We cannot deprecate them nearly as easily. I mean, it will be easy for RBE specifically, because we don't actually implement output symlinks, but any server that does implement them will have to go through a manual migration to change the stored ActionResults.

I'm not sure how big of a problem this is. WDYT? Do we do it, and if yes, this PR or separate? I am inclined to do this separately, because both the impact/reasons of the symlink change and the migration path are sufficiently different from the output_path split.

[juergbi]

I was thinking that a change at the same time would make the migration easier. The API could specify that the new output_symlinks must be populated only for symlinks specified in output_paths. For clients that still use output_files and output_directories, the server would still populate output_file_symlinks and output_directory_symlinks, respectively (if the server supports it).

If I'm not forgetting anything, this should avoid the need for a manual migration. It would require clients to migrate to output_paths and output_symlinks at the same time, however, I don't expect this to be an issue.

[ola-rozenfeld]

I like @juergbi 's plan. It requires a bit more logic on the server side, for servers that implement output symlinks, but otherwise avoids the need to migrate.
Addressed the comments, PTAL!

[juergbi]

Looks good to me, thanks for the update.

[ola-rozenfeld]

Addressed a couple of Eric's comments:

• require deduplication of output_files for cache key purposes, but not require the server to check for it
• be more permissive/deliberately vague about what the server should do about the old output_<file/directory>_symlink fields. Enable Jurg's suggestion, but also make it clear that other options are possible -- as long as a server that reports backwards compatibility will correctly support it, it's all good.

@buchgr Jakob, can you please take a look to make sure this is okay from Bazel's PoV? In Bazel, I intend to use the new output_symlinks field if it is populated, otherwise use the old fields for outputs. As for inputs, there are two options: I would prefer to conditionally use output_paths if the server reports v2.1 compatibility, but if you prefer the simpler option of populating both fields, that's also fine.

Thank you!

[ola-rozenfeld]

Alright, I'll merge this and we'll see how to best handle it on the Bazel side later. I'll send a PR to Bazel after 2.1.0 is released, which will not be immediate because I want to address the other remaining PRs as well.

[werkt]

This deprecation of output_files was brought up today in the REAPI meeting, and admittedly was the first time that I noticed this change. Was there ever any movement on the bazel side for this? I couldn't find any Issue/PR references...

[ola-rozenfeld]

There was not, sorry! It was my plan to do it, but I moved away from RE-API work and didn't have the bandwidth. It should be a simple change on the Bazel-side, IIUC (add the new field alongside the old field, waiting on server-side implementations).

…

On Tue, May 12, 2020 at 11:42 AM George Gensure ***@***.***> wrote: This deprecation of output_files was brought up today in the REAPI meeting, and admittedly was the first time that I noticed this change. Was there ever any movement on the bazel side for this? I couldn't find any Issue/PR references... — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#96 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGFAVODUKB4ZJYLEKHN7NYDRRFU7FANCNFSM4I3GJXRQ> .

[ulfjack]

It would be great if someone could invite me to the REAPI meeting! For the output_files migration, the client can also look at the supported server versions rather than always sending both. On Tue, May 12, 2020 at 5:49 PM Ola Rozenfeld <notifications@github.com> wrote:

…

There was not, sorry! It was my plan to do it, but I moved away from RE-API work and didn't have the bandwidth. It should be a simple change on the Bazel-side, IIUC (add the new field alongside the old field, waiting on server-side implementations). On Tue, May 12, 2020 at 11:42 AM George Gensure ***@***.***> wrote: > This deprecation of output_files was brought up today in the REAPI > meeting, and admittedly was the first time that I noticed this change. Was > there ever any movement on the bazel side for this? I couldn't find any > Issue/PR references... > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > < #96 (comment)>, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AGFAVODUKB4ZJYLEKHN7NYDRRFU7FANCNFSM4I3GJXRQ > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#96 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYD2YIAXAVNJQNMVPNTMULRRFVZ3ANCNFSM4I3GJXRQ> .

[peterebden]

For the output_files migration, the client can also look at the supported server versions rather than always sending both.

Well that was why I brought it up - we would need to tag v2.1 of the API for that to be possible.
(Or in our case, we'd like to simply require that the server supported it and ditch the code supporting the alternative since it will inevitably get it wrong eventually and fail if the server is not permissive)

[mostynb]

It would be great if someone could invite me to the REAPI meeting!

I think you get an automatic invitation when you join the google group:
https://groups.google.com/forum/#!forum/remote-execution-apis

[sstriker]

FWIW, I added Ulf to the invite manually. Cheers, Sander

…

On Tue, May 12, 2020 at 7:56 PM mostynb ***@***.***> wrote: It would be great if someone could invite me to the REAPI meeting! I think you get an automatic invitation when you join the google group: https://groups.google.com/forum/#!forum/remote-execution-apis — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#96 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQVXZ2B4EOO5D2UMS6ADTLRRGEU3ANCNFSM4I3GJXRQ> .