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

Replace cmp_version with rpm python bindings #110

Merged
merged 1 commit into from
Jun 7, 2019

Conversation

rbikar
Copy link
Member

@rbikar rbikar commented Jun 6, 2019

This change replaces buggy cmp_version package
with python bindings of official rpm project.

This should ensure that ubipop will sort rpms by correctly implemented and maitained vercmp algorithm.

Fixes #104

This change replaces buggy cmp_version package
with python bindings of official rpm project.
@rbikar rbikar requested a review from a team June 6, 2019 13:46
@rbikar rbikar changed the title Replace cmp_version rpm python bindings Replace cmp_version with rpm python bindings Jun 6, 2019

try:
from urllib.parse import urljoin
except ImportError:
from urlparse import urljoin

import requests
from rpm import labelCompare as label_compare # pylint: disable=no-name-in-module
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding an extra dep, would not be easy to just borrow this function?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, sounds like a plan.

Copy link
Member

Choose a reason for hiding this comment

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

But it's implemented in C, isn't it? I would rather keep our stuff pure-python as much as we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it's in C, I also wouldn't like to have extra C code in our stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, if it's in C makes sense.

@@ -1,4 +1,4 @@
requests
more-executors
ubi-config
cmp_version
rpm-py-installer
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 this might cause some issues for the RPM packages of this project. Is this going to result in the egg always requiring some "rpm-py-installer" distribution to be present, while it's actually nonsense to set that as Requires in an RPM?

In general though I think it's OK for what we put in these repos to be somewhat ignorant of RPM packaging complexities. So this comment is more like a heads up that this might cause us to run into issues and we may need patches in dist-git, or may need to revisit this later.

@rbikar rbikar merged commit e6e0fa7 into release-engineering:master Jun 7, 2019
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.

cmp_version is unable to correctly sort rpm with uneven release parts
5 participants