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

universal-lock: add marker expressions to PubGrubPackage #3359

Closed
Tracked by #3350
BurntSushi opened this issue May 3, 2024 · 0 comments · Fixed by #3673
Closed
Tracked by #3350

universal-lock: add marker expressions to PubGrubPackage #3359

BurntSushi opened this issue May 3, 2024 · 0 comments · Fixed by #3673
Labels
preview Experimental behavior

Comments

@BurntSushi
Copy link
Member

As part of #3350, we'll need to distinguish conflicting dependencies among forks. We can achieve this by adding the relevant marker expressions to PubGrubPackage:

/// A Python package.
Package(
PackageName,
Option<ExtraName>,
/// The URL of the package, if it was specified in the requirement.
///
/// There are a few challenges that come with URL-based packages, and how they map to
/// PubGrub.
///
/// If the user declares a direct URL dependency, and then a transitive dependency
/// appears for the same package, we need to ensure that the direct URL dependency can
/// "satisfy" that requirement. So if the user declares a URL dependency on Werkzeug, and a
/// registry dependency on Flask, we need to ensure that Flask's dependency on Werkzeug
/// is resolved by the URL dependency. This means: (1) we need to avoid adding a second
/// Werkzeug variant from PyPI; and (2) we need to error if the Werkzeug version requested
/// by Flask doesn't match that of the URL dependency.
///
/// Additionally, we need to ensure that we disallow multiple versions of the same package,
/// even if requested from different URLs.
///
/// To enforce this requirement, we require that all possible URL dependencies are
/// defined upfront, as `requirements.txt` or `constraints.txt` or similar. Otherwise,
/// solving these graphs becomes far more complicated -- and the "right" behavior isn't
/// even clear. For example, imagine that you define a direct dependency on Werkzeug, and
/// then one of your other direct dependencies declares a dependency on Werkzeug at some
/// URL. Which is correct? By requiring direct dependencies, the semantics are at least
/// clear.
///
/// With the list of known URLs available upfront, we then only need to do two things:
///
/// 1. When iterating over the dependencies for a single package, ensure that we respect
/// URL variants over registry variants, if the package declares a dependency on both
/// `Werkzeug==2.0.0` _and_ `Werkzeug @ https://...` , which is strange but possible.
/// This is enforced by [`crate::pubgrub::dependencies::PubGrubDependencies`].
/// 2. Reject any URL dependencies that aren't known ahead-of-time.
///
/// Eventually, we could relax this constraint, in favor of something more lenient, e.g., if
/// we're going to have a dependency that's provided as a URL, we _need_ to visit the URL
/// version before the registry version. So we could just error if we visit a URL variant
/// _after_ a registry variant.
Option<VerbatimUrl>,
),

I did this in my prototype by adding them to PubGrubPackage::Package:

Package {
name: PackageName,
extra: Option<ExtraName>,
marker: Option<MarkerTree>,

But I think it might be worth experimenting with creating a new variant. Note that like extras, a package with a non-None marker expression should introduce a dependency on the package with the same name, but without the marker expressions:

// If a package has an extra, insert a constraint on the base package.
if extra.is_some() || marker.is_some() {
constraints.push(
PubGrubPackage::Package {
name: package_name.clone(),
extra: None,
marker: None,
url: url.clone(),
},
Range::singleton(version.clone()),
None,
);
}

This adds the necessary constraints to ensure pubgrub's resolution is itself properly constrained.

@BurntSushi BurntSushi added the preview Experimental behavior label May 3, 2024
BurntSushi added a commit that referenced this issue May 20, 2024
This just adds the field to the type and always sets it to `None`. There
are semantic changes in this commit.

Closes #3359
BurntSushi added a commit that referenced this issue May 20, 2024
This just adds the field to the type and always sets it to `None`. There
are semantic changes in this commit.

Closes #3359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Experimental behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant