Merged
Conversation
tdejager
commented
Jul 4, 2025
| uv-platform-tags = { git = "https://github.com/astral-sh/uv", tag = "0.7.8" } | ||
| uv-pypi-types = { git = "https://github.com/astral-sh/uv", tag = "0.7.8" } | ||
| uv-requirements-txt = { git = "https://github.com/astral-sh/uv", tag = "0.7.8" } | ||
| uv-build-frontend = { git = "https://github.com/astral-sh/uv", tag = "0.7.19" } |
Comment on lines
+401
to
+402
| reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8" } | ||
| reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8" } |
Contributor
Author
There was a problem hiding this comment.
We need these patched versions, I took them from the uv repo on the designated tag.
| uv-installer = { workspace = true } | ||
| uv-normalize = { workspace = true } | ||
| uv-python = { workspace = true } | ||
| uv-redacted = { workspace = true } |
Contributor
Author
There was a problem hiding this comment.
This is the new url redaction stuff.
| let index = options | ||
| .index_url | ||
| .clone() | ||
| .map(DisplaySafeUrl::from) |
Contributor
Author
There was a problem hiding this comment.
Need to map into this from now on.
| ); | ||
|
|
||
| PinnedGitSpec::new(dist.git.repository().clone(), pinned_checkout) | ||
| PinnedGitSpec::new(dist.git.repository().clone().into(), pinned_checkout) |
Contributor
Author
There was a problem hiding this comment.
Again we need to go back to Url in this case..
| let expected_uv_req = RequirementSource::Git { | ||
| git: uv_git_types::GitUrl::from_fields( | ||
| Url::parse("ssh://git@github.com/user/test.git").unwrap(), | ||
| DisplaySafeUrl::parse("ssh://git@github.com/user/test.git").unwrap(), |
Contributor
Author
There was a problem hiding this comment.
New type here as well.
| // Before hitting the network let's make sure the credentials are available to uv | ||
| for url in self.index_locations.indexes().map(|index| index.url()) { | ||
| let success = store_credentials_from_url(url); | ||
| let success = store_credentials_from_url(url.url()); |
Contributor
Author
There was a problem hiding this comment.
This is an IndexUrl, which should still contain the credentials...
Comment on lines
+115
to
+121
| let url = url | ||
| .to_string() | ||
| .strip_prefix("git+") | ||
| .map(ToString::to_string) | ||
| .unwrap_or_else(|| url.to_string()) | ||
| .parse::<Url>() | ||
| .unwrap(); |
Contributor
Author
There was a problem hiding this comment.
To get the test to work..
Comment on lines
-251
to
+258
| if locked_git_url.url.repository() | ||
| != &installed_repository_url.into_url() | ||
| { | ||
| let locked_repository_url = | ||
| RepositoryUrl::new(locked_git_url.url.repository()); | ||
| if locked_repository_url != installed_repository_url { |
Contributor
Author
There was a problem hiding this comment.
This would now errorneously do a comparison with fragments, so the test failed, tried to get it to work here.
Comment on lines
+516
to
+525
| fn build_arena(&self) -> &BuildArena<Self::SourceDistBuilder> { | ||
| // Ensure the build dispatch is initialized | ||
| self.ensure_build_dispatch_initialized(); | ||
|
|
||
| // Get the inner build dispatch and delegate to its build_arena method | ||
| self.build_dispatch | ||
| .get() | ||
| .expect("build dispatch not initialized, this is a programming error") | ||
| .build_arena() | ||
| } |
Contributor
Author
There was a problem hiding this comment.
New type introduced by uv, we are just forwarding the implementation.
nichmor
reviewed
Jul 7, 2025
Contributor
nichmor
left a comment
nichmor
left a comment
There was a problem hiding this comment.
Looks good! some small comments
Co-authored-by: nichmor <nmorkotilo@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade to uv 0.7.19, so that we can prepare for a PR I hope to land in uv, because the upgrade always takes a while, I decide to start doing this now.
The main thing is, that uv now seems to strip the
git+prefix in some cases, so we had to start doing this as well. Please check this all very closely in the review please :) They've also added aDisplaySafeUrl, which is similar to Url redaction you've added @nichmor.