[WIP] Unresolved symlinks #15866
[WIP] Unresolved symlinks #15866
Conversation
The check in ActionMetadataHandler that an output declared to be a symlink is indeed created as a symlink by the action was ineffective as it would only run if a stat of the output showed that is already was a symlink. The test only passed by accident since it runs sandboxed and the sandbox setup would call Path.readSymbolicLink on what it expects to be a symlink. As this does not happen on Windows, the test correctly fails there. This is fixed by calling Path.readSymbolicLink on the output path of an expected symlink before rather than after stat-ing it and trusting the file type contained in the stat info. With this issue fixed, bazel_symlink_test can be enabled on Windows with the following test-only changes: * Pass --windows_enable_symlinks as a startup option. * Use relative instead of absolute symlink targets as these have consistent shape under Unix and Windows. * Use a helper java_binary to create symlinks rather than `ln -s`, which doesn't seem to be able to create unresolved symlinks on Windows.
fmeum
force-pushed
the
unresolved-symlinks
branch
from
48a2b63 to
6a65562
Compare
Also adds support for such symlinks to the remote worker implementation.
fmeum
force-pushed
the
unresolved-symlinks
branch
from
6a65562 to
e1e2f58
Compare
|
Is there a bug and design behind this? |
| if (e instanceof NotASymlinkException) { | ||
| reporter.handle(Event.error( | ||
| action.getOwner().getLocation(), | ||
| String.format("declared output '%s' is not a symlink", output.prettyPrint()))); |
There was a problem hiding this comment.
How about "Output '%s' was declared as a symlink, but is a '%s'"?
There was a problem hiding this comment.
Good point, will update the corresponding PR if possible.
| @@ -185,14 +185,14 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { | |||
| } | |||
|
|
|||
| @Override | |||
| protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) | |||
| protected void createSymbolicLink(PathFragment linkPath, String rawTarget) | |||
There was a problem hiding this comment.
I am none too happy about turning this into just taking a raw string. What is it about the PathFragment normalization that needs to be addressed in this commit? Passing around simple strings increases the ways things can go wrong.
There was a problem hiding this comment.
A symlink with target bar/../foo is not the same as a symlink with target foo: If bar is itself a symlink (or a junction on Windows), then bar/../foo will resolve relative to the target of bar.
Given these complications around symlinks, there has to be a way for Bazel to create "raw" symlinks via its filesystem abstractions. The respective PR is still WIP, but I plan to add comments explaining that the PathFragment overload should be preferred in almost all cases - except where it doesn't work, e.g. when explicitly creating a raw symlink.
I agree that the context would be really useful here. That also goes together with @larsrc-google's excellent question about using raw string for symlinks--the explanation is sufficient for why that technically is needed, but it would be interesting to know what particular use cases need the symlinks not to be normalized. |
|
@alexjski Please see the individual PRs for context:
More recent context can also be found at #15454. The current PR is merely a testbed to verify all the PRs function together, it is not a good starting point for a review (hence is marked as Draft). |
Thank you for the explanation! Should we move the discussion to the corresponding PRs instead? |
|
@alexjski You might see some context in bazelbuild/rules_pkg#115. The basic requirement is that when you are building distribution packages, you almost always want symlinks preserved rather than followed. For example, you want a binary and support files to live under /usr/share/my_package/{bin, lib, messages, ...}, and you want a symlink from I've been approaching this from the rules_pkg side. My latest thinking is that
|
No description provided.