Rewrite symlinks for vendored repositories #22723

meteorcloudy · 2024-06-12T13:59:47Z

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.

meteorcloudy · 2024-06-12T17:27:57Z

Sign... TIL: Windows resolves symlinks differently. Somehow cat /c/tmp/... works, but cat C:/tmp/ doesn't.

pcloudy@bazel-windows-playground:/c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpdbw1krzm (rewrite_symlinks)
$ ls -l /c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpxyvsr7o6/external/_main~ext~foo/path_foo
lrwxrwxrwx 1 pcloudy None 37 Jun 12 19:08 /c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpxyvsr7o6/external/_main~ext~foo/path_fo
o -> ../_bazel-external/_main~ext~foo/data
pcloudy@bazel-windows-playground:/c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpdbw1krzm (rewrite_symlinks)
$ cat /c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpxyvsr7o6/external/_main~ext~foo/path_foo
Hello from foo!
pcloudy@bazel-windows-playground:/c/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpdbw1krzm (rewrite_symlinks)
$ cat C:/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpxyvsr7o6/external/_main~ext~foo/path_foo
cat: 'C:/tmp/74bsjlkt/execroot/_main/_tmp/9293b221820db76a6f9697c2a01fde7c/tests_root/tmpxyvsr7o6/external/_main~ext~foo/path_foo': No such file or directory

meteorcloudy · 2024-06-13T09:35:15Z

@fmeum @Wyverald I finally made this work, the windows part is a bit annoying. Please take a look!

site/en/external/vendor.md

fmeum · 2024-06-13T10:12:33Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java

+   */
+  private void replantSymlinks(Path repoUnderVendor, Path externalRepoRoot) throws IOException {
+    try {
+      Collection<Path> symlinks = FileSystemUtils.traverseTree(repoUnderVendor, Path::isSymbolicLink);


Could we inline this into the moveTree file system traversal to avoid another one?

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?

Not at this point, no. But maybe we could add some profiler spans?

Good point! Profile added.

fmeum · 2024-06-13T10:14:07Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java

+            .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.


That seems error-prone, could we fix its implementation instead?

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?

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.

fmeum · 2024-06-13T10:16:08Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java

+          // 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()));


Are we also rewriting junctions to symlinks here? I'm asking because the former wouldn't require admin rights, but the latter may.

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.

Since junction only supports absolute path, this has to be a symlink. I updated the doc to mention this.

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

meteorcloudy requested review from gregestren, fweikert and Wyverald as code owners June 12, 2024 13:59

meteorcloudy requested review from fmeum and removed request for gregestren and fweikert June 12, 2024 13:59

github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Jun 12, 2024

fmeum reviewed Jun 13, 2024

View reviewed changes

meteorcloudy added 14 commits June 17, 2024 17:12

Rewrite symlinks under vendor dir

a5db01c

Rewrite symlinks for vendored repositories

b239b28

add doc

3361a8e

small change

9018dda

small change

6558975

Fix test

998da16

Deal with windows

71965ff

Fix typo

1ce22c5

Create extra symlink on Windows

20fafe4

test symlink for dir

4f4d5bf

Add some comments

0013bea

minor fix

e9989da

Rename IsJunction to IsReparsePoint

b263af6

Rename _bazel-external to bazel-external

8edd507

meteorcloudy force-pushed the rewrite_symlinks branch from 8f15f54 to 8edd507 Compare June 17, 2024 15:13

Update windows doc

28ac924

fmeum approved these changes Jun 17, 2024

View reviewed changes

Add profile

2415962

copybara-service bot closed this in c94c1c3 Jun 18, 2024

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite symlinks for vendored repositories #22723

Rewrite symlinks for vendored repositories #22723

meteorcloudy commented Jun 12, 2024 •

edited

Loading

meteorcloudy commented Jun 12, 2024

meteorcloudy commented Jun 13, 2024

fmeum Jun 13, 2024

meteorcloudy Jun 17, 2024

fmeum Jun 17, 2024

meteorcloudy Jun 18, 2024

fmeum Jun 13, 2024

meteorcloudy Jun 17, 2024

meteorcloudy Jun 17, 2024

fmeum Jun 13, 2024

meteorcloudy Jun 17, 2024

meteorcloudy Jun 17, 2024

Rewrite symlinks for vendored repositories #22723

Rewrite symlinks for vendored repositories #22723

Conversation

meteorcloudy commented Jun 12, 2024 • edited Loading

meteorcloudy commented Jun 12, 2024

meteorcloudy commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meteorcloudy commented Jun 12, 2024 •

edited

Loading