pep440: fix version ordering #1883
Conversation
BurntSushi
requested review from
charliermarsh and
konstin
and removed request for
charliermarsh
|
Nice find. I'm too tired to review this today. Does this have any implications on perf? |
|
I don't think. I can check later. If anything, it will help, because it expands the set of versions that can be represented in a packed format. But also, do not merge this yet. We need a way to invalidate the cache before we can ship this. Otherwise existing versions in the cache will be interpreted incorrectly. (Which might lead to panics or even silent logic bugs.) |
We can bump the cache bucket suffixes ( |
A couple moons ago, I introduced an optimization for version comparisons by devising a format where *most* versions would be represented by a single `u64`. This in turn meant most comparisons (of which many are done during resolution) would be extremely cheap. Unfortunately, when I did that, I screwed up the preservation of ordering as defined by the [Version Specifiers spec]. I think I messed it up because I had originally devised the representation so that we could pack things like `1.2.3.dev1.post5`, but later realized it would be better to limit ourselves to a single suffix. However, I never updated the binary encoding to better match "up to 4 release versions and up to precisely 1 suffix." Because of that, there were cases where versions weren't ordered correctly. For example, this fixes a bug where `1.0a2 < 1.0dev2`, even though all dev releases should order before pre-releases. We also update a test so that it catches these kinds of bugs in the future. (By testing all pairs of versions in a sequence instead of just the adjacent versions.) [Version Specifiers spec]: https://packaging.python.org/en/latest/specifications/version-specifiers/#summary-of-permitted-suffixes-and-relative-ordering
BurntSushi
force-pushed
the
ag/fix-version-order
branch
from
de07d75
to
177895a
Compare
Done! |
| number, | ||
| }) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
Should number be 0 here? (Should we assert?)
There was a problem hiding this comment.
Nope! If the kind isn't a pre-release, then it could be something else with a non-zero number.
A couple moons ago, I introduced an optimization for version comparisons
by devising a format where most versions would be represented by a
single
u64. This in turn meant most comparisons (of which many aredone during resolution) would be extremely cheap.
Unfortunately, when I did that, I screwed up the preservation of
ordering as defined by the Version Specifiers spec. I think I messed
it up because I had originally devised the representation so that we
could pack things like
1.2.3.dev1.post5, but later realized it wouldbe better to limit ourselves to a single suffix. However, I never
updated the binary encoding to better match "up to 4 release versions
and up to precisely 1 suffix." Because of that, there were cases where
versions weren't ordered correctly. For example, this fixes a bug where
1.0a2 < 1.0dev2, even though all dev releases should order beforepre-releases.
We also update a test so that it catches these kinds of bugs in the
future. (By testing all pairs of versions in a sequence instead of just
the adjacent versions.)