Updates to path source walking. #8095

ehuss · 2020-04-11T17:48:52Z

This is a collection of loosely related changes to path source walking:

Add more context to error messages.
Allow package.exclude patterns to match directories. Previously, the walker would recurse into the directory, and skip every file. Instead, just skip the whole directory. This can be helpful if the directory is not readable, or otherwise want to avoid walking.
Don't require Cargo.toml to be in root of a git repo in order to use git to guide the selection. I'm not sure I understand the original reasoning that (any) Cargo.toml had to reside next to the .git directory.

The last is a moderately risky change, since it's hard to predict how this might affect more complex project layouts or new interactions with .gitignore that didn't exist before. Also, I'm wondering if it should just ignore if it fails to open the repo instead of emitting an error?

Closes #1729
Closes #6188
Closes #8092

rust-highfive · 2020-04-11T17:48:55Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton

Nice!

I left a comment about the change in behavior, but I think this should be fine to land myself.

alexcrichton · 2020-04-13T14:25:54Z

src/cargo/sources/path.rs

@@ -162,50 +175,59 @@ impl<'cfg> PathSource<'cfg> {
        &self,
        pkg: &Package,
        root: &Path,
-        filter: &mut dyn FnMut(&Path) -> CargoResult<bool>,
+        filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
    ) -> Option<CargoResult<Vec<PathBuf>>> {


FWIW I know you aren't responsible for this but this return type seems pretty wonky nowadays and it may be best to switch the option/result so ? and normal idiomatic error handling could be used here.

alexcrichton · 2020-04-13T14:29:30Z

src/cargo/sources/path.rs

-                        return Some(self.list_files_git(pkg, &repo, filter));
-                    }
-                }
+        let repo = match git2::Repository::discover(root) {


I feel like we historically actually did this but at some point backed away from it. That being said my mind here is quite fuzzy and I don't really remember the details (nor with some searching can I find anything).

The best I can remember is that folks sometimes have their whole $HOME dir as a git repo and they did or didn't want that messing with Cargo. That being said this here is only being used to list the files within a repo for a package we already know, which seems like it's basically just respecting .gitignore, right? If that's the case I can't really think right now how this can go astray...

🤷 Maybe play it safe and wait till next week to merge this so that it gets in the next release and gets the maximum amount of time on nightly?

ehuss · 2020-04-13T18:07:36Z

src/cargo/sources/path.rs

+        let repo_safe_path = repo_relative_path
+            .join("Cargo.toml")
+            .to_string_lossy()
+            .replace('\\', "/");


I have submitted rust-lang/git2-rs#549 as an upstream fix for this. I think it might be good to wait to merge that, then this can be removed.

@alexcrichton would you mind publishing a new git2 release, and then I can remove this?

er oops, done now!

alexcrichton · 2020-04-13T18:53:03Z

Ok sounds good to me to wait to merge this next week

bors · 2020-04-17T15:22:25Z

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

…walking.

alexcrichton · 2020-04-23T18:08:40Z

Looks like windows may have some different output orderings?

ehuss · 2020-04-24T18:13:59Z

Updated git2 to address the Windows issue.
@bors r=alexcrichton

bors · 2020-04-24T18:14:00Z

📌 Commit 25715e4 has been approved by alexcrichton

bors · 2020-04-24T18:35:30Z

⌛ Testing commit 25715e4 with merge ef3cbfc...

bors · 2020-04-24T18:58:22Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing ef3cbfc to master...

Update cargo 11 commits in 8751eb3010d4cdb5329b5a6bd2b6d765c95b0dca..90931d9b31e8b854522fed00916504a3ac6d8619 2020-04-21 18:04:35 +0000 to 2020-04-28 01:56:59 +0000 - Use associated constants directly on primitive types instead of modules (rust-lang/cargo#8077) - Clear `RUSTDOCFLAGS` before running tests (rust-lang/cargo#8168) - Fix warning for `resolve` mismatch in workspace. (rust-lang/cargo#8169) - Fix flaky linking_interrupted test. (rust-lang/cargo#8162) - Fixed some unnecessary borrows and clones. (rust-lang/cargo#8146) - Added warning when using restricted names in Windows. (rust-lang/cargo#8136) - Add changelog about dylib uplift. (rust-lang/cargo#8161) - Mention that cargo_metadata can parse json messages (rust-lang/cargo#8158) - Re-enable rustc-info-cache test again (rust-lang/cargo#8155) - Updates to path source walking. (rust-lang/cargo#8095) - Bump to 0.46.0, update changelog (rust-lang/cargo#8153)

Fix error with git repo discovery and symlinks. There are some cases where Cargo would generate an error when attempting to discover if a package is inside a git repo when the git repo has a symlink somewhere in its ancestor paths. One way this manifests is with `cargo install --git ...` where the given repo has a `build.rs` script. Another scenario is `cargo build --manifest-path somelink/Cargo.toml` where `somelink` is a symlink to the real thing. The issue is that libgit2 is normalizing paths and removing symlinks, but the path Cargo uses is the path with symlinks. This was introduced in #8095. The solution is to try to canonicalize both paths when trying to get a repo-relative path. If that fails for whatever reason, it shouldn't generate an error since this is just a "best effort" attempt to use git to list package files. Fixes #8183

rust-highfive assigned alexcrichton Apr 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2020

ehuss force-pushed the paths-walking branch from 3d97a60 to 3e7525b Compare April 11, 2020 18:54

alexcrichton reviewed Apr 13, 2020

View reviewed changes

ehuss commented Apr 13, 2020

View reviewed changes

ehuss added 4 commits April 22, 2020 11:21

Add some more context to errors walking path sources.

9ed56ca

Allow package.exclude patterns to match directories.

51e0c71

Don't require Cargo.toml to be in root of a git repo for path source …

1232ad3

…walking.

Swap Result/Option for discover_git_and_list_files.

3a5af29

ehuss force-pushed the paths-walking branch from 2e267a8 to 3a5af29 Compare April 22, 2020 18:33

ehuss mentioned this pull request Apr 23, 2020

Allow absolute paths for Repository::is_path_ignored. rust-lang/git2-rs#554

Merged

ehuss mentioned this pull request Apr 23, 2020

Fix more Windows paths. rust-lang/git2-rs#557

Merged

Bump git2.

25715e4

ehuss force-pushed the paths-walking branch from f8e199f to 25715e4 Compare April 24, 2020 16:58

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2020

bors merged commit ef3cbfc into rust-lang:master Apr 24, 2020

bors mentioned this pull request Apr 24, 2020

Added warning when using restricted names in Windows. #8136

Merged

ehuss mentioned this pull request Apr 28, 2020

Update cargo rust-lang/rust#71642

Merged

This was referenced Apr 30, 2020

latest cargo update broke cargo install command in my ci #8183

Closed

Fix error with git repo discovery and symlinks. #8186

Merged

ehuss mentioned this pull request Jun 7, 2021

Inconsistency with publishing/vendoring listing of files #9555

Open

DeadMozay mentioned this pull request Jul 14, 2021

Compilation error zstd-sys nushell/nushell#3772

Closed

ehuss added this to the 1.45.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to path source walking. #8095

Updates to path source walking. #8095

ehuss commented Apr 11, 2020

rust-highfive commented Apr 11, 2020

alexcrichton left a comment

alexcrichton Apr 13, 2020

ehuss Apr 13, 2020

alexcrichton Apr 13, 2020

ehuss Apr 13, 2020

ehuss Apr 13, 2020

ehuss Apr 22, 2020

alexcrichton Apr 22, 2020

alexcrichton commented Apr 13, 2020

bors commented Apr 17, 2020

alexcrichton commented Apr 23, 2020

ehuss commented Apr 24, 2020

bors commented Apr 24, 2020

bors commented Apr 24, 2020

bors commented Apr 24, 2020

Updates to path source walking. #8095

Updates to path source walking. #8095

Conversation

ehuss commented Apr 11, 2020

rust-highfive commented Apr 11, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Apr 13, 2020

Choose a reason for hiding this comment

ehuss Apr 13, 2020

Choose a reason for hiding this comment

alexcrichton Apr 13, 2020

Choose a reason for hiding this comment

ehuss Apr 13, 2020

Choose a reason for hiding this comment

ehuss Apr 13, 2020

Choose a reason for hiding this comment

ehuss Apr 22, 2020

Choose a reason for hiding this comment

alexcrichton Apr 22, 2020

Choose a reason for hiding this comment

alexcrichton commented Apr 13, 2020

bors commented Apr 17, 2020

alexcrichton commented Apr 23, 2020

ehuss commented Apr 24, 2020

bors commented Apr 24, 2020

bors commented Apr 24, 2020

bors commented Apr 24, 2020