Skip to content

Conversation

tiran
Copy link
Contributor

@tiran tiran commented Jul 26, 2021

Use the "packaging" package instead of semantic_version package to parse
Rust version and spec. Packaging is used by setuptools to parse version
strings and specs.

This also solves a deprecation warning with semamtic_version. The
partial argument to Version() has been deprecated.

Signed-off-by: Christian Heimes [email protected]

Use the "packaging" package instead of semantic_version package to parse
Rust version and spec. Packaging is used by setuptools to parse version
strings and specs.

This also solves a deprecation warning with semamtic_version. The
partial argument to Version() has been deprecated.

Signed-off-by: Christian Heimes <[email protected]>
@tiran tiran force-pushed the packaging_version branch from 59788ff to a99f46e Compare July 26, 2021 10:37
@davidhewitt
Copy link
Member

Thanks for this. I'm definitely in favour of updating dependencies.

Using packaging to parse Rust versions makes me slightly uneasy. e.g. https://www.python.org/dev/peps/pep-0440/#semantic-versioning explicity says that the PEP 440 "Version" is not compatible with semantic version prereleases. Does this mean therefore that this PR would break compatibility with Rust nightly and beta releases?

I wonder if instead we can fix usage of the semantic_version package. Or is it totally deprecated?

@tiran
Copy link
Contributor Author

tiran commented Jul 28, 2021

semantic_version is still supported, just the partial argument was deprecated in 2.7, https://github.com/rbarrois/python-semanticversion/blob/master/ChangeLog#270-2019-08-28

I'll investigate support for pre-releases with packaging and get back to you.

@tiran
Copy link
Contributor Author

tiran commented Jul 29, 2021

Packaging's version parser parses Rust's beta versions correctly, e.g. rustc 1.55.0-beta.1 (739f8f0a8 2021-07-28) is recognized as 1.55.0b1.

>>> packaging.version.Version("1.55.0-beta.1") < packaging.version.Version("1.55")
True
>>> packaging.version.Version("1.55.0-beta.1") < packaging.version.Version("1.55.0-alpha.1")
False
>>> packaging.version.Version("1.55.0-beta.1") < packaging.version.Version("1.55.0-rc.1")
True

However it does not understand -nightly suffix:

>>> packaging.version.Version("1.56.0-nightly")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/site-packages/packaging/version.py", line 298, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
packaging.version.InvalidVersion: Invalid version: '1.56.0-nightly'

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, we're almost there.

I'm happy for us to proceed with packaging (as it's better maintained) but we'll need to handle this mapping in a couple more places:

  • SpecifierSet presumably also needs to map -nightly to .dev0.
  • The error message raised from get_rust_version if the compiler is not suitable will now use packaging syntax when it formats min_version on utils.py line 73. I think it'd potentially be confusing for users to see Rust versions which don't follow the actual Rust release syntax, so we should map the .bx back to -beta.x and .dev0 back to -nightly in the error.

return None
try:
return semantic_version.SimpleSpec.parse(self.rust_version)
return SpecifierSet(self.rust_version)
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably need to do a similar mapping for -nightly here?

@tiran
Copy link
Contributor Author

tiran commented Aug 3, 2021

The error message raised from get_rust_version if the compiler is not suitable will now use packaging syntax when it formats min_version on utils.py line 73. I think it'd potentially be confusing for users to see Rust versions which don't follow the actual Rust release syntax, so we should map the .bx back to -beta.x and .dev0 back to -nightly in the error.

min_version is provided as an argument to get_rust_version. It doesn't use the representation of packaging.version.Version.

@davidhewitt
Copy link
Member

min_version is provided as an argument to get_rust_version. It doesn't use the representation of packaging.version.Version.

Not sure I follow - I think it's a packaging.version.SpecifierSet now, if I follow the code correctly? (If you're willing to add type annotations to any changed code, that's both welcome and appreciated)

We map the rustc specs to SpecifierSet now, so the error message will have changed?

Also, I think I've spotted another potential issue - it looks like we currently depend on ordering semantic_version.Version objects to determine the "minimum" version:

min_version=max(
filter(
lambda version: version is not None,
(ext.get_rust_version() for ext in self.extensions),
),
default=None,
)

It looks like packaging.SpecifierSet doesn't implement ordering operators, so this code will break?

@davidhewitt
Copy link
Member

Note that I've now removed the deprecated partial flag in #174

In general I'm now leaning towards the opinion that we should stick to semantic_version instead of using packaging. There are enough differences between the Python package versions packaging wants and the semver-compliant Rust compiler versions; as we've proceeded with this PR we have hit a few kinks which have made me think that switching dependency won't make things simpler.

Unless you would like to make a further case for packaging, I propose to close this PR. Thanks for all your work on it.

@davidhewitt davidhewitt closed this Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants