Fix watching paths in undefined repos in repo rules

[Wyverald]

Watching a path in an undefined repo (eg. $output_base/external/i_am_undefined/blah) currently causes an NPE because we expect it to need a skyframe restart when it actually doesn't. The underlying reason is rather convoluted, but the gist is that we request RepositoryDirectoryValue.Key(@@i_am_undefined) explicitly, when we don't actually have to -- we can simply use FileValue.Key($output_base/external/i_am_undefined/blah directly, and rely on the fact that FileFunction knows to request the corresponding RepositoryDirectoryValue and behave appropriately when the repo is undefined. (This is due to logic in ExternalFilesHelper.maybeHandleExternalFile.)

Additionally, this PR removes the rctx.symlink(watch_target=) parameter. The symlink target's existence and its contents should not affect the creation of the symlink at all, so there's no reason to ever watch it.

Fixes #21483.

[fmeum]

... currently causes an NPE because we expect it to need a skyframe restart when it actually doesn't.

The changes look good to me, but couldn't this NPE still show up if something is available in Skyframe that we didn't expect to be ready? I would sleep better with logic preventing that entirely.

[Wyverald]

... currently causes an NPE because we expect it to need a skyframe restart when it actually doesn't.

The changes look good to me, but couldn't this NPE still show up if something is available in Skyframe that we didn't expect to be ready? I would sleep better with logic preventing that entirely.

Not sure what you mean here -- I was referring to the case where RepoCacheFriendlyPath.getRootedPath() returns null but actually doesn't need a Skyframe restart (ie. when the repo doesn't exist here).

[fmeum]

Ah, I see, then I agree we don't need to do anything more.

[lberki]

The code looks reasonable as far as I can tell (although I managed to code myself silly today), but do we want to allow repositories to depend on files in non-existent other repositories?

Until now, depending on non-existent repositories caused an error and you are breaking this invariant. I'd rather not because, well, it's another avenue for creativity.

[Wyverald]

Until now, depending on non-existent repositories caused an error and you are breaking this invariant. I'd rather not because, well, it's another avenue for creativity.

I don't think any invariant is being broken because until now there was no way to "depend on non-existent repositories" at all -- rctx.watch is a new API. Trying to rctx.read a nonexistent path (whether in an undefined repo or any other nonexistent path) will still result in an error.

[lberki]

Before this change, you couldn't say "please inform me if this non-existent repo springs into existence", now you can, if I understand correctly.

I haven't gamed out all the ways this could go wrong, but it looks like a lot of rope to me. I might be overly cautious here, but is there any reason to allow this functionality as opposed to just bailing out if one tries to watch a file in another repository, which does not exist?

[Wyverald]

I see... I don't think there's any explicit reason to allow it; it just happened naturally because that's what FileFunction does. Presumably the same could happen through any other FileFunction dependency on something in $output_base/external, though I don't know if that's possible outside of repo rules. Maybe we could just make RepositoryFunction.addExternalFilesDependencies() throw an exception if the repo in question is undefined. WDYT?

[lberki]

I'm afraid it's too late here for me to think through all the ramifications, but in general, we should either think through the consequences of behavior we allow or disallow it. The latter seems to be much simpler.

re: making addExternalFilesDependencies(), if anything breaks, my guess would be that it's when there is a symlink under a source root to such an undefined repository. I don't see any reason why anyone would possibly want to create such a symlink, so I'm for prohibiting it. If it doesn't work for some reason, hopefully our test battery will signal.

[Wyverald]

it's when there is a symlink under a source root to such an undefined repository.

ah, indeed -- so it's already possible today. IMO that lowers the priority of this drastically. Depending on the existence of a repo name through rctx.watch is exactly as nonsensical as having a symlink into that path. I'll send a change, but won't block 7.1.0 on it.

[lberki]

SGTM. I'd be worried if it was a regression, but it's not.