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

Incorrect removal of dash in case of pre-release tag string ? #524

Closed
smarie opened this issue Feb 24, 2021 · 21 comments · Fixed by #584
Closed

Incorrect removal of dash in case of pre-release tag string ? #524

smarie opened this issue Feb 24, 2021 · 21 comments · Fixed by #584

Comments

@smarie
Copy link
Contributor

smarie commented Feb 24, 2021

I found this today: 1.0.0-rc1 becomes 1.0.0rc1, see details here smarie/python-getversion#10

The underlying error is due to pkg_resources that removes the pre-release dash automatically in its string representation since version 0.6a9.

However as suggested in my conclusions I think that the issue should probably better be fixed in the default scheme of setuptools_scm because faithfulness to git tags and compliance with semver for example, is probably a higher concern for you than for pkg_resources.

Thanks again for this great package !

@RonnyPfannschmidt
Copy link
Contributor

RonnyPfannschmidt commented Feb 24, 2021

At first glance i think setuptools scm is correct as it handles standard compliant python version numbers

Those are not in line with semver

@RonnyPfannschmidt
Copy link
Contributor

After reading your conclusion I'll review the details and either fix this in a major release or document the choice

@smarie
Copy link
Contributor Author

smarie commented Feb 24, 2021

Great ! Thanks for your reactivity @RonnyPfannschmidt . In the meantime I'll ship getversion with a patch using a custom version scheme

@RonnyPfannschmidt
Copy link
Contributor

https://www.python.org/dev/peps/pep-0440/#pre-release-separators

I believe that a decision on whether or not to normalise should be made

Im leaning towards always normalising

@jaraco any opinion on the way

@smarie
Copy link
Contributor Author

smarie commented Feb 24, 2021

Note that a conservative move would be that you include a patched version scheme in the "alternate but built-in" version schemes of setuptool_scm : see my current dirty hack

@jaraco
Copy link
Member

jaraco commented Feb 24, 2021

In general, I try to avoid normalizing user inputs, to honor the user's indication, and only to normalize in storage or comparison. However, the consensus from the Python community seems to be to prefer normalization early, to essentially disregard the user's intention and replace it with a normal form.

It does seem to me to be a gap that the most popular versioning scheme (semver) is in conflict with the packaging normalization rules.

I like the idea of providing an "alternate but built-in" version that's lenient to semver syntax. That would provide an opportunity to explore an implementation and to give users a nice escape hatch. I'd probably not recommend this except for very popular and compatible forms like semver.

@smarie
Copy link
Contributor Author

smarie commented Feb 24, 2021

If a PR (with reasonable guidance) is needed at some point, let me know !

@RonnyPfannschmidt
Copy link
Contributor

i would propose enabling users to supply own Version classes
and defaulting to the packaging one either available via setuptools or via packaging

the only requirement being that the str(custom_version_instance) parses as strict python version

additionally i'm not opposed to provide a

NonNormalizedVersion subclass, which simply adds the original input string, if and only if that does not break expectations for pypi/tooling

@smarie
Copy link
Contributor Author

smarie commented Feb 25, 2021

I am not familiar enough with setuptools_scm internals but two things come to my mind, please discard them without caring if they do not apply because there is something I did not understand :

  • str(version.tag) where version is an instance of setuptools_scm.ScmVersion and tag is an instance of pkg_resources.<...>.Version is already what is used in the various formatting templates so there is potentially a conflict between "this could reflect the git version with fidelity" and "this parses as strict python version"

  • there is already a preformatted=True/False attribute somewhere which purpose is to support the fact that version.tag can already be a string (not sure when this can happen though, at least probably this is used when the appropriate OS environment variable is set to "force-hack" a version instead of reading git). I do not know how this relates to your second point about NonNormalizedVersion

@RonnyPfannschmidt
Copy link
Contributor

Preformat is unrelated to this issue

Setuptools_scm should reject wrong versions

So parsing is necessary

A version subclass that stores the non normalised version seem to be a working solution

@abitrolly
Copy link
Contributor

How to turn off the version check? While I understand that people want consistency, I don't think hard policing version numbers is the job of SCM extraction tool.

@RonnyPfannschmidt
Copy link
Contributor

The standard tool setuptools-scm uses to parse valid versions does normalise,

A new version of it that does not would have to be created as i mentioned earlier

Ideally this happens upstream in packaging

@abitrolly
Copy link
Contributor

The tag is already a version string, so how to skip the upstream parser? It looks like preformat does this, but it is not passed down from pyproject.toml.

@RonnyPfannschmidt
Copy link
Contributor

Again, currently that's not implemented

abitrolly added a commit to abitrolly/setuptools_scm that referenced this issue Apr 11, 2021
@abitrolly
Copy link
Contributor

I've added some code to #555 but I have no setup ATM to test it works.

@smarie
Copy link
Contributor Author

smarie commented Jun 17, 2021

Guys, checking this again I see that we have a valid proposal here: #524 (comment)

My only concern would be about configuration: how would users be able to provide the custom class qualname in the use_scm_version ?

For our use, we have use_scm_version={"write_to": f"{PKG_NAME}/_version.py"}, in setup.py.
Is it already something accepted to pass another item in the dictionary (the config class qualname) ? It would ease the PR implementation that such a mechanism exists already

@jaraco
Copy link
Member

jaraco commented Jun 17, 2021

Is it already something accepted...

I suspect not. Designing that interface would likely be part of this effort.

@smarie
Copy link
Contributor Author

smarie commented Jun 17, 2021

It seems that there is such a mechanism actually, and it is documented in the readme :

image

I'll try to dig into this

@RonnyPfannschmidt
Copy link
Contributor

The version class should be configured, it's up for discussion on where to put the non normalising version (packaging or setuptools_scm)

I have a practical use case now (calver that keeps zero prefixes)

@smarie
Copy link
Contributor Author

smarie commented Jun 17, 2021

I'll try to shoot a PR by tomorrow

@RonnyPfannschmidt
Copy link
Contributor

Lovely, I'll complete git archives support and get back to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants