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

Versions start with v #1724

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
3 participants
@eyal0
Copy link
Contributor

commented Jan 21, 2017

What does this PR do and why is it necessary?

Allows github versions to being with a "v". Up-to-date versions of pkg_resources, which OctoPrint uses to parse version strings, can handle version strings that start with a "v". But if the pkg_resources version that is available is old, OctoPrint uses the tuple returned to create a version string. But it handle the leading "v" like the new version and so github releases that start with a "v" will not be sorted correctly.

github themselves suggests putting a "v" in front of release names so it would be nice for OctoPrint to support it, too.

How was it tested? How can it be tested by the reviewer?

I tested this by uploading to my OctoPi, which has an old version of pkg_resources, and forced a check for software updates and it worked where previously it didn't. I also ran it from python to see if it works:

└──> ~/private/OctoPrint/venv/bin/python
Python 2.7.6 (default, Oct 26 2016, 20:30:19) 
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import softwareupdate
>>> import softwareupdate.version_checks.github_release
>>> softwareupdate.version_checks.github_release._get_latest_release("eyal0","OctoPrint-UserWhitelist","python")
/home/eyalsoha/private/OctoPrint/venv/local/lib/python2.7/site-packages/requests-2.7.0-py2.7.egg/requests/packages/urllib3/util/ssl_.py:90: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. For more information, see https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
(u'Fix default usernames list', u'v1.0.2', u'https://github.com/eyal0/OctoPrint-UserWhitelist/releases/tag/v1.0.2')

What are the relevant tickets if any?

This fixes OctoPrint/issues#1723 .

@derpicknicker1

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Nice PR. I absolutely support it. There were several plugin authors (including me) who got into a mess with this issue.

@eyal0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2017

It isn't unprecedented, either. You can see that a leading v is supported in the new parse_version:

https://github.com/pypa/setuptools/blob/master/pkg_resources/_vendor/packaging/version.py#L160

@@ -140,6 +140,9 @@ def _get_base_from_version_tuple(version_tuple):
"""

base_version = []
# A leading v is common in github release tags. Remove it.

This comment has been minimized.

Copy link
@foosel

foosel Jan 24, 2017

Owner

This will only modify the version tuple of a base version ("1.2.0" instead of "1.2.0.dev23") was requested for version comparison - the v needs to be stripped from the tuple in any case however.

I think it would need to be moved into _get_comparable_version_pkg_resources for that.

This comment has been minimized.

Copy link
@eyal0

eyal0 Jan 24, 2017

Author Contributor

Hmm, I suppose that's true. I'll modify the PR.

@foosel

This comment has been minimized.

Copy link
Owner

commented Jan 24, 2017

Nice!

I think there's still a problem with the current implementation though and added a comment about that, could you take a look?

As a side note, the relevant code from the new parse_version has been moved here.

I also briefly wondered if it would make sense to extend the tuple handling to strip any parts that are prefixed with * from the start, but that would only work for old versions and I guess the goal is rather compatibility to newer implementations.

@eyal0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

I made the modifications necessary. This is the result of testing using an old version of setuptools:

pi@octopi:~/OctoPrint/src/octoprint/plugins $ ~/oprint/bin/python
Python 2.7.9 (default, Mar  8 2015, 00:52:26) 
[GCC 4.9.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import softwareupdate.version_checks.github_release
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("v1.2.3-dev1", True)
('00000001', '00000002', '00000003')
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("v1.2.3-dev1", False)
('00000001', '00000002', '00000003', '*@', '00000001', '*final')
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("1.2.3-dev1", True)
('00000001', '00000002', '00000003')
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("1.2.3-dev1", False)
('00000001', '00000002', '00000003', '*@', '00000001', '*final')
>>> 

This is the result on a new version of setuptools:

└──> ~/private/OctoPrint/venv/bin/python
Python 2.7.6 (default, Oct 26 2016, 20:30:19) 
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import softwareupdate.version_checks.github_release
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("v1.2.3-dev1", True)
<Version('1.2.3')>
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("v1.2.3-dev1", False)
<Version('1.2.3.dev1')>
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("1.2.3-dev1", True)
<Version('1.2.3')>
>>> softwareupdate.version_checks.github_release._get_comparable_version_pkg_resources("1.2.3-dev1", False)
<Version('1.2.3.dev1')>
>>> 

We can see that whether or not there is a leading v is ignored, both for old and new setuptools.

@eyal0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

Let me know if further changes are needed, thanks!

@foosel

This comment has been minimized.

Copy link
Owner

commented Jan 24, 2017

Looks fine now 👍

I adjusted a code comment slightly (more as a reminder for me than anything - the code in the plugin manager is only needed for checking OctoPrint's own tags, which will always follow semantic versioning without a leading v, but it still makes sense to match behaviour of newer setuptools versions), added some whitespace and added you to AUTHORS.md before squashing your commits and cherry picking them on maintenance so they'll be in 1.3.2!

See 6b2f282 and 9250cd0

I'll also merge that up into devel ASAP.

One thing: Github doesn't catch cherry picked commits on another branch and therefore will mark this PR as closed instead of merged. Don't be alarmed about that, your work has in fact been merged and it's just Github being a tad too inflexible here to cope with such situations. Thank you!

@foosel foosel closed this Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.