Repository navigation
Conversation
Per Debian Policy, the version is split at the last hyphen: https://www.debian.org/doc/debian-policy/ch-controlfields.html#version Add tests for the ordering of '+', which is a regular non-letter sorted by its ASCII value (see Comparing version numbers on the same page).
|
This PR doesn't close any open issue. I opened it because we can't upload libwebp $ dpkg --compare-versions '1.6.0-rc1+really1.6.0-0' gt '1.6.0-rc1-0' && echo newer
neweraptly's |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1646 +/- ##
==========================================
- Coverage 77.37% 77.06% -0.32%
==========================================
Files 165 165
Lines 15747 15747
==========================================
- Hits 12185 12136 -49
- Misses 2356 2409 +53
+ Partials 1206 1202 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Probably someone should switch debian bullseye container sources to debian archive to avoid all these 404 errors. |
| } | ||
|
|
||
| i = strings.Index(ver, "-") | ||
| i = strings.LastIndex(ver, "-") |
There was a problem hiding this comment.
You sure? From lib/dpkg/parsehelp.c (https://git.dpkg.org/cgit/dpkg/dpkg.git/tree/lib/dpkg/parsehelp.c):
colon = strchr(string, ':');
if (colon) {
long epoch;
errno = 0;
epoch = strtol(string, &eepochcolon, 10);
if (string == eepochcolon)
return dpkg_put_error(err, _("epoch in version is empty"));
if (colon != eepochcolon)
return dpkg_put_error(err, _("epoch in version is not number"));
if (epoch < 0)
return dpkg_put_error(err, _("epoch in version is negative"));
if (epoch > INT_MAX || errno == ERANGE)
return dpkg_put_error(err, _("epoch in version is too big"));
if (!*++colon)
return dpkg_put_error(err, _("nothing after colon in version number"));
string = colon;
rversion->epoch = epoch;
} else {
rversion->epoch = 0;
}
rversion->version = nfstrnsave(string, end - string);
// [SEETHIS] this is the part where the revision is extracted
hyphen = strrchr(rversion->version, '-');
if (hyphen) {
*hyphen++ = '\0';
if (*hyphen == '\0')
return dpkg_put_error(err, _("revision number is empty"));
}
rversion->revision = hyphen ? hyphen : "";Look at the comment which I added with [SEETHIS] in dpkg's source, it seems to actually split at the first hyphen (unless I am missing something else).
There was a problem hiding this comment.
Looking again slightly deeper. Yeah the docs to mention that it should be splitted at last hyphen, but dpkg is doing something different internally.
The incompatibility I believe exists in dpkg's lib/dpkg/version.c:
static int
verrevcmp(const char *a, const char *b)
{
if (a == NULL)
a = "";
if (b == NULL)
b = "";
while (*a || *b) {
int first_diff = 0;
while ((*a && !c_isdigit(*a)) || (*b && !c_isdigit(*b))) {
int ac = order(*a);
int bc = order(*b);
if (ac != bc)
return ac - bc;
a++;
b++;
}
while (*a == '0')
a++;
while (*b == '0')
b++;
while (c_isdigit(*a) && c_isdigit(*b)) {
if (!first_diff)
first_diff = *a - *b;
a++;
b++;
}
if (c_isdigit(*a))
return 1;
if (c_isdigit(*b))
return -1;
if (first_diff)
return first_diff;
}
return 0;
}and aptly's CompareVersionPart deb/version.go:
func compareVersionPart(part1, part2 string) int {
i1, i2 := 0, 0
l1, l2 := len(part1), len(part2)
for {
j1, j2 := i1, i2
for j1 < l1 && !unicode.IsDigit(rune(part1[j1])) {
j1++
}
for j2 < l2 && !unicode.IsDigit(rune(part2[j2])) {
j2++
}
s1, s2 := part1[i1:j1], part2[i2:j2]
r := compareLexicographic(s1, s2)
if r != 0 {
return r
}
i1, i2 = j1, j2
for j1 < l1 && unicode.IsDigit(rune(part1[j1])) {
j1++
}
for j2 < l2 && unicode.IsDigit(rune(part2[j2])) {
j2++
}
s1, s2 = part1[i1:j1], part2[i2:j2]
n1, _ := strconv.Atoi(s1)
n2, _ := strconv.Atoi(s2)
if n1 < n2 {
return -1
}
if n1 > n2 {
return 1
}
i1, i2 = j1, j2
if i1 == l1 && i2 == l2 {
break
}
}
return 0
}There was a problem hiding this comment.
// [SEETHIS] this is the part where the revision is extracted hyphen = strrchr(rversion->version, '-');
strrchr looks for the last occurence unlike strchr which looks for the first one.
There was a problem hiding this comment.
Ah okay, makes sense then (somehow I didn't read it properly). But the test case I added back in 836d9f3 for 4.3.5a and 4.3.5rc1-1 needs to be left as is.
$ dpkg --compare-versions 4.3.5a lt 4.3.5-rc1-1; echo $?
0
So for CompareVersions() it should return 1 (-1 for left side is greater, 0 for equal, 1 for right side)
Yeah I was wrong in that commit for how the hyphen is supposed to split, but I still believe that CompareVersionPart() behaviour differs from that of dpkg's behaviour looking at how this breaks
There was a problem hiding this comment.
I'm a bit unsure about the sign here, so correct me if I'm wrong
dpkg --compare-versions 4.3.5a lt 4.3.5-rc3-1 is true, so I'd expect -1 (the old 1 passed only because of the first-hyphen split).
For the sign convention: CompareVersions("1.0", "2.0") returns -1 in aptly, and dpkg --compare-versions 1.0 lt 2.0 is true, so -1 means "first is lower" in both.
I also ran every pair from TestCompareVersions through dpkg --compare-versions manually, and with this PR all of them match dpkg (before it, three didn't: 4.3.5a vs 4.3.5-rc3-1, 1.0+a-1 vs 1.0-a-1, and 1.6.0-rc1+really1.6.0-0 vs 1.6.0-rc1-0).
3 participants
Description of the Change
Per Debian Policy, the version is split at the last hyphen: https://www.debian.org/doc/debian-policy/ch-controlfields.html#version
Add tests for the ordering of '+', which is a regular non-letter sorted by its ASCII value (see Comparing version numbers on the same page).
Why this change is important?
Checklist
AUTHORS