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

Add a version parsing and comparison API to librpmio #1221

Merged
merged 10 commits into from May 27, 2020

Conversation

pmatilai
Copy link
Member

This adds a (beginnings of) a version parsing and comparison API to librpmio.

Details in the commit messages, but the (obvious) motivation is to finally have a meaningful API for handling rpm versions in and out of rpm. For maximum coverage, this needs to be in librpmio which allows usage from Lua, macro engine and expression parser.

--nopromote support is ripped out to the extent possible without requiring soname bump to avoid dragging that ancient remnant into the new API.

Fixes #561 and #897 and paves the way for #1217.

No functional change, but we'll need this in the next step.
Adding a new header just for this seems a bit much but we'll be adding
stuff there shortly.

No functional changes as such, this is prerequisite for supporting
version comparison in expressions.
No functional changes, just lose extra baggage that was needed to append
initialize and append rpm.vercmp() from librpm side when the rest is in
librpmio. We might need to bring it back some day, but given the history
here it doesn't seem all that likely.
Epoch promotion was a thing around and before the turn of the millenium,
we really shouldn't be carrying that cruft around anymore.

Remove all traces that we can without too much guilt about breaking
ABI/API and not bumping soname which we dont want to do for this
stupid old thing: all the symbols are left in place, they just don't
work anymore. Nobody should notice as nobody can have been using this
stuff in the last 15+ years.
@pmatilai
Copy link
Member Author

Rebase to clear fixups.

@ffesti
Copy link
Contributor

ffesti commented May 27, 2020

Removing the nopromote API in Python but keeping it in C seems inconsistent. Not sure if I really care.

@ffesti
Copy link
Contributor

ffesti commented May 27, 2020

Overall this looks good. I am wondering what all the malloc and free costs us as we might do a few version comparisons during a transaction. Probably not enough to actually worry. Converting the EVR of the rpmds object just for comparison seems kinda weird. I am fine with this as a first step but we should consider moving the rpmds to use versions from the start. Which asks the question how this relates to rpmsid...
That's not something we want to get into right now but we might want to think about in the 4.17 time frame. For now it is great we have something like this at all.

@pmatilai
Copy link
Member Author

This doesn't actually add any mallocs that were not there before for the busy case of rpmds comparisons: previously the strings were strdup()'ed in rpmdsCompareEVR() before passing to parseEVR(), now this is happens in rpmevr.c instead. The ver handle is all alloced as a single blob so there's no added cost (except for some error checking).

As for rpmds and string pool, all good questions that occurred to me too, but I don't have immediate answers. It'd seem attractive to run the rpmds versions through this just for enforced error checking, but then those are in the strpool, and the pool has its own non-trivial cost, and the private pool trick we use elsewhere seems way out of proportion for storing a couple of tiny strings.

My excuse for ripping epoch promotion from Python but leaving the ruins in C is basically just that in C, removing would require soname bump, but Python has nothing of the sort. I can certainly put it back there if you prefer.

BTW, last fixup added support for python (e,v,r) tuples as a way of creating version objects, I initially pushed the python bindigs a little bit too hastily :)

@pmatilai
Copy link
Member Author

Another fixup to avoid multiple reallocations in rpmverEVR().

It's more than a little hysterical that rpm hasn't had a meaningful
public API for parsing and comparing version strings. The API may seem
kinda overkill for some things but then this does give us a place to
check for invalid version strings, which is a thing we never had before
(although the current code doesn't do much in the way of checking)

Fixes: rpm-software-management#561
…software-management#897)

The low-level rpmvercmp() was always the wrong thing to export to Lua,
but its what people have used because it's there and its used for
comparing full versions. So rather than add a separate API for the higher
level comparison, lets just change rpm.vercmp() do the right thing instead.

Fixes: rpm-software-management#897
Use the newly added version converter function for parsing labelCompare()
arguments, gaining automatic access to all formats that we support
in rpm.ver() constructor. Currently this means (E,V,R) tuples which
labelCompare() always used, plus plain old strings. In future, might
be something more.
@pmatilai
Copy link
Member Author

Rebased once more with some tweaks to gain support strings in Python labelCompare() essentially for free things like rpm.labelCompare("1:1.0-1", "2.0-3") now do the right thing without requiring the painful tuples.

@ffesti ffesti merged commit 4f7a500 into rpm-software-management:master May 27, 2020
@Vogtinator
Copy link
Contributor

Just to make sure, this behaviour change of rpm.vercmp in lua is backwards-compatible, right?
I would assume so, as any segment is also a valid EVR, but there might be some edge case.

Currently we're doing the parsing "by hand" in lua: https://build.opensuse.org/package/view_file/Base:System/rpm/rpmsort?expand=1 I'm really looking forward to drop that for good.

@pmatilai
Copy link
Member Author

It should be compatible for legit values of E, V, R. I'm quite positive you can come up with strange behavior differences if you feed it versions containing ':' or '-' though.

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

Successfully merging this pull request may close these issues.

Add an API + bindings for parsing EVR
3 participants