-
Notifications
You must be signed in to change notification settings - Fork 61
aktualizr-lite: Fix logic for finding latest version #1247
Conversation
Trying to find a nice way to not use sscanf. Its actually doing what we need and strtol is kind of a pain for this. |
If we're confident it's fine, you can still add a lint exception, we already have some. |
I think its fine, but I'd like to add a unit test to prove it. That should also help explain what this is capable of comparing. |
Codecov Report
@@ Coverage Diff @@
## master #1247 +/- ##
==========================================
- Coverage 79.49% 79.42% -0.08%
==========================================
Files 168 169 +1
Lines 10144 10153 +9
==========================================
Hits 8064 8064
- Misses 2080 2089 +9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to also test how it deals with "known bad" versions:
"1..2"
"1.-1.3"
"2.lol.6"
"7..#.%1"
"3.&2.7.0.7.8.9"
You get the point :)
Found an even better more used way to do this. I still think the tests are handy so that we have some idea of the various permutations that can happen and what they product. |
You can't assume the targets will be ordered latest->oldest. This creates a version comparison helper that tries its best to compare things based on the glibc function: http://man7.org/linux/man-pages/man3/strverscmp.3.html Signed-off-by: Andy Doan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the two things that break CI, it looks good.
src/aktualizr_lite/CMakeLists.txt
Outdated
aktualizr_source_file_checks(main.cc) | ||
add_aktualizr_test(NAME lite-version SOURCES version_test.cc) | ||
|
||
aktualizr_source_file_checks(${SOURCES} version_test.cc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main.cc
is missing here and it causes a configure error when BUILD_OSTREE
is not defined
src/aktualizr_lite/version.h
Outdated
|
||
struct Version { | ||
std::string raw_ver; | ||
Version(const std::string& version) : raw_ver(std::move(version)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from clang-tidy:
aktualizr/src/aktualizr_lite/version.h:7:49: error: std::move of the const variable 'version' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg,-warnings-as-errors]
Version(const std::string& version) : raw_ver(std::move(version)) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about always getting clang-tidy stuff wrong. I can't seem to find a way to run clang-tidy that produces the same results as TravisCI. I've tried installing clang-6 into to docker container and I think I've also tried clang-7. From a "clean" code base both versions produces lots of warnings. I need to just sit down and figure out how I can get this to work in the standard docker container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, this version problem is indeed annoying. If you run it from Dockerfile.debian.testing
, you should be quite close from what's going on on CI.
e.g: ./scripts/run_docker_test.sh docker/Dockerfile.debian.testing
, then ninja clang-tidy
from a CMake directory or directly ./scripts/test.sh
with the flags used in the CI job.
Signed-off-by: Andy Doan <[email protected]>
Sorry it still fails, our clang-tidy setup is sometimes picky. The fix should be:
|
Signed-off-by: Andy Doan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
You can't assume the targets will be ordered latest->oldest. This
creates a version comparison helper that tries its best to compare
things.
Signed-off-by: Andy Doan [email protected]