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

fixed git submodule bug when using relative paths #12156

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Choudhry18
Copy link
Contributor

Summary

This fixes installations using git which involved submodules declared through relative paths, fixes #9822 .

Test Plan

I did testing using a private gitlab server that I was running locally, to recreate the issue. I have also added tests that involve installing from a repository that I created on my GitHub. Adding that repository and it's accompanying submodule to uv-test (I see there is already a repository - uv_submodule_pypackage that tests submodule installation but that doesn't use relative path) should work.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@charliermarsh
Copy link
Member

This looks reasonable and I greatly appreciate the test. My only concern is that Cargo does it in the order we have here 🤔 Can you briefly check if there are any such issues reported in Cargo?

@charliermarsh
Copy link
Member

They do have one test here -- I wonder why it succeeds? https://github.com/rust-lang/cargo/blob/bc577dc6c4d65b6d023d08cae20f682a12d85c0c/tests/testsuite/git.rs#L926

@charliermarsh charliermarsh added the bug Something isn't working label Mar 13, 2025
@Choudhry18
Copy link
Contributor Author

They do have one test here -- I wonder why it succeeds? https://github.com/rust-lang/cargo/blob/bc577dc6c4d65b6d023d08cae20f682a12d85c0c/tests/testsuite/git.rs#L926

I looked into Cargo’s implementation and found that their tests do not fail due to differences in how submodules are updated.

Currently, uv relies on the Git CLI (git submodule update --recursive) to update submodules, whereas Cargo passes the remote URL to a submodule function that fetches it using either git_cli, gitoxide, or libgit2. This means that when git reset breaks the link to the remote origin, Cargo’s implementation remains unaffected.

One key reason Cargo seems to run reset first is their guard mechanism, which helps mitigate interruptions while updating submodules and uv's guard mechanism is not affected by this change.

I did think of scenarios where updating submodules before a reset could cause issues—for example, in a local repository with uncommitted changes affecting submodules. To address this, I implemented the fix so that reset is performed before updating submodules.

@Choudhry18
Copy link
Contributor Author

Apologies for the ping, @charliermarsh . I just wanted to check in to see if you had any questions about the implementation or if there's anything on my end that might be holding up the review of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git submodule cloning not working when using relative paths
2 participants