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

Add tests for ignoring symlinks #10047

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Nov 5, 2021

This adds tests for the expected behavior in #10032. Interestingly, these tests pass (🎉). Will update that issue with more details shortly, but figured these tests were worthwhile to add to the testsuite anyway now that I've written them.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2021
@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 5, 2021

The Windows failure here is surprising. It suggests that on Windows, Cargo considers the symlinks files, not directories, and thus they don't match /symlink/. Probably around here

let is_dir = path.is_dir();
if !is_root && !(*filter)(path, is_dir)? {

Even though the docs for Path::is_dir pretty clearly state

This function will traverse symbolic links to query information about the destination file.

🤔

Given that I don't have a Windows computer handy to debug with, I think this has me stomped for now.

I'll just add that the fix I had originally intended for #10032 is

        let is_dir = path.is_dir();

        // Symlinks that point to directories are a little odd because they are both directories
        // _and_ files for the purposes of matching. At least as far as Cargo is concerned, since
        // we never include symlinks directly in the pacakge. The actual rules are:
        //
        //  1. A pattern of `/symlink` should match the symlink itself.
        //  2. A pattern of `/symlink/` should match the symlink's target's contents.
        //
        // But for us, this means that if a directory symlink is excluded as either a directory
        // _or_ a file, it should be skipped.
        if !is_root && is_dir {
            let is_symlink = path.symlink_metadata()?.file_type().is_symlink();
            // See if the symlink is filtered out when viewed as a file -- if so, skip it.
            // The directory case is handled by the regular logic below.
            if is_symlink && !(*filter)(path, false)? {
                return Ok(());
            }
        }

        if !is_root && !(*filter)(path, is_dir)? {

which may come in handy for whoever decides to try tackling this.

@epage epage added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2022
@ehuss
Copy link
Contributor

ehuss commented Mar 22, 2022

I believe the issue here is with libgit2. When creating the repository, the add_all function ends up adding the symbolic links to the repo. There is something about libgit2's gitignore implementation that is not honoring symlinks as directories. I have opened libgit2/libgit2#6250 with a reproduction.

If you want to just disable these tests on Windows, I think it would be fine.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 22, 2022

Pushed a change that ignores the test on Windows 👍

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2022

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 23, 2022

📌 Commit 79cc65f has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 23, 2022
@bors
Copy link
Collaborator

bors commented Mar 23, 2022

⌛ Testing commit 79cc65f with merge e458f6d...

@bors
Copy link
Collaborator

bors commented Mar 23, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing e458f6d to master...

@bors bors merged commit e458f6d into rust-lang:master Mar 23, 2022
@jonhoo jonhoo deleted the ignore-symlink-dir branch March 23, 2022 22:19
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2022
Update cargo

13 commits in 109bfbd055325ef87a6e7f63d67da7e838f8300b..1ef1e0a12723ce9548d7da2b63119de9002bead8
2022-03-17 21:43:09 +0000 to 2022-03-31 00:17:18 +0000
- Support `-Zmultitarget` in cargo config (rust-lang/cargo#10473)
- doc: Fix document url for libcurl format (rust-lang/cargo#10515)
- Fix wrong info in "Environment variables" docs (rust-lang/cargo#10513)
- Use the correct flag in --locked --offline error message (rust-lang/cargo#10512)
- Don't treat host/target duplicates as duplicates (rust-lang/cargo#10466)
- Unstable --keep-going flag (rust-lang/cargo#10383)
- Part 1 of RFC2906 - Packages can inherit fields from their root workspace (rust-lang/cargo#10497)
- Remove unused profile support for -Zpanic-abort-tests (rust-lang/cargo#10495)
- HTTP registry implementation (rust-lang/cargo#10470)
- Add a notice about review capacity. (rust-lang/cargo#10501)
- Add tests for ignoring symlinks (rust-lang/cargo#10047)
- Update doc string for deps_of/compute_deps. (rust-lang/cargo#10494)
- Consistently use crate::display_error on errors during drain (rust-lang/cargo#10394)
@ehuss ehuss added this to the 1.61.0 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants