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

RFC: support rpm version comparison in expression parser #1220

Closed
wants to merge 3 commits into from

Conversation

pmatilai
Copy link
Member

Inspired by #1217, this is a proof-of-concept patch series to add support for version comparison in the expression parser, allowing things like this to do the right thing:

%if `%{python_version}` < `3.9`
...
%endif

...and also macros, eg

$ ./rpm --eval '%["5.3.0" == "5.3.0+"]'
0
$ ./rpm --eval '%[`5.3.0` == `5.3.0+`]'
1

Backticks are used to denote versions as opposed to regular strings not because I particularly like them but just because I had to use something and single quotes get painful when testing with shell.
As noted in the commit messages, this isn't right as it uses rpmvercmp() whereas this would need to compare entire EVR labels, but suffices for PoC purposes.

No functional change, but we'll need this in the next step.
@pmatilai pmatilai added the RFC label May 14, 2020
@hroncok
Copy link
Contributor

hroncok commented May 14, 2020

Thank you!

This is prerequisite for supporting version comparison in expressions.
Add support for rpm version strings as distinct class of values
in expressions, denoted by backticks (ie `1.2`) to differentiate from
regular strings.

This uses rpmvercmp() for comparison which is wrong for complete EVR
labels, but sufficient for a proof-of-concept.
@mlschroe
Copy link
Contributor

Another possibility would be the pythonish/perlish v"1.2.3".

@hroncok
Copy link
Contributor

hroncok commented May 14, 2020

Listed in #1217 (comment)

@mlschroe
Copy link
Contributor

How about supporting EVR syntax instead of that rpmvercmp() call?

@pmatilai
Copy link
Member Author

@mlschroe , as noted in the description and commit messages: "this isn't right as it uses rpmvercmp() whereas this would need to compare entire EVR labels, but suffices for PoC purposes."

@pmatilai
Copy link
Member Author

I'll try to submit a more complete version tomorrowish, there's some bulldozing in order...

@mlschroe
Copy link
Contributor

Yeah, I should read the comments instead of just looking at the "Files Changed" tab...

@voxik
Copy link
Contributor

voxik commented May 15, 2020

In Ruby, there is quite commonly used something like %q{foo}, %r{foo} to denote quoted string and regexp respectively. Therefore my suggestion would be to use %v{1.2.3} if we really need some special syntax.

But also %{version:1.2.3} wold be good enough. And maybe much better.

@hroncok
Copy link
Contributor

hroncok commented May 15, 2020

%{version:1.2.3} would be my choice as well, if only it would not be confusing with %{version}. That' why I proposed %{ver:5.6}

@pmatilai
Copy link
Member Author

pmatilai commented May 15, 2020

Folks, please keep the discussion in the ticket, this is just a PoC not intended for merging and will go away soonish. Sorry for not making this clear initially.

@pmatilai
Copy link
Member Author

(hmm, maybe we need a separate PoC label, or maybe we should just rename RFC to PoC...)

@pmatilai
Copy link
Member Author

pmatilai commented May 15, 2020

Intended to rebase this on top of #1221 for poper EVR comparison and change to the proposed v".." syntax, but seems I emptied my bucket for the day. If somebody's bored and wants to have a go, feel free. If not, I'll get back into it next week.

@pmatilai pmatilai closed this May 19, 2020
@pmatilai
Copy link
Member Author

Thanks for the feeback folks, submitted a proper version as #1233 now.

@pmatilai pmatilai deleted the vercmp-poc branch May 27, 2020 13:44
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.

None yet

4 participants