Support `SymlinkNode` in `ComputeMerkleTree()`? about remote-apis-sdks HOT 9 OPEN

If we won't get response from @peterebden before Nov 4th (JST), I'll ask @k-ye to work for this.

from remote-apis-sdks.

Apologies for the delay - I haven't had any time to look into this further (and probably won't soon unfortunately). Thanks for picking it up!

from remote-apis-sdks.

I guess this feature request somehow works against #134 ..

from remote-apis-sdks.

We could add this support, but it would have to be optional (additional Client option that configures how symlinks should be handled). The options are: pretend that symlinks are regular files/directories (current); pretend that absolute symlinks are regular files/directories, but use a SymlinkNode for relative symlinks; use SymlinkNode for everything. The later distinction can be made on the basis of the server Capabilities; alternatively, it can be configuration, but the client will error out of server Capabilities disallow it. Also, what to do about invalid symlinks will need to be configured as well (ignore, error, or create a dangling SymlinkNode).

Yes, you're right that this would have to go all the way through to FileMetadata.
All in all, not a simple feature -- I supported it in Bazel at one point, and it was a bit annoying to get in.

Can you please expand on your current usage of the SDK and how this will help you, so that we could better prioritize? Thank you!

from remote-apis-sdks.

pretend that absolute symlinks are regular files/directories, but use a SymlinkNode for relative symlinks; use SymlinkNode for everything.

For Chromium Swarming's .isolate file, all the file paths are relative, so this two options are essentially the same for our usage. Besides, we'd also like to support dangling symlink nodes :-)

Can you please expand on your current usage of the SDK.

Sure. Basically, an .isolate defines a list of files we'd like to upload to RBE-CAS. So in our prototype impl, it will probably be as simple as:

rootDg, chunkers, stats, err := ComputeMerkleTree(execRoot, isFromIsolate, chunkSize, cache)
if err == nil {
    client.UploadIfMissing(ctx, chunkers...)
}
// |rootDg| should contain file, directory and *symlink* nodes.

@atetubou has made #143 so that Directory of execRoot is also included inside the chunkers.

Thanks!

from remote-apis-sdks.

So we ran across this using more or less the same code as above (hence why I sent #188). From my perspective the current behaviour is undesirable; it silently converts symlinks to regular files which can break consumers that care about the difference (which is how we found it).

To me the obvious thing I would expect is to get SymlinkNodes back that describe the original symlink (the third option above); I'm not sure that this needs to directly concern itself with whether the links are dangling or not (that seems like a concern for the author of the rules; a dangling link is potentially valid in some contexts). Noted that the rex API has this setting for whether absolute symlinks are allowed, but the current behaviour doesn't honour that (it converts to the regular file that's pointed to regardless of the destination).

from remote-apis-sdks.

from remote-apis-sdks.

This is now a problem in our usage in https://crbug.com/1143567.
We are handling mac test files containing
https://en.wikipedia.org/wiki/Bundle_(macOS)
and test fails if we send regular file instead of symlink.

@peterebden do you still plan to work for this? If not, I will work for this issue.

from remote-apis-sdks.

Let me take this over.

from remote-apis-sdks.