-
Notifications
You must be signed in to change notification settings - Fork 192
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
semantic version comparison #1400
base: master
Are you sure you want to change the base?
Conversation
a81db8b
to
d91d6cb
Compare
v1.1: address issues raised by |
d91d6cb
to
d2da1c5
Compare
v1.2: second round of PR check fixes :-) |
Thanks for your contribution! I understand your "protect against downgrades" use-case for a
Doing the bundle version comparison in RAUC itself would make it more explicit (which is good). Before reviewing the code, I'd like to discuss some high-level questions:
Even if having a A possible way to solve that would be to use the system-info handler to provide the system version. Another way would be to define that the bundle version of the booted slot is the system version. But what about systems installed from a disk image? |
:-D
our stop-gap solution so far is a rotating signing keys puts (a lot of) burden on the PKI infrastructure... which is not always feasible/easy
none so far, added it mostly for symmetry/completeness sake.
the way we're using it is system-version == bundle version; so it would depend on the definition
are you referring to the
oooh, that the slot status (= "installed bundle version") is empty after an initial bringup from a diskimage is sth i missed - thnx! |
on rethinking: those should also be ok, since they bring the or what was your train-o-thought that led to the above question? |
d2da1c5
to
654d147
Compare
v1.3: third round: addressing |
Note that for this purpose, the 'install-check' handler has an explicit 'interface' for user-visible feedback:
It's not ideal, but better than digging in logs 😏 https://rauc.readthedocs.io/en/latest/using.html#install-hooks |
Hmm. I don't think For
If you really have incompatible hardware, I'd suggest handling that via different compatible strings (or perhaps variants) instead of overloading both concepts into the same "version". See below for more.
As we've not been using this strict definition in RAUC so far, other users could be using a different approach. Spending time on good naming usually pays of in the long term. :)
No, the I meant that with the Similar considerations are the reason why we've not simply used the bundle version and installed software version interchangeably so far. But even with your approach, a bundle with a full custom handle could be used to get out of such a situation, it's just not easy or continent. :)
That only relevant for the "require intermediate updates" case and not for "protect against downgrades". We don't need to implement both cases at the same time, but we need to make sure to have clear semantics which work for both. You're right, that's the usual flow. I was thinking about: The factory originally installed So the question becomes: How do we get an "installed bundle version" on devices which have never installed a bundle. If that's even a valid concept. :) |
looking through those logs... can't see how those should be fixed :-S |
A different question: SemVer requires the version core to consist of exactly three parts. We've seen projects use date-based versions (such as You've only implemented comparison for the version core, any specific reason for that? |
Let's focus on the design questions for now. Then we can look at the implementation details. :) |
yes it would reject those, since it compares a triplet
the version core is |
https://semver.org/#spec-item-11 describes a precedence algorithm which handles these cases. We could apply the Perhaps https://manpages.debian.org/unstable/dpkg-dev/deb-version.5.en.html is also useful as inspiration, as that's less restrictive. |
we're starting to speculate possible use-cases :-D
true, that would be cleaner
ACK, since it's the system.conf which holds these -> changing it with one of the next pushes
or use the
i was wrong there: the comparison happens between system.conf <> new bundle; |
Any optional features still has costs, in maintenance, interactions with other features and complexity that needs to be documented. RAUC is opinionated and intended to discourage "incorrect" usage, so any new feature needs to have a clear use and be consistent with the rest of the design. That's not always easy. :)
A CLI or D-Bus switch wouldn't help, as these are normally not under the control of the person installing the update (except in a development scenario). For now, I'd tend to add a warning to the documentation mentioning this potential issue.
I'm confident enough that we can handle these use-cases separately, so we don't need to discuss he "require intermediate updates" case here. |
654d147
to
9a6382a
Compare
v2: changes included, based on the discussions so far:
|
9a6382a
to
eecd6db
Compare
v2.1: fixing |
eecd6db
to
9690e09
Compare
v2.2: another |
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.
I'm not convinced that the split into semver_parse
and semver_less_equal
is useful.
semver_less_equal
could encounters overflows while parsing the pre-release parts, so it would need to be able to return GErrors
as well.
What do you think about keeping the version as a string in both RaucConfig
and RaucManifest
, getting rid of semver_parse
and handling the core version by iterating through a GStrv
in semver_less_equal
, similar to how you implemented the pre-release parsing?
During config parsing, one could use semver_less_equal("0", config_version, &ierror) == TRUE
to check if the given min-bundle-version
is sensible.
thats an idea! basically pushing the whole handling (=parsing+comparing) into one place, and back to the point in time bundles are installed (or created...) -- i'll rework the PR towards that; and also pickup all the other comments&findings = thnx! |
9690e09
to
2d99888
Compare
332b097
to
736d5d1
Compare
v4: rebase onto master
|
Add a parser to decompose a semantic version [1] string into its constituents. Link: https://semver.org/ Signed-off-by: Johannes Schneider <[email protected]>
Add a utility function to do a version A <= version B comparison. Signed-off-by: Johannes Schneider <[email protected]>
736d5d1
to
4c7a5b1
Compare
v4.1: minor change: use more |
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.
The core code looks fine now.
What's still missing is test coverage for the new code in config_file.c
and install.c
. For the config, you can add two cases (good and invalid) similar to config_file_logger_full
and config_file_logger_invalid_event
in test/config_file.c
.
Testing the check during installation is more involved, so I'll probably add that myself after this is merged.
:-D
added with v4.2 :-)
wow true... managed to cobble sth together that seems to get the job done; but the boilerplate seems way thicker than it needs to be - feel free to take the blowtorch to it ;-D while putting it together, was wondering if the actual version check should be moved into this |
7fea555
to
413ae02
Compare
Add a min (semantic) version key to the system section of the config file, which can/will be used to compare the version string of an incoming bundles against during installation. Signed-off-by: Johannes Schneider <[email protected]>
Add a version comparison/check between the "core version" of the semantic version set in the bundles manifest, and the system.conf set "min-bundle-version". Signed-off-by: Johannes Schneider <[email protected]>
Add a switch to disable any version comparison/checks during installation. Signed-off-by: Johannes Schneider <[email protected]>
Change the string parsing to also allow "relaxed semantic version" parsing where not the full Major.Minor.Patch is required, but also variants with just Minor.Patch[-pre_release][+build] or Patch[-pre_release][+build] are allowed. Note that internally the core-version is still kept as as guint[3] but the missing version parts are just filled in with zero. Signed-off-by: Johannes Schneider <[email protected]>
Add test coverage for the new system.conf option: 'min-bundle-version'. Signed-off-by: Johannes Schneider <[email protected]>
Add test cases for bundle installations on a system that sets a minimum barrier. Signed-off-by: Johannes Schneider <[email protected]>
89af3c1
to
c93004f
Compare
v4.2:
test/install:
the build is "almost green", not sure where the remaining failure comes from :-S Thought/suggestion: moving the |
We're currently reworking the tests a bit (see #1401), hopefully simplifying this a bit. In general, you should use No need to try things out in this PR right now, I'll take a closer look later. |
This PR adds
min-version
andmax-version
configuration options to the system.conf, that can be set to a version triplet Major.Minor.Patch. If now bundles are versioned in a way that follows the semantic versioning scheme - e.g.1.2.3-g35f26fd
- the bundle version can be compared against these configured limits, and the installation will be (gracefully) refused should the bundle version not fall within set limits.What do you use the feature for?
updates can introduce breaking changes, which could render a device inoperable should a downgrade happen; with this feature a breaking change could advance the
min-version
and from then on refuse to install "older" bundles.How does RAUC benefit from the feature?
new feature :-D
similar feature to the compatible check, but orthogonal
How did you verify the feature works?
added tests were feasible; manually on project/target hardware
If hardware is needed for the feature, which hardware is supported and which
hardware did you test with?
no special requirements, just a hardware platform that uses rauc