Skip to content

Restore check_version() #65

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
Merged
merged 3 commits into from
Nov 1, 2020
Merged

Restore check_version() #65

merged 3 commits into from
Nov 1, 2020

Conversation

zakv
Copy link
Contributor

@zakv zakv commented Aug 18, 2020

Resolves #62

This PR reverts d7230c5 and replaces distutils.version.LooseVersion with packaging.version as suggested in #62.

@zakv
Copy link
Contributor Author

zakv commented Aug 18, 2020

Just to double check, LooseVersion('3.0.0rc2.dev11+g8803447') < LooseVersion('3.0.0') gave False, but packaging.version.parse('3.0.0rc2.dev11+g8803447') < packaging.version.parse('3.0.0') gives True. Is that new behavior desired?

@philipstarkey
Copy link
Member

I think the new behaviour is what we want. Release candidates/development versions come before actual releases. The only thing I'm uncertain of is whether our setuptools_scm configuration actually roles over to a new minor version once we create a release branch. If it doesn't then new development would still be on the old version and appear older in check_version comparisons even though it's actually newer.

@zakv
Copy link
Contributor Author

zakv commented Aug 18, 2020

Yep I think that is an issue. That '3.0.0rc2.dev11+g8803447' version string above is what I have after branching off of the most recent commit from lyse's master branch and then committing some changes. I would have expected that version to count as greater than 3.0.0 and 3.0.0rc2, but the opposite is true.

Maybe this means that the setuptools_scm configuration needs to be changed? Alternatively I could revert the code back to using LooseVersion.

@chrisjbillington
Copy link
Member

chrisjbillington commented Aug 18, 2020 •
edited
Loading

Branching off release branches should never be done without a tag at the branch point incrementing the minor number. That's our plan, anyway.

Doesn't that mean this distinction is moot?

@zakv
Copy link
Contributor Author

zakv commented Aug 18, 2020

Are you saying that there should be a '3.1.0' tag in the lyse repo right now which would make feature branches be versioned '3.1.0...' instead of '3.0.0...?

@philipstarkey
Copy link
Member

philipstarkey commented Oct 30, 2020 •
edited
Loading

@zakv I've made a change to how setuptools_scm does version numbers in #70. Does this change the observed behaviour for you in this PR? (you may need to rerun the editable pip install command with --no-deps to get a new version...can't remember if we fixed the need for rerunning that after update or not now)

EDIT: You don't need to rerun the pip install. __version__.py detects the .git folder and uses setuptools_scm instead of package metadata (which is the point of fixing setuptool_scm config)

@zakv
Copy link
Contributor Author

zakv commented Oct 30, 2020

Yep! Without #70 labscript_utils.__version__ gives '3.0.0rc2.dev39+g91b7562', but with #70 that gives '3.1.0.dev44+g570f587'. I believe that makes the ordering between release versions and commits on master make sense; commits since '3.0.0' are '3.1.0.dev...' so they are ordered after '3.0.0' but before '3.1.0'.

Looks like the 'rc' part isn't included in the version string prodcued with #70. Does that matter?

@philipstarkey
Copy link
Member

Yep, the missing RC is expected because we have not tagged any releases on master after the last release branch. If you wanted to test that locally, you could add a tag like v3.1.0rc1 and see what happens. Don't forget to delete the tag before your next push though if you do that!

@philipstarkey philipstarkey self-assigned this Oct 31, 2020
@zakv zakv mentioned this pull request Oct 31, 2020
Merged
@philipstarkey philipstarkey merged commit 5f8280f into labscript-suite:master Nov 1, 2020
@zakv zakv deleted the restore-check_version branch November 1, 2020 23:38
philipstarkey added a commit to philipstarkey/labscript-utils that referenced this pull request Nov 2, 2020
@philipstarkey
Added API docs for versions submodule added in labscript-suite#65
2c433e2 
Formatted docstrings to match sphinx Doogle style and added rst file to docs to display them.
philipstarkey added a commit that referenced this pull request Nov 2, 2020
@philipstarkey
Merge pull request #72 from philipstarkey/feature/versions-module-docs
4581953 
Added API docs for versions submodule added in #65
philipstarkey added a commit that referenced this pull request Nov 2, 2020
@philipstarkey
Squashed commit of the following:
a0d4442 
commit 4581953
Merge: d6335b2 eaa13a0
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 16:27:23 2020 +1100

    Merge pull request #72 from philipstarkey/feature/versions-module-docs

    Added API docs for versions submodule added in #65

commit eaa13a0
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 16:24:02 2020 +1100

    Second attempt at fixing the missing site package functions.

    This time we explicitly mock the missing functions using a lambda that returns an empty version of whatever types the methods usually return (hard coded into conf.py)

commit d6335b2
Merge: f1c0962 720003b
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 15:58:56 2020 +1100

    Merge pull request #73 from philipstarkey/bugfix/setuptools_scm

    Removes setuptools_scm configuration from pyproject.toml

commit 720003b
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 15:57:33 2020 +1100

    Removes setuptools_scm configuration from pyproject.toml

    As such, I see no way to utilise pyproject.toml for these settings, and am reverting to using the configuration specified in setup.py (restored in #71)

commit f1c0962
Merge: dce6ac8 fa2654c
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 13:06:10 2020 +1100

    Merge pull request #71 from philipstarkey/bugfix/setuptools_scm

    Bugfixes for the mistakes in #70

commit 83c6ded
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 13:04:42 2020 +1100

    Attempt to fix the site module issue on readthedocs.

commit 2c433e2
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 12:51:48 2020 +1100

    Added API docs for versions submodule added in #65

    Formatted docstrings to match sphinx Doogle style and added rst file to docs to display them.

commit fa2654c
Author: Phil Starkey <philipstarkey@users.noreply.github.com>
Date:   Mon Nov 2 11:09:10 2020 +1100

    Bugfixes for the mistakes in #70

    labscript-suite/labscript-suite#53 erroenously removed the env var that removed the local version (required for uploading to test PyPI).

    Contrary to my statements in #62, the packaging library does not seem to be a dependency of setuptools. Instead they have vendored it as an internal submodule. Rather than rely on that, packaging is now a dependency of labscript-utils directly.
@dihm dihm mentioned this pull request Feb 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@philipstarkey philipstarkey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restore check_version
3 participants
@zakv @philipstarkey @chrisjbillington