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

feat(manager/uv): set registry URLs #31186

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

Conversation

mkniewallner
Copy link
Contributor

@mkniewallner mkniewallner commented Sep 3, 2024

Changes

Set registry URLs for uv manager based on index-url and extra-index-url. As noted by the documentation, extra-index-url have higher priority over index-url (whether we use the default one, or set it explicitly). Note that uv does not yet support mapping specific dependencies to specific indexes (this is tracked by astral-sh/uv#171).

There is also an index-strategy parameter to control a bit the behaviour when dealing with multiple indexes (for instance stop at first match for each registry), but since this happens after the dependency resolution, it's not something we can really act on I believe (or at least not at this level in the codebase).

This PR made me realise that the whole uv-specific configuration can also be set using uv.toml instead of pyproject.toml, but since it was left out in the initial PR implementing uv, I think it would be best to create a separate discussion/issue for that.

Context

Closes #31151.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@mkniewallner mkniewallner marked this pull request as ready for review September 3, 2024 20:33
@mkniewallner mkniewallner requested a review from rarkins September 4, 2024 07:44
Comment on lines +38 to +40
for (const dep of deps) {
dep.registryUrls = [...registryUrls];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally if registry URLs apply to the entire package file then we want them set once at the packageFile level instead duplicated into every dep.

Also, are these likely to all be pypi datasource now and forever? Or is it possible that there could be a git dependency mixed in there at one point (which would then make the registry URL invalid for that dep)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally if registry URLs apply to the entire package file then we want them set once at the packageFile level instead duplicated into every dep.

There is a plan to be able to map specific dependencies to specific sources in astral-sh/uv#171, similarly to what PDM does I believe. According to this comment it should be worked on soon enough. It doesn't mean that we can't implement what you're suggesting, which makes sense, but maybe since this is meant to evolve, setting registries at the dependency level is more conservative?

Also, are these likely to all be pypi datasource now and forever? Or is it possible that there could be a git dependency mixed in there at one point (which would then make the registry URL invalid for that dep)

That's a really good point, even today it's already possible to set other sources, like git, URL, path, or workspace dependencies. We should definitely skip those for now similarly to what we do in cargo, by checking if a source is defined for a dependency (and later on we could support updates for git dependencies for instance).

I can make a separate PR to handle skipping those dependencies, since I believe this is not directly related to what this PR implements, as today we already consider dependencies with different sources as public PyPI ones anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #31270 for the 2nd point, thanks for pointing that out, I should have been implemented from the start.

@rarkins rarkins added the auto:inactive-pr PR has become inactive and we want to prompt the submitter label Feb 7, 2025
Copy link
Contributor

github-actions bot commented Feb 7, 2025

Hi there,

This PR appears to have been inactive for a while. Please let us know if you are still working on it, or if we can close it for now.

Thanks, the Renovate team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:inactive-pr PR has become inactive and we want to prompt the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants