Repository: vercel/next.js
Pull request: Turbopack: Symlink handling follow-up

### What?

* Run symlink glob tests with windows too
* make link content os specific before comparing it with the old value


Changed files:
filename: packages/next/src/build/utils.ts
patch: @@ -1276,7 +1276,7 @@ export async function copyTracedFiles(
               await fs.symlink(symlink, fileOutputPath)
             } catch (err: any) {
               // Windows doesn't support creating symlinks without elevated privileges, unless
-              // "Developer Mode" is turned on. If we failed to crate a symlink due to EPERM, try
+              // "Developer Mode" is turned on. If we failed to create a symlink due to EPERM, try
               // creating a junction point instead.
               //
               // Ideally we'd just preserve the input file type (junction point or symlink), but

filename: turbopack/crates/turbo-tasks-fs/src/lib.rs
patch: @@ -1069,6 +1069,43 @@ impl FileSystem for DiskFileSystem {
                 .transpose()?
                 .unwrap_or_default();
 
+            enum OsSpecificLinkContent {
+                Link {
+                    #[cfg(windows)]
+                    is_directory: bool,
+                    target: PathBuf,
+                },
+                NotFound,
+                Invalid,
+            }
+
+            let os_specific_link_content = match &*content {
+                LinkContent::Link { target, link_type } => {
+                    let is_directory = link_type.contains(LinkType::DIRECTORY);
+                    let target_path = if link_type.contains(LinkType::ABSOLUTE) {
+                        Path::new(&inner.root).join(unix_to_sys(target).as_ref())
+                    } else {
+                        let relative_target = PathBuf::from(unix_to_sys(target).as_ref());
+                        if cfg!(windows) && is_directory {
+                            // Windows junction points must always be stored as absolute
+                            full_path
+                                .parent()
+                                .unwrap_or(&full_path)
+                                .join(relative_target)
+                        } else {
+                            relative_target
+                        }
+                    };
+                    OsSpecificLinkContent::Link {
+                        #[cfg(windows)]
+                        is_directory,
+                        target: target_path,
+                    }
+                }
+                LinkContent::Invalid => OsSpecificLinkContent::Invalid,
+                LinkContent::NotFound => OsSpecificLinkContent::NotFound,
+            };
+
             // TODO(sokra) perform a untracked read here, register an invalidator and get
             // all existing invalidators
             let old_content = match retry_blocking(full_path.clone().into_owned(), |path| {
@@ -1084,21 +1121,12 @@ impl FileSystem for DiskFileSystem {
                 Ok(res) => Some((res.is_absolute(), res)),
                 Err(_) => None,
             };
-            let is_equal = match (&*content, &old_content) {
-                // TODO(bgw): Is this logic correct? This might give us `is_equal = false` when it
-                // shouldn't, and cause us to do extra work.
-                // - `old_content` may have windows path separators, but `LinkContent` normalizes
-                //   them.
-                // - Windows junction points are always transformed to absolute before being stored.
-                // - `LinkContent`'s target field stores absolute paths as relative to the project
-                //   root, but `old_target` is relative to the real filesystem's root.
-                // We should compute the final target `Path` first, and then it with that value, not
-                // the normalized format stored in
-                (LinkContent::Link { target, link_type }, Some((old_is_absolute, old_target))) => {
-                    Path::new(target) == old_target
-                        && link_type.contains(LinkType::ABSOLUTE) == *old_is_absolute
-                }
-                (LinkContent::NotFound, None) => true,
+            let is_equal = match (&os_specific_link_content, &old_content) {
+                (
+                    OsSpecificLinkContent::Link { target, .. },
+                    Some((old_is_absolute, old_target)),
+                ) => target == old_target && target.is_absolute() == *old_is_absolute,
+                (OsSpecificLinkContent::NotFound, None) => true,
                 _ => false,
             };
             if is_equal {
@@ -1114,23 +1142,13 @@ impl FileSystem for DiskFileSystem {
                 return Ok(());
             }
 
-            match &*content {
-                LinkContent::Link { target, link_type } => {
-                    let is_directory = link_type.contains(LinkType::DIRECTORY);
-                    let target_path = if link_type.contains(LinkType::ABSOLUTE) {
-                        Path::new(&inner.root).join(unix_to_sys(target).as_ref())
-                    } else {
-                        let relative_target = PathBuf::from(unix_to_sys(target).as_ref());
-                        if cfg!(windows) && is_directory {
-                            // Windows junction points must always be stored as absolute
-                            full_path
-                                .parent()
-                                .unwrap_or(&full_path)
-                                .join(relative_target)
-                        } else {
-                            relative_target
-                        }
-                    };
+            match os_specific_link_content {
+                OsSpecificLinkContent::Link {
+                    target,
+                    #[cfg(windows)]
+                    is_directory,
+                    ..
+                } => {
                     let full_path = full_path.into_owned();
 
                     let create_directory = old_content.is_none();
@@ -1156,7 +1174,7 @@ impl FileSystem for DiskFileSystem {
                             })?;
                     }
 
-                    retry_blocking(target_path, move |target_path| {
+                    retry_blocking(target.clone(), move |target_path| {
                         let _span = tracing::info_span!(
                             "write symlink",
                             name = display(target_path.display())
@@ -1178,23 +1196,24 @@ impl FileSystem for DiskFileSystem {
                     .await
                     .with_context(|| {
                         #[cfg(not(windows))]
-                        let message = format!("failed to create symlink to {target}");
+                        let message = format!("failed to create symlink to {}", target.display());
                         #[cfg(windows)]
                         let message = if is_directory {
-                            format!("failed to create junction point to {target}")
+                            format!("failed to create junction point to {}", target.display())
                         } else {
                             format!(
-                                "failed to create symlink to {target}\n\
-                                (Note: creating file symlinks on Windows require developer mode or admin permissions: https://learn.microsoft.com/en-us/windows/advanced-settings/developer-mode)"
+                                "failed to create symlink to {}\n\
+                                (Note: creating file symlinks on Windows require developer mode or admin permissions: https://learn.microsoft.com/en-us/windows/advanced-settings/developer-mode)",
+                                target.display()
                             )
                         };
                         message
                     })?;
                 }
-                LinkContent::Invalid => {
+                OsSpecificLinkContent::Invalid => {
                     bail!("invalid symlink target: {}", full_path.display())
                 }
-                LinkContent::NotFound => {
+                OsSpecificLinkContent::NotFound => {
                     remove_symbolic_link_dir_helper(&full_path)
                         .concurrency_limited(&inner.write_semaphore)
                         .await

filename: turbopack/crates/turbo-tasks-fs/src/read_glob.rs
patch: @@ -245,6 +245,29 @@ pub mod tests {
         glob::{Glob, GlobOptions},
     };
 
+    fn symlink<P: AsRef<std::path::Path>, Q: AsRef<std::path::Path>>(
+        target: Q,
+        path: P,
+    ) -> std::io::Result<()> {
+        assert!(target.as_ref().is_absolute());
+        let _ = std::fs::remove_dir(&path);
+        let _ = std::fs::remove_file(&path);
+
+        #[cfg(unix)]
+        {
+            std::os::unix::fs::symlink(target, path)
+        }
+        #[cfg(windows)]
+        {
+            let metadata = std::fs::metadata(&target).ok();
+            if metadata.is_none_or(|m| m.is_file()) {
+                std::os::windows::fs::symlink_file(target, path)
+            } else {
+                std::os::windows::fs::junction_point(target, path)
+            }
+        }
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn read_glob_basic() {
         let scratch = tempfile::tempdir().unwrap();
@@ -313,11 +336,8 @@ pub mod tests {
         .unwrap();
     }
 
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn read_glob_symlinks() {
-        use std::os::unix::fs::symlink;
-
         let scratch = tempfile::tempdir().unwrap();
         {
             // root.js
@@ -420,10 +440,8 @@ pub mod tests {
         path.track_glob(Glob::new(rcstr!("**"), GlobOptions::default()), false)
     }
 
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn track_glob_invalidations() {
-        use std::os::unix::fs::symlink;
         let scratch = tempfile::tempdir().unwrap();
 
         // Create a simple directory with 2 files, a subdirectory and a dotfile
@@ -506,13 +524,10 @@ pub mod tests {
         .unwrap();
     }
 
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn track_glob_symlinks_loop() {
         let scratch = tempfile::tempdir().unwrap();
         {
-            use std::os::unix::fs::symlink;
-
             // Create a simple directory with 1 file and a symlink pointing at at a file in a
             // subdirectory
             let path = scratch.path();
@@ -562,13 +577,10 @@ pub mod tests {
     }
 
     // Reproduces an issue where a dead symlink would cause a panic when tracking/reading a glob
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn dead_symlinks() {
         let scratch = tempfile::tempdir().unwrap();
         {
-            use std::os::unix::fs::symlink;
-
             // Create a simple directory with 1 file and a symlink pointing at a non-existent file
             let path = scratch.path();
             let sub = &path.join("sub");
@@ -625,13 +637,10 @@ pub mod tests {
     }
 
     // Reproduces an issue where a dead symlink would cause a panic when tracking/reading a glob
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn symlink_escapes_fs_root() {
         let scratch = tempfile::tempdir().unwrap();
         {
-            use std::os::unix::fs::symlink;
-
             // Create a simple directory with 1 file and a symlink pointing at a non-existent file
             let path = scratch.path();
             let sub = &path.join("sub");
@@ -657,13 +666,10 @@ pub mod tests {
         .unwrap();
     }
 
-    #[cfg(unix)]
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn read_glob_symlinks_loop() {
         let scratch = tempfile::tempdir().unwrap();
         {
-            use std::os::unix::fs::symlink;
-
             // Create a simple directory with 1 file and a symlink pointing at at a file in a
             // subdirectory
             let path = scratch.path();