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.

Sign up for GitHub

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

Jump to bottom

Fix parsing of package versions in packaging 22. #2037

Merged
merged 4 commits into from
Dec 8, 2022
Merged

Fix parsing of package versions in packaging 22. #2037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
088e792
Fix parsing of package versions in packaging 22.
hodgestar Dec 8, 2022
46d0943
Remove unused import of re module.
hodgestar Dec 8, 2022
5e32a4d
Run bash with -e so that the towncrier check fails correctly.
hodgestar Dec 8, 2022
6ec6747
Add towncrier entry.
hodgestar Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ defaults:
# necessary for having conda behave sensibly. We use bash as the shell even
# on Windows, since we don't run anything much complicated, and it makes
# things much simpler.
shell: bash -l {0}
shell: bash -l -e {0}

jobs:
cases:
Expand Down
1 change: 1 addition & 0 deletions doc/changes/2037.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix parsing of package versions in packaging 22.
6 changes: 4 additions & 2 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import collections
import os
import pathlib
import re
import subprocess
import sys
import sysconfig
Expand Down Expand Up @@ -134,7 +133,10 @@ def _determine_version(options):
with open(version_filename, "r") as version_file:
version_string = version_file.read().strip()
version = packaging.version.parse(version_string)
if isinstance(version, packaging.version.LegacyVersion):
# LegacyVersion was removed in packaging 22, but is still returned by
# packing <= 21
LegacyVersion = getattr(packaging.version, "LegacyVersion", type(None))
if isinstance(version, LegacyVersion):
raise ValueError("invalid version: " + version_string)
Comment on lines +136 to 140
Copy link

@pradyunsg pradyunsg Dec 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a drive-by suggestion for something that doesn't rely on LegacyVersion in any way:

try:
   version = packaging.version.Version(version_string)
except packaging.version.InvalidVersion:
   raise ValueError("invalid version: " + version_string) from None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! After the discussion in the packaging repo issue, I have been thinking about simplifying things this way, but I need to go look at the history of Version first and see which packaging releases it has been around for and whether the behaviour has changed over time so that I don't accidentally mess anything up for users still using older releases of packaging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even just do version = packaging.version.Version(version_string) because we don't care about what kind of exception is raised, just that we get an error if we try to build a package with a bad version string.

Copy link

@pradyunsg pradyunsg Dec 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been around for a really long time.

Here's that from the oldest tag in the git repo, from before the parse function being introduced: https://github.com/pypa/packaging/blob/14.0/packaging/version.py#L78

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

options['short_version'] = str(version.public)
options['release'] = not version.is_devrelease
Expand Down