Generic reason for custom incompatibility #208

konstin · 2024-04-15T13:15:57Z

Restructure the incompatibility into pubgrub internal metadata and a custom external type.

The internal types are:

not root: The root incompatibility that drives resolution
no versions: We tried all versions and there is none remaining
from dependency of: {A, not B} from A -> B
derived from: {A, not C} from {A, not B} and {B, not C}
custom: Any incompatibility that originates outside pubgrub. The idea is that users can set M to a custom enum of theirs. Examples:
- The version would require building the package, but builds are disabled.
- The package is not available in the cache, but internet access has been disabled.
- The package has invalid metadata

I'm currently working on adapting the formatting to adapt to the uv of this. This should reduce our diff significantly.

This refactoring is relevant to collapsing custom incompatibilities, but i haven't looked into this properly yet.

Co-authored-by: Zanie Blue <[email protected]>

zanieb · 2024-04-15T14:42:32Z

src/internal/incompatibility.rs

+    /// The package is unavailable for reasons outside pubgrub.
+    ///
+    /// Examples:
+    /// * The version would require building the package, but builds are disabled.
+    /// * The package is not available in the cache, but internet access has been disabled.
+    Custom(P, VS, M),


Should we just call this Unavailable or Unusable? We talked about this a little back in #153

I'm trying to name this something that says it's outside pubgrub, unavailable/unusable sounds too much like it could come from the resolution itself

I wonder if External is overloaded.

There's the External type in derivation tree. It's more non-derived than external, but it would be an invasive change

Yeah I agree that's my concern about it being overloaded 🤷‍♀️ I find it confusing there honestly but curious to see what Jacob thinks of all this.

I came in expecting to want Unusable, but the discussion has made me less certain. @mpizenberg is the one with a knack for naming things.

I generally like this change. I will have a bit of time to review code/naming after May (I have a conf talk next week and changing job end of this month). Otherwise consider I'm ok with it and might (or not) suggest a renaming PR later.

We could merge away external and use that name here instead: dev...astral-sh:pubgrub:konsti/dev/merge-external

Eh2406 · 2024-04-16T16:59:37Z

Sorry for the delay. I just had a chance to look at the diff, and I absolutely love it. Thank you so much! I need to poke at this in an editor before I write up the few nits I have.

Eh2406 · 2024-04-16T17:03:39Z

src/internal/incompatibility.rs

+        let self_metadata = self.metadata();
+        if self_metadata != other.metadata() {
+            return None;
+        }


I don't think this code path is needed at the moment, because we can only get here if as_dependency is Some and dependencies can't hold metadata. But perhaps they should be able to hold metadata, like what file and line number this dependency edge came from.

Thoughts?

Removed it unless @zanieb wants to keep this

We actually were just talking about needing that... but it can wait :D

konstin · 2024-04-17T08:09:38Z

src/internal/incompatibility.rs

-    /// because its list of dependencies is unavailable.
-    pub fn unavailable_dependencies(package: P, version: VS::V) -> Self {
+    /// Create an incompatibility for a reason outside pubgrub.
+    #[allow(dead_code)] // Used by uv


We can also drop this here and only use it in our fork

konstin · 2024-04-17T08:33:43Z

Should we create a trait for M: Eq + Clone + Debug + Display?

Eh2406 · 2024-04-27T02:22:15Z

Yes. I think we should have a trait. Although it could be in a follow-up PR. It should, one day, have a method can_merge(&Self, &Self) -> Option<Self> instead of the Eq bound. None would mark that the two incompatibilities should not be merged. And Some(M) would provide the new metadata for the merged incompatibility. That would allow a error like "unavailable because of HTTP 404, with request ID 123456789" to be merged with "unavailable because of HTTP 404, with request ID 987654321" even though they are not structurally equal.

Eh2406

I poked at this in an editor, and rustc told me why everything was there.
Adding a trait and renaming the type can be done in follow-up PR's.

Pubgrub got a new feature where all unavailability is a custom, instead of the reasonless `UnavailableDependencies` and our custom `String` type previously (pubgrub-rs/pubgrub#208). This PR introduces a `UnavailableReason` that tracks either an entire version being unusable, or a specific version. The error messages now also track this difference properly. The pubgrub commit is our main rebased onto the merged pubgrub-rs/pubgrub#208, i'll push `konsti/main-rebase-generic-reason` to `main` after checking for rebase problems.

feat: Generic reason for custom incompatibility

8838dca

Co-authored-by: Zanie Blue <[email protected]>

konstin force-pushed the konsti/dev/custom-incompatibility-metadata branch from fe96e23 to 8838dca Compare April 15, 2024 13:16

zanieb reviewed Apr 15, 2024

View reviewed changes

konstin added 2 commits April 15, 2024 17:55

Pacify clippy

faa3fe7

Document the intended usage of DependencyProvider::M

4dd3484

Eh2406 reviewed Apr 16, 2024

View reviewed changes

konstin commented Apr 17, 2024

View reviewed changes

konstin added 2 commits April 17, 2024 10:26

Remove metadata merging check

10cf15c

Improve docs

8ee971d

konstin marked this pull request as ready for review April 17, 2024 08:33

zanieb mentioned this pull request Apr 23, 2024

Combine dependency clauses with the same root astral-sh/uv#3225

Merged

Eh2406 approved these changes May 1, 2024

View reviewed changes

Eh2406 added this pull request to the merge queue May 1, 2024

Merged via the queue into pubgrub-rs:dev with commit d5ed801 May 1, 2024
5 checks passed

konstin deleted the konsti/dev/custom-incompatibility-metadata branch May 1, 2024 21:23

konstin mentioned this pull request May 2, 2024

Use generic pubgrub incompatibility reason astral-sh/uv#3335

Merged

konstin mentioned this pull request Jun 7, 2024

Allow an arbitrary, custom incompatibilities #152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic reason for custom incompatibility #208

Generic reason for custom incompatibility #208

konstin commented Apr 15, 2024 •

edited

Loading

zanieb Apr 15, 2024

konstin Apr 15, 2024

zanieb Apr 15, 2024

konstin Apr 15, 2024

zanieb Apr 15, 2024

Eh2406 Apr 16, 2024

mpizenberg Apr 17, 2024 •

edited

Loading

konstin Apr 19, 2024

Eh2406 commented Apr 16, 2024

Eh2406 Apr 16, 2024

konstin Apr 17, 2024

zanieb Apr 23, 2024

konstin Apr 17, 2024

konstin commented Apr 17, 2024

Eh2406 commented Apr 27, 2024

Eh2406 left a comment

Generic reason for custom incompatibility #208

Generic reason for custom incompatibility #208

Conversation

konstin commented Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpizenberg Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eh2406 commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstin commented Apr 17, 2024

Eh2406 commented Apr 27, 2024

Eh2406 left a comment

Choose a reason for hiding this comment

konstin commented Apr 15, 2024 •

edited

Loading

mpizenberg Apr 17, 2024 •

edited

Loading