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

Fix compatibility with >=python-semanticversion-2.7.0 #86

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rbarrois
Copy link

@rbarrois rbarrois commented Sep 7, 2019

Context

The recent versions of python-semanticversion made changes to private
APIs, removing the interaction between Version(x, partial=True) and
Spec() (partial=True was designed for implementing the Spec class
only).

This breaks releases, has mentioned in #84 and #85; it has also been reported as affecting twine's doc building (pypa/twine#492).

The attached patch switches to the recommended usage of python-semanticversion (whose docs are still lacking in that regard, sorry 😞), while keeping all tests green - both with updated and old python-semanticversion releases.

Implementation notes

The code used Version(..., partial=True) to exclude ranges of version whose major
component didn't match a bugfix/issue range; the code went akin to:

Version('1', partial=True) in Spec('>=1.0')

This no longer works; this patch changes that behaviour to exclude
families where no actual release matches the bugfix/issue range - this
should be more accurate.

The patch also uses Version.coerce, the intended API to manage
non semver-compliant version strings.

Compatibility

The patch has been tested with both python-semanticversion==2.6.0 and
python-semanticversion==2.8.1; it can be included to an upgraded version
of releases even if users haven't yet upgraded python-semanticversion.

The recent versions of python-semanticversion made changes to private
APIs, removing the interaction between `Version(x, partial=True)` and
`Spec()` (`partial=True` was designed for implementing the `Spec` class
only)

The code used these classes to exclude ranges of version whose major
component didn't match a bugfix/issue range; the code went akin to:

    Version('1', partial=True) in Spec('>=1.0')

This no longer works; this patch changes that behaviour to exclude
families where no actual release matches the bugfix/issue range - this
should be more accurate.

The patch also uses `Version.coerce`, the intended API to manage
non semver-compliant version strings.

The patch has been tested with both python-semanticversion==2.6.0 and
python-semanticversion==2.8.1; it can be included to an upgraded version
of `releases` even if users haven't yet upgraded python-semanticversion.
@rbarrois
Copy link
Author

rbarrois commented Sep 7, 2019

The two failing tests seem to me to be a travis-side issue: I haven't been able to reproduce them locally with the same Sphinx and Python version.

Travis reports an issue with a sphinx build exiting with an unexpected error code, but the trace mentions that the build has succeeded and no error message...

@jaraco
Copy link

jaraco commented Nov 17, 2019

I've confirmed this change fixes releases where it currently fails. Please find a way to release this change.

@rixx
Copy link

rixx commented Dec 23, 2019

Any news on this PR? It would be nice to be able to unpin python-semanticversion.

@bitprophet
Copy link
Owner

bitprophet commented Jan 10, 2020

I'm actually leaning towards just pinning our requirements at semantic-version<2.7 (re #84), since they've now proven a tendency to break APIs on minor versions.

Do you have needs for features added in semantic-version in 2.7 or 2.8? To my knowledge Releases itself does not, so I'd rather punt until we do.

@bitprophet bitprophet closed this Jan 10, 2020
@rbarrois
Copy link
Author

@bitprophet As the maintainer of python-semanticversion, I feel a bit hurt by the wording of your decision — derivating a "tendency" from a single change to a private, non-documented API seems a bit harsh.

Since releases is a release management tool, pinning the version on your side forces that decision onto users of releases too; they'll have to choose between:

  • Not using releases
  • Using an outdated version of python-semanticversion
  • Using two different virtual environments, one for building/testing and another for releasing (and all calls to releases.

How can we avoid this blocking situation? Would you be interested by a further pull request that would either vendor python-semanticversion, or simply include a minimal subset of its functionalities?

@bitprophet
Copy link
Owner

bitprophet commented Jan 29, 2020

@rbarrois That's fair, "tendency" was poor word choice on my part, I should have put something less implying a string of events - apologies.

I'm confused by the assertion that the API call is private though - all our subclass did was call Version with partial=True and that seems to be part of the public API. Am I overlooking something? I definitely didn't have a mental note that we were using its undocumented innards (like I do have for all of our Sphinx interactions...).

EDIT after reading the PR more and discovering the commit message especially: got it. partial I guess was undocumented previously?


Re: the rest: I'm well aware of the tradeoffs involved in pinning, sadly. It's never ideal but especially with limited dev time it's often a very tempting band-aid.

That said, since I last visited this ticket, I've decided to pop out a 2.0 of Releases for Sphinx reasons (dropping older Sphinxes; no API change on our end); so I'm tempted to merge this after all, and if necessary raise our semantic_version requirement (but hopefully not?).

@bitprophet bitprophet reopened this Jan 29, 2020
@bitprophet
Copy link
Owner

Unfortunately as-is the PR breaks the test suite. If I can't figure out exactly why, I may try creating a variant that is less of a delta from master while still being compliant with modern versions of semantic_version.

@bitprophet
Copy link
Owner

bitprophet commented Jan 29, 2020

Still poking but it always seems to come back to the fact that a lot of the logic in Releases /really/ wants that partial-version feature.

Many such spots feel like they logically should kinda be able to be Spec objects instead (and in fact this is suggested in semantic_version's docs, eg SimpleSpec('1.x.x')), but Specs are not Versions and so cannot be treated quite like a partial Version could (eg you cannot ask if one of these Specs matches another, as they lack truncate).

Trying to see if I can work around this one way or another (e.g. trying to use 'floor' versions, 1.0.0 instead of '1.*' etc).

bitprophet added a commit that referenced this pull request Jan 30, 2020
… objects instead of partial ones, plus related pseudo hacks. all unit tests pass but one
@bitprophet
Copy link
Owner

Ran out of working tendons but have an interim branch (autolinked above). Some changes the same as this PR, some different. Only one test fails vs the many failing in this PR, so I think I am on the right track and missing one spot somewhere.

The changes here imply I want to keep a Version subclass around that "acts partial" but in a way that does not rely on the existing/deprecated, internal-to-semantic_version partial kwarg. (It would instead just isolate the dumb "tack on zeroes" stuff seen in my branch.)

As seen in TODOs, also, a lot of the Releases-level code is probably kinda dumb and wants reorganizing anyways. Whether I'll do that as part of this or punt, not sure.

@rbarrois
Copy link
Author

@bitprophet awesome, thanks for looking into this! I'll take a look at your branch later today, hopefully I'll be able to help you finding a good solution, or to see what kind of APIs would be needed over in python-semantic_version to support your use case ;)

@bitprophet bitprophet added this to the 1.5 milestone Apr 24, 2020
KAction pushed a commit to KAction/nixpkgs that referenced this pull request Nov 4, 2022
Previously, derivation patched-out constraint on semantic_version, but did not
address run-time error about

    Call either Version('1.2.3') or Version(major=1, ...).

This is known issue, but upstream chooses to pin "semantic_version<2.7" as
solution. More information in bitprophet/releases#86.
@bitprophet bitprophet force-pushed the main branch 6 times, most recently from f3f660b to f986143 Compare December 18, 2022 04:35
@bitprophet bitprophet force-pushed the main branch 2 times, most recently from cda450e to 80e1d49 Compare December 19, 2022 02:07
@bitprophet bitprophet force-pushed the main branch 4 times, most recently from 8d2fe0c to b18cd60 Compare December 31, 2022 04:19
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.

4 participants