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.

Sign up for GitHub

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

Jump to bottom

feat: Follow symlinks while walking files #2141

Merged
merged 5 commits into from
Sep 27, 2024
Merged

feat: Follow symlinks while walking files #2141

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8de7dc
Follow symlinks while walking files
0xbe7a Sep 26, 2024
3d6ef67
fmt
0xbe7a Sep 26, 2024
abcf54b
use windows api on windows
0xbe7a Sep 26, 2024
39015ff
symlinking on windows might fail
0xbe7a Sep 27, 2024
baf5661
fix: clippy warning
ruben-arts Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/task/file_hashes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ impl FileHashes {
.git_ignore(false)
.git_global(false)
.git_exclude(false)
.follow_links(true)
.build_parallel()
.run(|| {
let tx = tx.clone();
Expand Down Expand Up @@ -189,11 +190,29 @@ mod test {
write(target_dir.path().join("src/bla/lib.rs"), "fn main() {}").unwrap();
write(target_dir.path().join("Cargo.toml"), "[package]").unwrap();

// Compute the hashes of all files in the directory that match a certain set of includes.
let hashes =
FileHashes::from_files(target_dir.path(), vec!["src/*.rs", "*.toml", "!**/lib.rs"])
.await
// create symlinked directory
let symlinked_dir = tempdir().unwrap();

#[cfg(unix)]
{
std::os::unix::fs::symlink(symlinked_dir.path(), target_dir.path().join("link"))
.unwrap();
}
#[cfg(windows)]
{
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 •
edited
Loading

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

}

write(symlinked_dir.path().join("main.rs"), "fn main() {}").unwrap();

// Compute the hashes of all files in the directory that match a certain set of includes.
let hashes = FileHashes::from_files(
target_dir.path(),
vec!["src/*.rs", "*.toml", "!**/lib.rs", "link/*.rs"],
)
.await
.unwrap();

assert!(
!hashes.files.contains_key(Path::new("build.rs")),
Expand All @@ -218,12 +237,20 @@ mod test {
Some("2c806b6ebece677c")
);

assert_matches!(
hashes
.files
.get(Path::new("link/main.rs"))
.map(String::as_str),
Some("2c806b6ebece677c")
);

#[cfg(unix)]
{
let mut hasher = Xxh3::new();
hashes.hash(&mut hasher);
let s = format!("{:x}", hasher.finish());
assert_eq!(s, "be05bb5d7c6e8e6");
assert_eq!(s, "96308f0071086f62");
}

let hashes = FileHashes::from_files(target_dir.path(), vec!["src/"])
Expand Down
Loading