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

Report incompatible distributions to users #1293

Merged
merged 10 commits into from
Feb 15, 2024
Merged

Report incompatible distributions to users #1293

merged 10 commits into from
Feb 15, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Feb 13, 2024

Instead of dropping versions without a compatible distribution, we track them as incompatibilities in the solver. This implementation follows patterns established in #1290.

This required some significant refactoring of how we track incompatible distributions. Notably:

  • Option<TagPriority> is now WheelCompatibility which allows us to track the reason a wheel is incompatible instead of just None.
  • Candidate now has a CandidateDist with Compatible and Incompatibile variants instead of just ResolvableDist; candidates are not strictly compatible anymore
  • ResolvableDist was renamed to CompatibleDist
  • IncompatibleWheel was given an ordering implementation so we can track the "most compatible" (but still incompatible) wheel. This allows us to collapse the reason a version cannot be used to a single incompatibility.
  • The filtering in the VersionMap is retained, we still only store one incompatible wheel per version. This is sufficient for error reporting.
  • A TagCompatibility type was added for tracking which part of a wheel tag is incompatible
  • Candidate::validate_python moved to PythonRequirement::validate_dist

I am doing more refactoring in #1298 — I think a couple passes will be necessary to clarify the relationships of these types.

Includes improved error message snapshots for multiple incompatible Python tag types from #1285 — we should add more scenarios for coverage of behavior when multiple tags with different levels are present.

@zanieb zanieb force-pushed the zb/no-wheel-platform branch from e323b62 to ce013a8 Compare February 13, 2024 03:12
@zanieb zanieb changed the title zb/no wheel platform Report incompatible distributions to users Feb 13, 2024
Base automatically changed from zb/refactor-dist to main February 13, 2024 04:19
@zanieb zanieb force-pushed the zb/no-wheel-platform branch from ce013a8 to 1933d60 Compare February 13, 2024 04:21
@zanieb zanieb added the error messages Messaging when something goes wrong label Feb 13, 2024
@zanieb zanieb force-pushed the zb/no-wheel-platform branch from 90afb5e to b5da4ff Compare February 14, 2024 18:54
…1299)

A smaller alternative to #1298
patching the bad behavior in #1293
@zanieb zanieb force-pushed the zb/no-wheel-platform branch from 77cba18 to 48a5401 Compare February 15, 2024 15:38
Comment on lines -7 to -17
/// A [`Dist`] and metadata about it required for downstream filtering.
#[derive(Debug, Clone)]
pub struct DistMetadata {
pub dist: Dist,

/// The version of Python required by the distribution
pub requires_python: Option<VersionSpecifiers>,

/// Is the distribution file yanked?
pub yanked: Yanked,
}
Copy link
Member Author

@zanieb zanieb Feb 15, 2024

Choose a reason for hiding this comment

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

Moved down; will be removed entirely in #1298

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

This is such an amazing improvement to the error messages. What a win!

}

/// Set the `exclude_newer` flag
pub fn set_exclude_newer(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

I might accept a yes: bool parameter here. Or rename to enable_exclude_newer. (I usually like the former since it's a little more flexible.)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 it's going away in the next pull request and seems fine so I'll leave it for now but keep in mind for the future

(Self::Incompatible(t_self), Self::Incompatible(t_other)) => t_self.cmp(t_other),
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I looked at this a bit to see if I could find a way that it breaks the contract for Ord, but nothing came to mind. :)

@zanieb zanieb merged commit e9e3e57 into main Feb 15, 2024
7 checks passed
@zanieb zanieb deleted the zb/no-wheel-platform branch February 15, 2024 16:48
zanieb added a commit that referenced this pull request Mar 8, 2024
Extends the "compatibility" types introduced in #1293 to apply to source
distributions as well as wheels.

- We now track the most-relevant incompatible source distribution
- Exclude newer, Python requirements, and yanked versions are all
tracked as incompatibilities in the new model (this lets us remove
`DistMetadata`!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Messaging when something goes wrong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants