Support using --index to refer to index names#17455
Conversation
747f88f to
6355b20
Compare
crates/uv/tests/it/edit.rs
Outdated
| uv_snapshot!(filters, context.add().arg("iniconfig").arg("--index").arg("test-index").current_dir(&child_dir), @r" | ||
| success: false | ||
| exit_code: 2 | ||
| ----- stdout ----- | ||
|
|
||
| ----- stderr ----- | ||
| error: Request failed after 3 retries | ||
| Caused by: Failed to fetch: `http://[LOCALHOST]/iniconfig/` | ||
| Caused by: HTTP status server error (503 Service Unavailable) for url (http://[LOCALHOST]/iniconfig/) | ||
| "); | ||
|
|
||
| //let lock = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; | ||
| //insta::with_settings!({ | ||
| // filters => filters, | ||
| //}, { | ||
| // assert_snapshot!(lock, @r#" | ||
| //"#); | ||
| //}); |
There was a problem hiding this comment.
Need to investigate what happens if you don't pass --index.
There was a problem hiding this comment.
So, I am going to leave this as is for now.
Although I think there's an argument to be made that if you're adding an index and it matches identically an index in the workspace (regardless of if you used the name, or spelled it out) that maybe we shouldn't mirror it in the child.
But the way things like #17610 work seems like maybe justification for leaving it as is.
There was a problem hiding this comment.
Can you remove the commented out code, and/or leave a TODO comment?
| [[tool.uv.index]] | ||
| name = "test-index" | ||
| url = "https://pypi-proxy.fly.dev/simple" |
There was a problem hiding this comment.
I am not sure if we want this. But also, need to check what happens normally.
There was a problem hiding this comment.
Okay, so index name references in [tool.uv.sources] are resolved in "project" then "workspace" order.
But when it comes to picking an index to use for a package without a specified source, we pick "workspace" then "project".
I'm not really sure what the correct answer is here. I'm going to leave it as is for now...
There was a problem hiding this comment.
I'd prefer workspace root only, to avoid the problem of workspace root and member declaration getting out of sync.
4c9003d to
a4070c9
Compare
a4070c9 to
d5df9b1
Compare
crates/uv/tests/it/edit.rs
Outdated
| let filters: Vec<_> = context | ||
| .filters() | ||
| .into_iter() | ||
| .chain([(r"http://127\.0\.0\.1:\d+", "[PARENT_INDEX]")]) |
There was a problem hiding this comment.
nit: regex::escape the mock server URL - though that filter doesn't seem to be used?
crates/uv/tests/it/edit.rs
Outdated
| uv_snapshot!(filters, context.add().arg("iniconfig").arg("--index").arg("test-index").current_dir(&child_dir), @r" | ||
| success: false | ||
| exit_code: 2 | ||
| ----- stdout ----- | ||
|
|
||
| ----- stderr ----- | ||
| error: Request failed after 3 retries | ||
| Caused by: Failed to fetch: `http://[LOCALHOST]/iniconfig/` | ||
| Caused by: HTTP status server error (503 Service Unavailable) for url (http://[LOCALHOST]/iniconfig/) | ||
| "); | ||
|
|
||
| //let lock = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; | ||
| //insta::with_settings!({ | ||
| // filters => filters, | ||
| //}, { | ||
| // assert_snapshot!(lock, @r#" | ||
| //"#); | ||
| //}); |
There was a problem hiding this comment.
Can you remove the commented out code, and/or leave a TODO comment?
This seems problematic? |
Is it necessary for it to be copied into the child package? Or if you added it to the child package without doing so would the parent index still be used properly? |
So |
This doesn't cause any issues at the moment but we could de-duplicate things such that only the first index for each unique URL is kept in the order they would have been kept before. I don't think the name is itself used for anything? That being said, this seems like an issue with how I'm happy to fix it though, do you want me to do it as part of this PR?
It's definitely an option not to add it to the child package. The right index will be picked up as the name resolution order is project then workspace, so if the project doesn't have an index with that name, the workspace one will be picked. But there is this issue: #17610, which is somewhat related. I feel that if we were to fix this, we should fix this in a way which is agnostic to whether the index was mentioned by name, or fully written out with a conflicting name. Although we can also just mark the index as having originated by name and then not write it back in those situations. But we have to be careful as you can specify a
No, |
|
@EliteTK can we ship this under a preview flag so we don't need to wait until 0.10 to merge? |
I don't understand how to reconcile this comment with your original one? |
crates/uv-cli/src/options.rs
Outdated
| /// Resolve [`IndexArg`]s into [`Index`]es using indexes defined on the | ||
| /// filesystem and combine the `default_index` and `index` into one vector. |
There was a problem hiding this comment.
I think this could be a bit less descriptive of the code and more of the intent? LIke
| /// Resolve [`IndexArg`]s into [`Index`]es using indexes defined on the | |
| /// filesystem and combine the `default_index` and `index` into one vector. | |
| /// Resolve index options into a prioritized list of [`Index`]. | |
| /// | |
| /// Index name references are resolved to index URLs using values from the file system. | |
| /// | |
| /// The default index is placed after all other indexes. |
| Err(error) => { | ||
| eprintln!("error: {error}"); | ||
| std::process::exit(2); | ||
| } |
There was a problem hiding this comment.
Do you know why we do this in the other code? It's pretty janky. It doesn't respect our Printer, it doesn't print the error chain, etc.
Yeah, no objections here.
I will adjust it for clarity, but the section you referenced mentions that this PR uses the "second strategy" which refers to: "On the other hand, when resolving which index to use for a package which has no specified index, the resolution takes the workspace index first and then the package's indexes next." |
| pub struct ResolveIndexArgError(IndexName); | ||
|
|
||
| impl IndexArg { | ||
| /// Parse an Index passed on the command line |
There was a problem hiding this comment.
| /// Parse an Index passed on the command line | |
| /// Parse an index passed on the command line |
| } | ||
| } | ||
|
|
||
| pub fn try_resolve<I>(self, indexes: I) -> Result<Index, ResolveIndexArgError> |
There was a problem hiding this comment.
This could probably use a doc comment
| }) | ||
| } | ||
|
|
||
| pub fn indexes(&self) -> impl Iterator<Item = Index> { |
There was a problem hiding this comment.
This should probably have a doc comment
| self.index | ||
| .iter() | ||
| .flatten() | ||
| .cloned() |
There was a problem hiding this comment.
Do we need to clone? Can the caller clone?
| ----- stderr ----- | ||
| warning: Relative paths passed to `--index` or `--default-index` should be disambiguated from index names (use `[PREFIX]test-index`). Support for ambiguous values will be removed in the future | ||
| error: Directory not found for index: file://[TEMP_DIR]/test-index | ||
| error: Could not find an index named `test-index` |
There was a problem hiding this comment.
We might want a follow-up issue to enumerate searched source files here.
| warning: Relative paths passed to `--index` or `--default-index` should be disambiguated from index names (use `[PREFIX]test-index`). Support for ambiguous values will be removed in the future | ||
| error: Directory not found for index: file://[TEMP_DIR]/test-index |
There was a problem hiding this comment.
Have we considered continuing to support the current behavior if ./test-index exists and / or if the name test-index doesn't exist?
There was a problem hiding this comment.
I hadn't, but I would say it should be only if the directory exists and the index doesn't. Otherwise you're going to get a confusing error whenever you mis-type an index or mis-type a directory name.
But honestly I am mildly against this kind of special casing.
Rather simple to implement though.
There was a problem hiding this comment.
I think such special casing generally makes transitioning easier because then the change is breaking for less people? I don't think it's an ideal end state, but the mistake was when we allowed foo to be treated as a relative path without ./foo in the first place.
|
This will need some documentation updates, which might help explain the behavior? The code looks fine but I only sort of understand how behaves, tbh. |
d5df9b1 to
7c062e1
Compare
7c062e1 to
56472a1
Compare
It's only used by the cli parsing, and involves a silly destructuring/restructuring dance to set the `default` field. Making it a function also avoids needing to explicitly set the origin.
Original code by Zanie with the temporary jury-rigging done by Tom.
56472a1 to
492db42
Compare
These aren't used since IndexArg must only originate from the CLI.
We _could_ print the index name before resolution but the goal here isn't to verify the integrity of the resolve function (to ensure that it did actually do the equality comparison) but rather just to troubleshoot which index it resolved to (in which case the name is embedded in the final index).
06d4b10 to
ce7e763
Compare
Summary
Implement #13974.
Some of the code was inspired by existing code from
uv_distribution::metadata::loweringwhich handles resolving index name references withinpyproject.toml.This PR adds a new
IndexArgtype touv_distribution_typeswhich holds information about an index as passed on the CLI. Specifically the currentIndextype has a non-optionalurlfield.IndexArgsolves this by including two variants, one for theIndextype and one for anUnresolvedIndexwhich holds theIndexName. This means that all the CLI handling can useIndexArgand then the standardresolvefunctions can transform theseIndexArgs intoIndexes. This allows you to reference indexes by name if they are already defined somewhere else.Currently support for
--default-indexby name is not included.Error handling
The initial code made use of
Resultto propagate errors regarding resolution (i.e. no index found for that name). But I added a commit which replaces this with an earlyexit(2)as that seems to be the style used by otherresolvestyle code.The new
ResolvetraitPipOptionsrequired a bunch of changes as it was using theFromtrait initially but index resolution requires additional metadata for the names which could have been passed as a tuple except for the fact that it's ugly and also breaks the orphan rule so it wasn't actually an option. Another alternative would be to do the resolution earlier but that seemed like an excessive amount of boilerplate, so instead a new trait was used.Argument cross-references
You can't resolve an index which you passed on the CLI (e.g.
--index foo=bar --index foo) as I felt that didn't make sense as a feature even if it could have been supported. Maybe there is a use for it though? Not sure.To Do
pyproject.tomlin workspaces. I haven't finished figuring out the best way to get this working yet.Workspaceorigin will be used here and indexes withProjectorWorkspaceorigin won't get written back.uv toolPassing
--indextouv tooleven if it's the only index doesn't guarantee that it will use only that index for the tool. Moreover, due to the fact that--index <name>works identically to--index <name>=..., the side effect is that the index will appear twice in theuv-receipt.toml. I think that any changes in this area are probably outside of the scope of this change.Test Plan
Tests currently need updating and changing to deal with the other changes to this PR.