Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Follow symlinks while walking files #2141

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

0xbe7a
Copy link
Contributor

@0xbe7a 0xbe7a commented Sep 26, 2024

I am using pixi with detached environments. I have a postinstall task that runs pip install --no-build-isolation --no-deps --disable-pip-version-check -e .. I want to specify an output for this task that checks if the package is installed in .pixi/envs/defaults/... so that I can cache this task. This currently fails because the Walker used does not follow symlinks, and .pixi just points to my detached environments via a symlink. This PR enables resolving symlinks and adds a test for that.

@0xbe7a 0xbe7a changed the title Follow symlinks while walking files feat: Follow symlinks while walking files Sep 26, 2024
Comment on lines 203 to 204
std::os::windows::fs::symlink_dir(symlinked_dir.path(), target_dir.path().join("link"))
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symlinks require dev mode or admin rights on Windows to work. Maybe @baszalmstra can test whether this test works on a "normal" machine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed not work on my machine:

thread 'task::file_hashes::test::compute_hashes' panicked at src\task\file_hashes.rs:204:18:
called `Result::unwrap()` on an `Err` value: Os { code: 1314, kind: Uncategorized, message: "A required privilege is not held by the client." }

Copy link
Contributor

@ruben-arts ruben-arts Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xbe7a We can simply add a check if the symlink happened on windows, and only then assert. e.g.:

diff --git a/src/task/file_hashes.rs b/src/task/file_hashes.rs
index 25f5d405..eebc424e 100644
--- a/src/task/file_hashes.rs
+++ b/src/task/file_hashes.rs
@@ -198,10 +198,14 @@ mod test {
             std::os::unix::fs::symlink(symlinked_dir.path(), target_dir.path().join("link"))
                 .unwrap();
         }
+
+        // On Windows this test can fail, so we need to check if the symlink was created successfully.
+        // This works in our CI but might not work on all Windows systems.
+        #[allow(unused_assignments)]
+        let mut symlink_on_windows = false;
         #[cfg(windows)]
         {
-            std::os::windows::fs::symlink_dir(symlinked_dir.path(), target_dir.path().join("link"))
-                .unwrap();
+            symlink_on_windows = std::os::windows::fs::symlink_dir(symlinked_dir.path(), target_dir.path().join("link")).is_ok();
         }
 
         write(symlinked_dir.path().join("main.rs"), "fn main() {}").unwrap();
@@ -237,13 +241,15 @@ mod test {
             Some("2c806b6ebece677c")
         );

-        assert_matches!(
-            hashes
-                .files
-                .get(Path::new("link/main.rs"))
-                .map(String::as_str),
-            Some("2c806b6ebece677c")
-        );
+        if symlink_on_windows || cfg!(unix) {
+            assert_matches!(
+                hashes
+                    .files
+                    .get(Path::new("link/main.rs"))
+                    .map(String::as_str),
+                Some("2c806b6ebece677c")
+            );
+        }

         #[cfg(unix)]
         {his works for me and I guess it's still tested on CI

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @0xbe7a

@ruben-arts ruben-arts merged commit 33874c0 into prefix-dev:main Sep 27, 2024
39 checks passed
@0xbe7a 0xbe7a deleted the tasks-follow-symlinks branch September 28, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants