Skip to content

fix: reduce failing to open a git repo to a tracing warning #13498

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

Closed
wants to merge 1 commit into from

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Feb 27, 2024

When listing files in a path source, Cargo attempts to open the path source as a git repository with a fallback to walking the filesystem if path source isn't a git repository or fails for various reasons.

What does this PR try to resolve?

If the path source is part of a git repository, but that git repository uses features that cargo (via libgit2) doesn't understand, Cargo will error out. This PR changes the error to tracing warning, similar to other failures on this path.

Fixes #10150

How should we test and review this PR?

Manually validated on the repro repo linked in the issue.

@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2024
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Would there be a possilbility that

  • people accidentally set split-index, and then
  • cargo publish went with heuristic file inclusion rule instead of git tree, and
  • unexpected files got uploaded?

@weihanglo
Copy link
Member

I might be overthinking though

@arlosi
Copy link
Contributor Author

arlosi commented Feb 27, 2024

Would there be a possilbility that

  • people accidentally set split-index, and then
  • cargo publish went with heuristic file inclusion rule instead of git tree, and
  • unexpected files got uploaded?

This is a possibility, but I'd expect it to be rare.

The current situation is that it's not possible to build a crate with a build.rs that's contained anywhere in a git repo that uses an unsupported git feature. For example, if I vendor a dependency in a repo that's using split index, then the dependency can no longer build in the checked-in vendor directory.

One alternative fix would be to only attempt to open the git repo if we detect that the Cargo.toml is next to the .git directory. Based on this doc comment that would be correct.

    /// Returns `Some(git2::Repository)` if found sibling `Cargo.toml` and `.git`
    /// directory; otherwise, caller should fall back on full file list.

The comment may be out of date based on the implementation though, which appears to support Cargo.toml nested within the git repo.

@weihanglo
Copy link
Member

The doc explicitly states that Cargo respects gitignore files when packaging

If include is not specified, then the following files will be excluded:

  • If the package is not in a git repository, all “hidden” files starting with a dot will be skipped.
  • If the package is in a git repository, any files that are ignored by the gitignore rules of the repository and global git configuration will be skipped.

I have no idea how strong Cargo follows those rules. Some people do rely on the heuristics.

An alternative might be #9931, which disables the heuristics and sets rerun-if-changed=build.rs by default on an edition boundary. We do have this item in Cargo editio 2024 planning 😆.

Should we do a quick poll and see if people accept this seemly low risk?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo build crash in project with git Split Index
4 participants