Skip to content

Conversation

@mkniewallner
Copy link
Contributor

@mkniewallner mkniewallner commented Jan 22, 2025

Summary

Closes #10597.

Check for invalid extras specified on the CLI, through either --extra <extra> or --all-extras --no-extra <extra>

If an usage of dynamic optional dependencies is detected, validation is not performed, as in this case, we cannot determine which extras exist, as this is delegated to the build backend.

This concretely means that we do not perform the validation if:

  • for workspace, at least one project uses dynamic optional dependencies
  • for project, it uses dynamic optional dependencies

Adding this check required to update quite a lot of things. I've tried to split the changes as much as possible, especially since there are some changes I'm not sure about, in terms of architecture.

First, to my knowledge, optional dependencies work similarly to dependency groups for the validation, so I thought it would be ok to make DependencyGroupsTarget more generic by renaming it to SpecificationTarget, so that the same logic can be reused both for dependency groups and optional dependencies, but maybe you have a different opinion.

I also needed to add dynamic field to Project, and a few methods to check the usage of a dynamic key in a project or workspace. Not sure if there are better ways or places to do that.

Test Plan

Snapshot tests.

@zanieb zanieb added this to the v0.6.0 milestone Jan 22, 2025
@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2025

cross-talk note from #10861 which is working on a similar thing:

The pip install and pip compile commands also take --extra, and with my PR will also take --dependency-group. However those commands have a quirky ability to accept multiple pyproject.tomls, which raises questions of "how should diagnostics even work for that".

(It's fine if this PR doesn't handle the pip commands, just doing these ones is an Improvement!)

@mkniewallner
Copy link
Contributor Author

cross-talk note from #10861 which is working on a similar thing:

The pip install and pip compile commands also take --extra, and with my PR will also take --dependency-group. However those commands have a quirky ability to accept multiple pyproject.tomls, which raises questions of "how should diagnostics even work for that".

(It's fine if this PR doesn't handle the pip commands, just doing these ones is an Improvement!)

Thanks for the heads up! Depending on how complex it is, it might be easier to implement as a follow-up PR yep, not sure, I'm not familiar with the pip interface.

@mkniewallner mkniewallner force-pushed the feat/error-on-non-existent-extra branch from 1ab569d to 9f1a4f0 Compare January 23, 2025 02:44
@mkniewallner mkniewallner force-pushed the feat/error-on-non-existent-extra branch from 9f1a4f0 to a10ba07 Compare January 23, 2025 03:12
@mkniewallner mkniewallner marked this pull request as ready for review January 23, 2025 03:34
// thing is that `x2` should _not_ activate the `idna==3.4` dependency
// in `proxy1`. The `--extra=x2` should be a no-op, since there is no
// `x2` extra in the top level `pyproject.toml`.
// Error out, as x2 extra is only on the child.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the test is still relevant as this now errors out, but since there's also a lock assertion right after, I've added a sync call without argument below, as I wasn't sure.

@zanieb zanieb self-assigned this Jan 23, 2025
@Gankra
Copy link
Contributor

Gankra commented Jan 23, 2025

For the record I landed my checking here:

// Complain if dependency groups are named that don't appear.
// This is only a warning because *technically* we support passing in
// multiple pyproject.tomls, but at this level of abstraction we can't see them all,
// so hard erroring on "no pyproject.toml mentions this" is a bit difficult.
if let Some(groups) = self.groups.groups() {
for name in groups.names() {
if !metadata.dependency_groups.contains_key(name) {
warn_user_once!(
"The dependency-group '{name}' is not defined in {}",
path.display()
);
}
}
}

My implementation is a bit flawed, notably because it runs "per pyproject.toml" and in concurrent async. Because this wasn't the primary focus of my PR I put in a hacky filter to leave better checking as future work:

uv_snapshot!(context.filters().into_iter().chain([(
"warning: The dependency-group 'lies' is not defined in pyproject.toml\nwarning: The dependency-group 'lies' is not defined in subdir/pyproject.toml",
"warning: The dependency-group 'lies' is not defined in subdir/pyproject.toml\nwarning: The dependency-group 'lies' is not defined in pyproject.toml",
)]).collect::<Vec<_>>(), context.pip_install()

The "better" implementation of this would shove a bunch of extra state in SourceTreeResolution and return it from SourceTreeResolver::resolve_source_tree, so that SourceTreeResolver::resolve could see the defined extras/dependency-groups of all pyprojects at once and produce deterministic output (and potentially error on totally undefined keys?). I don't have plans to implement that myself, at the moment.

@Gankra Gankra self-requested a review January 23, 2025 14:12
@charliermarsh
Copy link
Member

I think we should be doing this against the lockfile instead -- same for dependency groups, probably. Then it will work with --frozen, and we won't need any sort of special-casing for dynamic.

@mkniewallner
Copy link
Contributor Author

I think we should be doing this against the lockfile instead -- same for dependency groups, probably. Then it will work with --frozen, and we won't need any sort of special-casing for dynamic.

Tried to implement the validation at lock file level in #10925, but one case that cannot be handled is if we have an extra with no dependencies, like so:

[project.optional-dependencies]
http = []

as in that case, the empty list would not get added to the lock file, so this will be reported as not existing.

Don't know how ok this is, but if it is fine, we'll probably have to rephrase the errors to not confuse the users.

@charliermarsh
Copy link
Member

Ah dang... I did think about this before posting, and I thought we included these in project.requires-dist, but I guess that's only true for empty groups (which are included in project.requires-dev). Arguably we should start writing them to the lockfile as provides-extra?

@charliermarsh
Copy link
Member

@mkniewallner -- Do you want me to first PR a change to add provides-extras to the lockfile, so we can use that?

@mkniewallner
Copy link
Contributor Author

@mkniewallner -- Do you want me to first PR a change to add provides-extras to the lockfile, so we can use that?

Won't have time to do so right away, so feel free to :)

Just curious though, since this would be a new field, that would I believe only be present if we have project.optional-dependencies (even if empty), how will the retro-compatibility be handled for older versions?

I believe we won't be able to differentiate between:

  • previous uv versions that did not set the new lock file attribute, even if project.optional-dependencies is set
  • new uv versions that set the new attribute, but could not be present if there is no project.optional-dependencies (which seems to be the behaviour of requires-dev from what I see)

I believe since the check would now live after locking it might be fine, but even though, this means that people working on the same project would all have to update uv to the minimum version that sets the new attribute to avoid adding/removing it depending on the version used for locking?

But I guess that since the check would land in 0.6.0, this is less problematic than shipping this in a patch version though, as this could be advertised.

@charliermarsh
Copy link
Member

We would need to consider always-setting provides-extra, even if it's empty, so we can detect whether the information is available.

@Gankra
Copy link
Contributor

Gankra commented Jan 30, 2025

Depends on #11063

@charliermarsh charliermarsh mentioned this pull request Feb 8, 2025
@mkniewallner
Copy link
Contributor Author

Closing in favour of #10925.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking A breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

uv sync ignores non-existent extras given to --extra option

4 participants