Select pre-releases like pip does #141
Conversation
|
This seems rather complex. Why not store whether prerelase are supported in the version instead of in the versionset. You know all the information when all versions are constructed. |
|
Hmm, maybe a good idea. We still need to store wether the versionset refers to any prerelease if I am not mistaken.
Does that make sense @tdejager @baszalmstra ? :) |
|
I think we just need one boolean in the version and one in the versionset. Both define wether prereleases are allowed. your 1 and 2 should be able to inform initial values there I guess! I think the options can stay in the provider and be read to compute the booleans. I dont think we need to complicate resolvo. |
|
Yep, you might be right :) I think what I just pushed captures your idea (I can revert back to stock-resolvo with this). I did this pretty quickly – I think the code can be made more beautiful :) |
|
Nice! Code could use some improvement indeed but I think the idea is solid! |
|
Implementation looks solid! Note that with this PR we are still deviating from the spec (https://packaging.python.org/en/latest/specifications/version-specifiers/#handling-of-pre-releases) if I'm reading it correctly namely. https://packaging.python.org/en/latest/specifications/version-specifiers/#handling-of-pre-releases
So instead matching pre-releases, only if the project has pre-releases, the spec said we should match if these are the only options that match this version range. However, as Bas and others noted, this is a bit of strange requirement that can potentially lead to unwanted behavior. Also, in issue #118 another issue followed that they want to investigate the spec and pip implementation anyways. pypa/pip#12471 (comment) So I think it's fine at this point to deviate from spec, however, it should be noted in the code that we are doing so I think |
wolfv
changed the title
|
Added some docs, but no test yet. :) |
| pub(crate) struct PypiVersionSet(Option<VersionOrUrl>); | ||
| pub(crate) struct PypiVersionSet { | ||
| spec: Option<VersionOrUrl>, | ||
| allows_prerelease: bool, |
There was a problem hiding this comment.
It would be good to add a little bit of documentation to this field.
crates/rattler_installs_packages/src/resolve/dependency_provider.rs
Outdated
Show resolved
Hide resolved
crates/rattler_installs_packages/src/resolve/dependency_provider.rs
Outdated
Show resolved
Hide resolved
| }, | ||
| ) => { | ||
| spec.contains(version) | ||
| && (self.allows_prerelease || *only_prerelease || !version.any_prerelease()) |
There was a problem hiding this comment.
I have to try really hard to understand this boolean logic. Would you be able to comment it a little?
crates/rattler_installs_packages/src/resolve/dependency_provider.rs
Outdated
Show resolved
Hide resolved
crates/rattler_installs_packages/src/resolve/dependency_provider.rs
Outdated
Show resolved
Hide resolved
crates/rattler_installs_packages/src/resolve/dependency_provider.rs
Outdated
Show resolved
Hide resolved
|
FYI, I've been testing this branch against the new apache-airflow, including the new 2.8.1 as they have completely revamped their packaging infrastructure: https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html#airflow-packaging-specification-follows-modern-python-packaging-standards-36537 The pre-releases look good, but I think this exposes some other issues with what packages rip visits when resolving, I will retest and file new issues once this lands (as I am not able to merge this branch with main locally myself). |
wolfv
force-pushed
the
prerelease-ii
branch
2 times, most recently
from
e2123ab to
3b1f98b
Compare
| /// This is true if there are only pre-releases available for this package | ||
| /// For example, if the package `foo` has only versions `foo-1.0.0a1` and `foo-1.0.0a2` | ||
| /// then this will be true. This allows us later to match against this version and | ||
| /// allow the selection of pre-releases. |
There was a problem hiding this comment.
Comment needs to be updated to account for all cases that this can be set to true :)
|
Let's merge when we have fixed the open comments :) |
I tried something slightly different here: matching pre-releases but being able to pass in additional options to the
contains(...)function.I was planning to add a
HashMap<PackageId, bool>that would contain wether a given package only has pre-releases, however, we don't have the pacakge names available. However, now that I am thinking of it, we could just add it as a second field to each version spec. Or maybe we should just add the prerelease yes/no option to the version spec itself. Maybe we don't even need the extra options ...