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

tests: add version-tool for comparing versions #7379

Merged
merged 8 commits into from
Sep 2, 2019

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Aug 30, 2019

I need to start comparing bash and kernel versions. I tried to hack
around in shell but it was murky and probably buggy as well. I decided
to thus write a small version comparison tool in python.

The tool can be used somewhat like the builtin "test" command. It takes
two versions, a relationship operator and an algorithm to use. An
example shell snippet using it might look like this:

if version-tool --naive "$(uname -r)" -ge 4.13; then
    ...
fi

Currently only one algorithm exist: naive, which is just enough to
compare kernel version numbers or bash version numbers. It looks through
the segments of each version, separated by dot, converts as much of the
segment to an integer as possible and computes the delta. Missing
segments are like segments reading zero, so 1.0 is equal to 1 but 1.2 is
smaller than 1.2.1. The tool comes with full array of unit tests and
documentation and is compatible with Python 3 and Python 2 alike.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

I need to start comparing bash and kernel versions. I tried to hack
around in shell but it was murky and probably buggy as well. I decided
to thus write a small version comparison tool in python.

The tool can be used somewhat like the builtin "test" command. It takes
two versions, a relationship operator and an algorithm to use. An
example shell snippet using it might look like this:

    if version-tool --naive "$(uname -r)" -ge 4.13; then
        ...
    fi

Currently only one algorithm exist: naive, which is just enough to
compare kernel version numbers or bash version numbers. It looks through
the segments of each version, separated by dot, converts as much of the
segment to an integer as possible and computes the delta. Missing
segments are like segments reading zero, so 1.0 is equal to 1 but 1.2 is
smaller than 1.2.1. The tool comes with full array of unit tests and
documentation and is compatible with Python 3 and Python 2 alike.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Why not something like "snap debug version-compare a b" and then return -1, 0, 1 via the exit code depending on the result? This could leverage the existing strutil.VersionComprae which we already use.

@zyga
Copy link
Collaborator Author

zyga commented Aug 30, 2019

@mvo5 mainly because I wanted something standalone and not muddled by the complexity of snap startup.

Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

one typo in the arguments, otherwise looks good to me. I think, while a bit counter-intuitive, it's actually better that we don't re-use the VersionCompare in go here so we can catch bugs in that with this hopefully (or the other way around perhaps)

tests/lib/bin/version-tool Outdated Show resolved Hide resolved
tests/lib/bin/version-tool Outdated Show resolved Hide resolved
tests/lib/bin/version-tool Outdated Show resolved Hide resolved
tests/lib/bin/version-tool Outdated Show resolved Hide resolved
zyga and others added 4 commits August 30, 2019 18:42
Co-Authored-By: Ian Johnson <person.uwsome@gmail.com>
The code has good unit tests for internals but not for the command
line interface. This patch corrects that.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

LGTM, nice integration test for the tool in spread

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga
Copy link
Collaborator Author

zyga commented Sep 2, 2019

I discussed this with mvo and I will make two improvements:

  • make the naive version strict
  • add output so that yes-no-error tri-outcome is possible

@mvo5
Copy link
Contributor

mvo5 commented Sep 2, 2019

Thanks @zyga ! I agree, I think we want to check that our versions look like kernel or bash versions and (ideally) error if they don't. Also I think kernel/bash after the first non version fragment because it becomes questionable what things mean then, like 1b1 gt 1a1 is a perfectly valid fragment in the Debian version schema and the result is "true" but in the naive schema its not and I think its great to be strict instead and ideally error with something like "this tool cannot parse the given version" (or somesuch). I like the output idea!

@zyga
Copy link
Collaborator Author

zyga commented Sep 2, 2019

@mvo5 I've implemented a strict checker and will push it here shortly, after a pass through spread.

tests/lib/bin/version-tool Outdated Show resolved Hide resolved
Copy link
Collaborator

@sergiocazzolato sergiocazzolato left a comment

Choose a reason for hiding this comment

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

I really like the change, the only I would like to add to the integration test (if possible) are some lines comparing core, snapd versions, like:

  • 2.41, 2.41.1, 2.41~pre1, 2.41+git425.g4d3b037
  • 16-2.41, 16-2.41.1, 16-2.41~pre1, 16-2.41+git425.g4d3b037

Thanks

tests/lib/bin/version-tool Outdated Show resolved Hide resolved
When version-tool is given something that is not a version, the --naive
algorithm might return surprising results and silently produce a
tri-state (-1, 0, +1) outcome. The new --strict algorithm will clearly
indicate an error and return a message and a distinct error code.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Michael felt strongly about it and wanted to avoid the possibility of using
it incorrectly without realising it.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga
Copy link
Collaborator Author

zyga commented Sep 2, 2019

@sergiocazzolato I removed support for non-strict versions so I can add unit tests for 2.41 and similar but things more fancy (2.41~pre2) will report an error and won't compare anywhere. If there's desire to support that I'd like to do it in a follow-up to unblock my other PR using this

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

@zyga zyga merged commit 2225bb4 into snapcore:master Sep 2, 2019
@zyga zyga deleted the feature/version-tool branch September 2, 2019 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants