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

Use git command to fetch repositories instead of libgit2 for robust SSH support #1781

Merged
merged 18 commits into from
Feb 21, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Feb 20, 2024

Closes #1775
Closes #1452
Closes #1514
Follows #1717

libgit2 does not support host names with extra identifiers during SSH lookup (e.g. github.com-some_identifier) so we use the git command instead for fetching. This is required for pip parity.

See the Cargo documentation for more details on using the git CLI instead of libgit2. We may want to try to use libgit2 first in the future, as it is more performant (#1786).

We now support authentication with:

git+ssh://git@<hostname>/...
git+ssh://git@<hostname>-<identifier>/...

Tested with a deploy key e.g.

cargo run -- \
    pip install uv-private-pypackage@git+ssh://git@github.com-test-uv-private-pypackage/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v

and

cargo run -- \
    pip install uv-private-pypackage@git+ssh://git@github.com/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v     

with a ssh config like

Host github.com
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519

Host github.com-test-uv-private-pypackage
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519

It seems quite hard to add test coverage for this to the test suite, as we'd need to add the SSH key and I don't know how to isolate that from affecting other developer's machines.

crates/uv-git/src/git.rs Outdated Show resolved Hide resolved
crates/uv-git/src/git.rs Outdated Show resolved Hide resolved
crates/uv-git/src/git.rs Outdated Show resolved Hide resolved
@zanieb zanieb marked this pull request as ready for review February 21, 2024 00:02
Base automatically changed from zb/git-auth-pat to main February 21, 2024 00:12
@zanieb zanieb added the bug Something isn't working label Feb 21, 2024
crates/uv-git/src/git.rs Outdated Show resolved Hide resolved
crates/uv/tests/common/mod.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Member

Do we have any benchmarks that use git dependencies? Does it change the performance in significant ways?

@zanieb
Copy link
Member Author

zanieb commented Feb 21, 2024

Do we have any benchmarks that use git dependencies? Does it change the performance in significant ways?

I don't think we do. Honestly I'm not sure it makes sense to take the time to measure right now because we need to support authentication. I think benchmarking should be done when considering #1786 instead.

@zanieb zanieb merged commit 71ec568 into main Feb 21, 2024
7 checks passed
@zanieb zanieb deleted the zb/git-auth-ssh branch February 21, 2024 18:44
zanieb added a commit that referenced this pull request Feb 22, 2024
Follow-up to #1781 improving the error message when a ref cannot be
fetched
ibraheemdev added a commit that referenced this pull request May 30, 2024
## Summary

We currently rely on libgit2 for most git-related functionality.
However, libgit2 has long-standing performance issues, as well as lags
significantly behind git in terms of new features. For these reasons we
now use the git CLI by default for fetching repositories
(#1781). This PR completely drops
libgit2 in favor of the git CLI for all git-related functionality, which
should allow us to use features such as partial clones and sparse
checkouts in the future for performance.

There is also a lot of technical debt in the current git code as it's
mostly taken from Cargo. Switching to the git CLI *vastly* simplifies
the `uv-git` codebase.

Eventually we might want to look into switching to
[`gitoxide`](https://github.com/Byron/gitoxide), but it's currently too
immature for our use case.
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
5 participants