Rewrite symlinks for vendored repositories#22723
Rewrite symlinks for vendored repositories#22723meteorcloudy wants to merge 16 commits intobazelbuild:masterfrom
Conversation
|
Sign... TIL: Windows resolves symlinks differently. Somehow |
| */ | ||
| private void replantSymlinks(Path repoUnderVendor, Path externalRepoRoot) throws IOException { | ||
| try { | ||
| Collection<Path> symlinks = FileSystemUtils.traverseTree(repoUnderVendor, Path::isSymbolicLink); |
There was a problem hiding this comment.
Could we inline this into the moveTree file system traversal to avoid another one?
There was a problem hiding this comment.
Hmm, I have to re-implement the moveTreesBelow to inline this. I'm not so convinced of the trade off between the performance gain and the duplicated code. Do you really think this optimization very important?
There was a problem hiding this comment.
Not at this point, no. But maybe we could add some profiler spans?
There was a problem hiding this comment.
Good point! Profile added.
| .getRelative(EXTERNAL_ROOT_SYMLINK_NAME) | ||
| .getRelative(target.relativeTo(externalRepoRoot.asFragment())); | ||
| if (OS.getCurrent() == OS.WINDOWS){ | ||
| // On Windows, FileSystemUtils.ensureSymbolicLink always resolves paths to absolute path. |
There was a problem hiding this comment.
That seems error-prone, could we fix its implementation instead?
There was a problem hiding this comment.
Indeed, I did a few test on Windows, it seems even long paths work with relative paths if resolved properly. But still, I prefer to implement this in a follow-up change, so that we can rollback easily if things go wrong. WDYT?
There was a problem hiding this comment.
Hmm, just find out that junctions can only be absolute paths: https://superuser.com/questions/343074/directory-junction-vs-directory-symbolic-link
Also verified on Windows
pcloudy@BAZEL-WINDOWS-P C:\Users\pcloudy\workdir\junction_test>mklink /J foo bar
Junction created for foo <<===>> bar
pcloudy@BAZEL-WINDOWS-P C:\Users\pcloudy\workdir\junction_test>dir
Volume in drive C has no label.
Volume Serial Number is 80B1-D386
Directory of C:\Users\pcloudy\workdir\junction_test
06/17/2024 08:22 PM <DIR> .
06/17/2024 08:22 PM <DIR> ..
06/17/2024 08:22 PM <JUNCTION> foo [C:\Users\pcloudy\workdir\junction_test\bar]
0 File(s) 0 bytes
3 Dir(s) 8,777,269,248 bytes free
That means it's gonna be hard to fix FileSystemUtils.ensureSymbolicLink since if the target is a relative path pointing to a directory, then we have to decide which property to prioritize. Choosing relative path means we need admin right (or developer mode) to create actual symlink, choosing junction means we lose the relative path. And the second choice doesn't work for our use case.
| // On Windows, FileSystemUtils.ensureSymbolicLink always resolves paths to absolute path. | ||
| // Use Files.createSymbolicLink here instead to preserve relative target path. | ||
| symlink.delete(); | ||
| Files.createSymbolicLink(java.nio.file.Path.of(symlink.getPathString()), java.nio.file.Path.of(newTarget.getPathString())); |
There was a problem hiding this comment.
Are we also rewriting junctions to symlinks here? I'm asking because the former wouldn't require admin rights, but the latter may.
There was a problem hiding this comment.
Nope, the Files. class doesn't offer any way to create Junction via API. But if we can solve the FileSystemUtils.ensureSymbolicLink problem, it should be able to do that.
There was a problem hiding this comment.
Since junction only supports absolute path, this has to be a symlink. I updated the doc to mention this.
8f15f54 to
8edd507
Compare
To make sure symlinks work correctly, Bazel uses the following strategy to rewrite symlinks in the vendored source: - Create a symlink `<vendor_dir>/bazel-external` that points to `$(bazel info output_base)/external`. It is refreshed by every Bazel command automatically. - For the vendored source, rewrite all symlinks that originally point to a path under `$(bazel info output_base)/external` to a relative path under `<vendor_dir>/bazel-external`. Fixes bazelbuild#22303 Closes bazelbuild#22723. PiperOrigin-RevId: 644349481 Change-Id: I853ac0ea5405f0cf58431e988d727e690cbbb013
To make sure symlinks work correctly, Bazel uses the following strategy to rewrite symlinks in the vendored source:
<vendor_dir>/bazel-externalthat points to$(bazel info output_base)/external. It is refreshed by every Bazel command automatically.$(bazel info output_base)/externalto a relative path under<vendor_dir>/bazel-external.Fixes #22303