remove packaging<22 cap #6463
remove packaging<22 cap #6463
Conversation
jtcohen6
added
Team:Language
ready_for_review
There was a problem hiding this comment.
dbt Core does actually use LegacyVersion under the covers in that the version code falls back to legacy version if the version method doesn't work. So switching to later versions of packaging might break version strings that are currently accepted. Which is why a number of the test cases are currently failing.
If you could identify some versions which succeed currently and would fail under a later version of packaging, we could make a decision about whether this is a change that we want to do at this point.
|
Another alternative might be to use our own version function. I haven't really looked into how feasible that is though. Pinning this forever is certainly not optimal. |
| if packaging_version.parse(a) > packaging_version.parse(b): | ||
| return 1 | ||
| elif packaging_version.parse(a) < packaging_version.parse(b): | ||
| return -1 |
There was a problem hiding this comment.
adapted from Slack: @jtcohen6 + @gshank on 6 Jan
There are two things at play here, leading to failures in test/unit/test_semver.py:
- This logic is not quite right! We're passing just part of a version string into
packaging.version.parse(), rather than the whole string. This is the cause of some of the failing tests. - Unfortunately, there are also some real discrepancies between PEP 440 and SemVer, and
packaging==22.0lands firmly on the side of PEP 440. This is the cause of the rest of the failing tests.
These versions are still supported by packaging==22.0. (Although PEP 440 says not to use a hyphen, and SemVer says to use a hyphen.) The failure is because we're trying to parse just the prerelease identifier (a1, b2) as if it's an entire version string:
'1.2.0a1''1.2.0-a1''1.2.0b2''1.2.0-b2'
These are valid per SemVer, but they are not valid per PEP 440, and so they fail on packaging.version.parse() starting with packaging==22.0:
'2.2.0-fishtown-beta''2.2.0asdf'
https://peps.python.org/pep-0440/#pre-releases
The pre-release segment consists of an alphabetical identifier for the pre-release phase, along with a non-negative integer value. Pre-releases for a given release are ordered first by phase (alpha, beta, release candidate) and then by the numerical component within that phase.
https://semver.org/#spec-item-9
A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-].
It would be nice to not pin "packaging" because of it... Maybe at this point we should defer this to a re-write
|
@smackesey we couldn't easily lift the upper bound from Thank you for opening the original issue in #6461, drafting this PR, and spawning the discussion in #6495 -- we appreciate all of them! |
resolves #6461
Description
This PR removes the version cap on
packaging. This cap was preventing dbt-core from being installed in the same environment as tox 4. It also did not appear to be necessary-- the main breaking change from packaging <22 to 22+ is the drop ofLegacyVersion(which used to be a possible return frompackaging.parse).dbt-coredid not contain any direct references toLegacyVersion, and AFAICT does not depend on any functionality specifically inpackaging<22, but I might be wrong.Checklist
changie newto create a changelog entry