Handle symlinks to directories

[thomwiggers]

When cargo encounters a link, check if it's a directory when considering it for further processing.
I'm not sure how this interacts with things such as submodules – it allowed me to publish the crate pqcrypto-kyber768 which uses a link to a submodule.

Fixes #2748.

This PR:

• Add new tests that demonstrate that links to dirs can't be packaged
• Fixes those tests
• Enabled symlink tests to run on Windows
• Fix a bug in the handling of symlinks
• Add new test runner behaviour on Windows (it will ignore symlink-related tests and instead run them if you specify --ignored.)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! I sort of forget the context around this code, but do you remember why COMMIT means directory? Additionally would it be possible to add a test for this as well?

I wonder if we could possibly query git instead of the filesystem to attempt to learn if a symlink points to a directory?

[thomwiggers]

I have no clues how any of those things work, unfortunately.

• pygit2 is a black box of mystery, with not-great docs
• I don't know how the test cases work. They seem to be generated somehow, I haven't looked at them.

[ehuss]

COMMIT is a git submodule. The is_dir code below will then walk the submodule (or if it is not a submodule, just walk the filesystem).

Querying git looks like it would be more trouble than it's worth. It would need to read the link's contents, do an Index::get_path, and check the result of that, and still handle the case where get_path returns another link or returns None (and it would wind up needing to get the metadata anyways). It would be a nontrivial amount of code for an edge case for an optimization.

I would suggest maybe something like this would be simpler.

diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs
index fd2e4bb5..d35e75ca 100644
--- a/src/cargo/sources/path.rs
+++ b/src/cargo/sources/path.rs
@@ -323,9 +323,17 @@ impl<'cfg> PathSource<'cfg> {
         // the untracked files are often part of a build and may become relevant
         // as part of a future commit.
         let index_files = index.iter().map(|entry| {
-            use libgit2_sys::GIT_FILEMODE_COMMIT;
-            let is_dir = entry.mode == GIT_FILEMODE_COMMIT as u32;
-            (join(root, &entry.path), Some(is_dir))
+            use libgit2_sys::{GIT_FILEMODE_COMMIT, GIT_FILEMODE_LINK};
+            // `is_dir` is an optimization to avoid calling `fs::metadata` on
+            // every file.
+            let is_dir = if entry.mode == GIT_FILEMODE_LINK as u32 {
+                // Let the code below figure out if this symbolic link points
+                // to a directory or not.
+                None
+            } else {
+                Some(entry.mode == GIT_FILEMODE_COMMIT as u32)
+            };
+            (join(root, &entry.path), is_dir)
         });
         let mut opts = git2::StatusOptions::new();
         opts.include_untracked(true);

@thomwiggers The tests are not generated. There is an introduction to writing tests at testsuite/support/mod.rs. There are a variety of git-helpers in support/git.rs. I would stick the test in testsuite/package.rs. There is a similar test in there (package_git_submodule) which you can use for inspiration, though just creating a symlink should be simpler.

[thomwiggers]

What is the desired behaviour for these symlinks anyway?

Say we have the folowing project layout:

target/cit/t0/
├── foo
│   ├── Cargo.lock
│   ├── Cargo.toml
│   ├── src
│   │   └── lib.rs
│   ├── submodule
│   │   └── Makefile
│   ├── submodule-link -> /home/thom/git/cargo/target/cit/t0/foo/submodule
│   └── target
│       └── package
│           └── foo-0.0.1.crate
├── home
└── submodule
    └── Makefile

8 directories, 6 files

Do we then want to handle the symlink like a copy of the submodule folder (how it's currently handled) or do we want Cargo to properly package a symlink.

What about the following scenario?

/home/thom/git/phd/tls/pqcrypto/
├── pqclean
│   └── Makefile, etc
├── pqcrypto-kyber
│   ├── pqclean -> ../pqclean
│   └── src

There just archiving as a symlink would probably not be sufficient, and you do want cargo to copy the files in .../pqclean over to pqcrypto-kyber.crate.

[thomwiggers]

There appears to be a compile issue in unrelated code that I can't reproduce locally.

error: cannot borrow `*p` as mutable because it is also borrowed as immutable
   --> tests\testsuite\support\mod.rs:794:17
    |
793 |             if let Some(cwd) = p.get_cwd() {
    |                                - immutable borrow occurs here
794 |                 p.cwd(cwd.join(path.as_ref()));
    |                 ^     --- immutable borrow later used here
    |                 |
    |                 mutable borrow occurs here
    |
note: lint level defined here
   --> tests\testsuite\main.rs:2:45
    |
2   | #![cfg_attr(feature = "deny-warnings", deny(warnings))]
    |                                             ^^^^^^^^
    = note: #[deny(mutable_borrow_reservation_conflict)] implied by #[deny(warnings)]
    = warning: this borrowing pattern was not meant to be accepted, and may become a hard error in the future
    = note: for more information, see issue #59159 <https://github.com/rust-lang/rust/issues/59159>
error: aborting due to previous error
error: Could not compile `cargo`.
warning: build failed, waiting for other jobs to finish...
error: build failed

[alexcrichton]

I think that should be fixed on master now, but it's a good point about how we want to support this. I think maybe an "ideal" would be supporting symlinks within a package but erroring out on symlinks that point outside the package, but dealing with paths and symlinks is so fraught with bugs that I think I'd personally just prefer to have a first-class error for directory symlinks or symlinks in general.

[thomwiggers]

Making it a strict error would seriously complicate maintenance of packages such as the one I'm building here: https://github.com/rustpq/pqcrypto. This project will at some point have:

• pqcrypto-kyber512
• pqcrypto-kyber768
• pqcrypto-kyber1024
• pqcrypto-frodokem-....
• ...

This project will be importing a whole bunch of different C implementations of cryptographic libraries and generating Rust bindings to use them. The neat place to put the shared C code would be in the parent directory; having to mount the submodule or copy and commit the files in each separate subpackage would be a huge pain.

[alexcrichton]

I think if we want to support this we'll want to avoid symlinks in archives and just copy all the contents in, and if that works for your use case it seems like a reasonable way to implement this perhaps

[ehuss]

avoid symlinks in archives and just copy all the contents in

I believe that is how it works now (or with this patch when using git).

[thomwiggers]

avoid symlinks in archives and just copy all the contents in

I believe that is how it works now (or with this patch when using git).

Correct. In very weird edge cases where there are a lot of symlinks to the same files this might inflate packages, but on the other hand the compression algorithm should take care of that I guess.

[thomwiggers]

Rebased on master to re-trigger tests.

[thomwiggers]

Error on Windows:

failures:
---- package::package_symlink_to_submodule stdout ----
running `C:\projects\cargo\target\debug\cargo.exe package --no-verify -v`
thread 'package::package_symlink_to_submodule' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout
--- stderr
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
error: The directory name is invalid. (os error 267)
', tests\testsuite\support\mod.rs:842:13
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
failures:
    package::package_symlink_to_submodule
test result: FAILED. 1558 passed; 1 failed; 4 ignored; 0 measured; 0 filtered out

How did this not occur on Travis?

Ah, wait, there's another hidden "the directory name is invalid" error.

[thomwiggers]

I needed to put in some extra effort to fix some things in how the test supports treat symlinks, but I think this should be reasonable now.

Removing symlinks on Windows was broken.

[bors]

☔ The latest upstream changes (presumably #6900) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

I'm getting privilege errors for the below tests on Windows 10. I think it would be good to avoid that if the system isn't configured to handle symlinks on Windows.

    package::broken_symlink
    package::package_symlink_to_dir
    package::package_symlink_to_submodule

Perhaps add some kind of check if symlinks are supported on Windows, and skip the test if they are not?

Also, can you squash all the commits?

[thomwiggers]

Perhaps add some kind of check if symlinks are supported on Windows, and skip the test if they are not?

Unfortunately you need admin privileges on Windows to create them in the first place. It's not possible to really skip tests in Rust... so I can't really do detection (marking them 'passed' seems inaccurate).

I could perhaps mark them as #[ignored] on Windows.

[thomwiggers]

I have rebased and cleaned up the commit history. I did not fully squash it, as I wanted the history to reflect the different things this PR addresses:

• Add new tests that demonstrate that links to dirs can't be packaged
• Fixes those tests
• Enabled symlink tests to run on Windows
• Fix a bug in the handling of symlinks
• Add new test runner behaviour on Windows (it will ignore symlink-related tests and instead run them if you specify --ignored.)

[thomwiggers]

Appveyor appears to fail for completely unrelated reasons.

[bors]

☔ The latest upstream changes (presumably #7139) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #7137) made this pull request unmergeable. Please resolve the merge conflicts.

[thomwiggers]

Rebased, removed running --ignored tests, ran cargo fmt

[thomwiggers]

It appears rustfmt nightly and stable disagree...

[ehuss]

Yea, I was trying to figure out how to deal with the rustfmt changes, but I think I'm just gonna leave it until 1.38 hits stable. It is annoying.

Thanks for seeing this through!

@bors r+

[bors]

📌 Commit d0f7c0e has been approved by ehuss

[bors]

⌛ Testing commit d0f7c0e with merge 0237953...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 0237953 to master...