Fix output base under /tmp.

[lberki]

Resolved symlink action outputs and param files are both fixed.

[fmeum]

@lberki I think that the missing case that is causing #20621 is that of a symlink to a source artifact. Since processResolvedSymlink doesn't take the source root mapping into account, it will return those symlinks unchanged, which is not correct.

I think that we would need to exercise the same logic as in L534-L545, but only if the symlink resolves to a source artifact. I am not sure how to check for the latter.

[fmeum]

Ah, I guess we can just check whether the resolved path lies under the exec root. If you don't get to work on this before the holidays, I can give it a try tomorrow.

[lberki]

You're absolutely right and in fact I discovered this independently due to some test breakages (our tests are winning!)-- by the time you wrote this, I had the fix. I pushed the latest version to #20603 . It works for me (protobuf, source tree + output base under /tmp, no disk cache). Mind also trying?

(don't look at the source code if you value your eyes, it's "hack to get all test cases green" quality and I'm not sure if all test cases are actually green)

[fmeum]

+1 for the test coverage, this is actually working now. I won't give a +1 for the control flow, but it's getting the job done ;⁠-⁠)

[lberki]

Yeah, I'm kinda embarrassed having it on the public Internet, even in a pull request :)

[lberki]

Update: this version fixes all known bugs modulo one, which is that the root directory of tree artifacts is stat()ed twice instead of once.

It can be fixed by replacing it with treeDir.isSymbolicLink() call in constructTreeArtifactValueFromFilesystem() with stat.isSymbolicLink() but that in turn breaks RemoteActionMetadataStoreTest and BwoBIntegrationTest because RemoteActionFileSystem violates the contract of FileSystem.stat(): stat.isSymbolicLink() returns false even if the original path was a symlink (it should return true even if the stat follows symlinks)

It's arguably silly to count stat() calls but not readlink() and default implementation of resolveSymbolicLinks() throws an exception on the common path (not a symlink) so there is a lot to be done here, but all of which is way more yak shaving than I had anticipated.

@tjgq what do? I'm inclined to update the test to allow for two stat() calls, file a bug on you and call it a day.

[tjgq]

@tjgq what do? I'm inclined to update the test to allow for two stat() calls, file a bug on you and call it a day.

See my reply in the internal CL :) I don't believe a contract is being violated. Two system calls are strictly required, so we should just fix the test assertion.

[lberki]

I'll drop this. fb6658c is the eventually submitted version. Unfortunately, it's quite far from the version here (@tjgq is a meticulous reviewer!) but the basic idea is the same.