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

[CT-2000] Fix broken semver comparison logic #6834

Closed
jtcohen6 opened this issue Feb 1, 2023 · 0 comments · Fixed by #6838
Closed

[CT-2000] Fix broken semver comparison logic #6834

jtcohen6 opened this issue Feb 1, 2023 · 0 comments · Fixed by #6838
Assignees
Labels
bug Something isn't working deps dbt's package manager

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 1, 2023

Originally posted by @jtcohen6 in #6463 (comment)

adapted from Slack: @jtcohen6 + @gshank on 6 Jan

dbt-core/core/dbt/semver.py

Lines 145 to 151 in 1a6e4a0

# This suppresses the LegacyVersion deprecation warning
with warnings.catch_warnings():
warnings.simplefilter("ignore", category=DeprecationWarning)
if packaging_version.parse(a) > packaging_version.parse(b):
return 1
elif packaging_version.parse(a) < packaging_version.parse(b):
return -1

There are two things at play here, leading to failures in test/unit/test_semver.py:

  1. 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.
  2. Unfortunately, there are also some real discrepancies between PEP 440 and SemVer, and packaging==22.0 lands 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-].

@gshank

It would be nice to not pin "packaging" because of it... Maybe at this point we should defer this to a re-write

@github-actions github-actions bot changed the title Fix broken semver comparison logic [CT-2000] Fix broken semver comparison logic Feb 1, 2023
@jtcohen6 jtcohen6 added bug Something isn't working Team:Language deps dbt's package manager labels Feb 1, 2023
@gshank gshank self-assigned this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deps dbt's package manager
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants