Fix: handle special deb package versions (bsc#1150113) #1412

mbologna · 2019-09-23T16:49:25Z

What does this PR change?

Package version for RPM does not contain any '-':

susemanager=# select count(*) from rhnpackage;
 count
--------
 217621
(1 row)

susemanager=# select * from rhnpackageevr where version like '%-%';
 id | epoch | version | release | evr
----+-------+---------+---------+-----
(0 rows)

For Debian packages, some packages are named with a '-' in their
version, e.g.:

	gcc-8-base-8-20180414-1ubuntu2.amd64-deb 	gcc-8-base-8.2.0-1ubuntu2~18.04.amd64-deb
	lib32gcc1-8-20180414-1ubuntu2:1.amd64-deb 	lib32gcc1-8.2.0-1ubuntu2~18.04:1.amd64-deb
	libatomic1-8-20180414-1ubuntu2.amd64-deb 	libatomic1-8.2.0-1ubuntu2~18.04.amd64-deb
	libcc1-0-8-20180414-1ubuntu2.amd64-deb 		libcc1-0-8.2.0-1ubuntu2~18.04.amd64-deb
	libgcc1-8-20180414-1ubuntu2:1.amd64-deb 	libgcc1-8.2.0-1ubuntu2~18.04:1.amd64-deb
	libgomp1-8-20180414-1ubuntu2.amd64-deb 		libgomp1-8.2.0-1ubuntu2~18.04.amd64-deb
	libitm1-8-20180414-1ubuntu2.amd64-deb 		libitm1-8.2.0-1ubuntu2~18.04.amd64-deb
	liblsan0-8-20180414-1ubuntu2.amd64-deb 		liblsan0-8.2.0-1ubuntu2~18.04.amd64-deb
	libmpx2-8-20180414-1ubuntu2.amd64-deb 		libmpx2-8.2.0-1ubuntu2~18.04.amd64-deb
	libquadmath0-8-20180414-1ubuntu2.amd64-deb 	libquadmath0-8.2.0-1ubuntu2~18.04.amd64-deb
	libstdc++6-8-20180414-1ubuntu2.amd64-deb 	libstdc++6-8.2.0-1ubuntu2~18.04.amd64-deb
	libtsan0-8-20180414-1ubuntu2.amd64-deb 		libtsan0-8.2.0-1ubuntu2~18.04.amd64-deb

The version is correctly parsed:

susemanager=# select * from rhnpackageevr where id in (select evr_id from rhnpackage where name_id in (select id from rhnpackagename where name like '%gcc-8-base%'));
  id  | epoch |  version   |     release      |            evr
------+-------+------------+------------------+---------------------------
  316 |       | 8-20180414 | 1ubuntu2         | (,8-20180414,1ubuntu2)
 1835 |       | 8.3.0      | 6ubuntu1~18.04.1 | (,8.3.0,6ubuntu1~18.04.1)
(2 rows)

This PR adds a special check in the version compare algorithm: if it contains a '-', then we need to check for the upstream_version of the Debian package. If the upstream_version is equal, then we need to compare for the second part of the string (the part after the '-') and compare the debian_revision.

Test coverage

Unit tests were added
DONE

Links

Fixes https://github.com/SUSE/spacewalk/issues/9382
Ports:

Manager-4.0: https://github.com/SUSE/spacewalk/pull/9657
Manager-3.2: https://github.com/SUSE/spacewalk/pull/9656
DONE

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

mbologna · 2019-09-23T16:51:01Z

Added JUnit should cover the Java part
For the SQL part:

old:

susemanager=# select * from rpm.rpmstrcmp('8-20180414', '8.3.0');
 rpmstrcmp 
-----------
         1
(1 row)

new:

susemanager=# select * from rpm.rpmstrcmp('8-20180414', '8.3.0');
 rpmstrcmp 
-----------
        -1
(1 row)

paususe · 2019-09-23T16:56:14Z

I do not like this solution.

Why do we need to replace - with ~?

That will cause trouble, as - is valid in package name and version number but ~ is only valid in version number

mbologna · 2019-09-23T16:59:52Z

I do not like this solution.

Why do we need to replace - with ~?

That will cause trouble, as - is valid in package name and version number but ~ is only valid in version number

We currently have one solver that is targeting RPM packages. To handle these special Debian packages that have '-' in their version, we replace '-' with a ~ just in the comparing algorithm; for RPM packages, tilde is allowed and is treated as special.

paususe · 2019-09-24T16:53:17Z

I do not like this solution.
Why do we need to replace - with ~?
That will cause trouble, as - is valid in package name and version number but ~ is only valid in version number

We currently have one solver that is targeting RPM packages. To handle these special Debian packages that have '-' in their version, we replace '-' with '~~' just in the comparing algorithm; for RPM packages, '~~' is allowed and is treated as special.

99% of Debian packages have "-" in their version (e. g. git 1:2.23.0-1). The exception is Debian-native software (e. g. dpkg 1.19.7). I guess you meant "special Debian packages that have '-' in their name".

But the fact that you have a '-' in the name or in the version does not mean it's a pre-release version. Furthermore, the \d-\d{8} pattern is completely arbitrary and cannot be relied upon.

A few examples of packages which IMHO this PR may fail:

spigot_0.2017-01-15.gdad1bbc6-1_arm64.deb => Upstream version is 0.2017-01-15.gdad1bbc6, which does not match \d-\d{8}, therefore '-' will not be replaced by '~'
candid_1.0.0~alpha+201804191824-24b36a9-0ubuntu2_arm64.deb => Upstream version is 1.0.0~alpha+201804191824-24b36a9, which does not match \d-\d{8}, therefore '-' will not be replaced by '~'
virtualbox-guest-additions-iso_5.2.11-122181-1_all.deb => Upstream version is 5.2.11-122181, which does not match \d-\d{8}, therefore '-' will not be replaced by '~'

I am attaching to this issue:

The Packages.gz files for all the sections and updates in Ubuntu 18.04 arm (the OS where we got the original bug report) : bionic, bionic-updates, bionic-backports, bionic-security, etc.
The full list of the 81,574 packages extracted from the above Packages.gz files - debversiontrouble-packages.txt. It can be generated with for I in Packages*.gz; do zgrep Filename "$I";done | sed 's|.*/$.*$|\1|' > debversiontrouble-packages.txt.
From the above list, the 298 version numbers which contain a hyphen - debversiontrouble-versions.txt. It can be generated with for I in Packages*.gz; do zgrep Filename "$I";done | sed 's|.*/$.*$|\1|' | cut -d '_' -f 2 | sed 's|$.*$-.*|\1|' | sort | uniq | grep '[[:digit:]]-[[:digit:]]' > debversiontrouble-versions.txt.
From the above list, the 283 version numbers which contain a hyphen but do not match \d-\d{8} - debversiontrouble-unmatchedbydd-dd8.txt. It can be generated with for I in Packages*.gz; do zgrep Filename "$I";done | sed 's|.*/$.*$|\1|' | cut -d '_' -f 2 | sed 's|$.*$-.*|\1|' | sort | uniq | grep '[[:digit:]]-[[:digit:]]' | egrep -v '[[:digit:]]-[[:digit:]]{8}' > debversiontrouble-unmatchedbydd-dd8.txt.

If you now run while read -r line; do grep _$line- debversiontrouble-packages.txt ; done < debversiontrouble-unmatchedbydd-dd8.txt, you'll find 605 packages where this PR may fail.

This is what the Debian Policy says:

upstream_version

This is the main part of the version number. It is usually the version number of the original (“upstream”) package from which the .deb file has been made, if this is applicable. Usually this will be in the same format as that specified by the upstream author(s); however, it may need to be reformatted to fit into the package management system’s format and comparison scheme.

The comparison behavior of the package management system with respect to the upstream_version is described below. The upstream_version portion of the version number is mandatory.

The upstream_version may contain only alphanumerics [6] and the characters . + - ~ (full stop, plus, hyphen, tilde) and should start with a digit. If there is no debian_revision then hyphens are not allowed.

https://www.debian.org/doc/debian-policy/ch-controlfields.html#version

It does not say '-' and '~' are equivalent or can be intercharged.

By replacing "-" with "~" for some arbitrary pattern, we are making sure we will fail in some corner cases. Please, do not do this. The 0.7% of the cases this PR may fail are enough to give us a lot of headaches and upset customers.

Debian packages have their own version comparison algorithm, which is documented in the dpkg manpage:

Sorting algorithm

The upstream-version and debian-revision parts are compared by the package management system using the same algorithm:

The strings are compared from left to right.

First the initial part of each string consisting entirely of non-digit characters is determined. These two parts (one of which may be empty) are compared lexically. If a difference is found it is returned. The lexical comparison is a comparison of ASCII values modified so that all the letters sort earlier than all the non-letters and so that a tilde sorts before anything, even the end of a part. For example, the following parts are in sorted order: ‘~~’, ‘~~a’, ‘~’, the empty part, ‘a’.

Then the initial part of the remainder of each string which consists entirely of digit characters is determined. The numerical values of these two parts are compared, and any difference found is returned as the result of the comparison. For these purposes an empty string (which can only occur at the end of one or both version strings being compared) counts as zero.

These two steps (comparing and removing initial non-digit strings and initial digit strings) are repeated until a difference is found or both strings are exhausted.

Note that the purpose of epochs is to allow us to leave behind mistakes in version numbering, and to cope with situations where the version numbering scheme changes. It is not intended to cope with version numbers containing strings of letters which the package management system cannot interpret (such as ‘ALPHA’ or ‘pre-’), or with silly orderings.

https://dyn.manpages.debian.org/buster/dpkg-dev/deb-version.7.en.html

debversiontrouble-packages.txt
debversiontrouble-unmatchedbydd-dd8.txt
debversiontrouble-versions.txt
Packages.gz
Packages(1).gz
Packages(2).gz
Packages(3).gz
Packages(4).gz
Packages(5).gz
Packages(6).gz
Packages(7).gz
Packages(8).gz
Packages(9).gz
Packages(10).gz
Packages(11).gz
Packages(12).gz
Packages(13).gz
Packages(14).gz
Packages(15).gz
Packages(16).gz
Packages(17).gz
Packages(18).gz
Packages(19).gz
Packages(20).gz

paususe

I'm afraid this will not work, see my comment here: #1412 (comment)

mcalmer · 2019-09-25T09:29:45Z

I think correct would be to split at every "-" dash and compare the parts until we find one which is not equal.

For RPM there are by definition only 2 (version and release) while debian can have more.

mbologna · 2019-09-25T11:36:08Z

The latest push fixes everything: I just have to deal with:

        assertCompareSymm(-1, "8.0.9", "a.8.0.9-22");

This is different from RPM: in Deb "The lexical comparison is a comparison of ASCII values modified so that all the letters sort earlier than all the non-letters"

mbologna · 2019-09-25T13:04:20Z

The latest push fixes everything: I just have to deal with:
        assertCompareSymm(-1, "8.0.9", "a.8.0.9-22");
This is different from RPM: in Deb "The lexical comparison is a comparison of ASCII values modified so that all the letters sort earlier than all the non-letters"

This case cannot be handled without knowing if the version is related to an RPM or a deb package.

mbologna · 2019-09-25T13:19:41Z

@mcalmer please review the algorithm I implemented here: I couldn't find any version that breaks mine and works with your version (and vice-versa).
In my opinion, the non-recursive version (this) should be more efficient.

What I propose: let's adopt one of the two solutions, test it with a PTF and stop the fire. Then, create an issue to address the fact that we need a DebVersionComparator as per #1412 (comment)

Still to be done:

Approve the algorithm
Write the plsql code
Write the migration
Produce PTF
Merge the PR
Create follow-up issue to implement Debian compare algorithm that covers the corner case Fix: handle special deb package versions (bsc#1150113) #1412 (comment)

mcalmer · 2019-09-25T13:35:20Z

I agree that we should try yours. But you need to adapt also the database compare mechanism.

paususe · 2019-09-25T16:33:13Z

This is the test set a fully-compliant implementation must pass:
https://git.dpkg.org/cgit/dpkg/dpkg.git/tree/scripts/t/Dpkg_Version.t

paususe · 2019-09-25T17:23:58Z

Also, will a more complete comparison algorithm require re-importing all the packages in the database?

mbologna · 2019-09-26T09:10:41Z

Also, will a more complete comparison algorithm require re-importing all the packages in the database?

I think that re-importing will not be required. This algorithm is used when comparing packages (e.g. installed vs proposed updates).

This is the test set a fully-compliant implementation must pass:
https://git.dpkg.org/cgit/dpkg/dpkg.git/tree/scripts/t/Dpkg_Version.t

I collected all the 43 tests published here and added in our JUnit tests, stripping the epoch from the versions (the comparator of this PR just compares versions, not epoch - that is done before this comparator).
Out of 43 tests, we are returning a wrong return code for 4 cases (9%). Those test cases are checked-in commented out.

My proposal:

@mcalmer : final review
@mbologna : Push the PTF
@mbologna : If both of the above OK, merge to master, Manager-4.0 and Manager-3.2
@mbologna : Create a follow-up issue to implement Debian compare algorithm that covers all the cases

mcalmer · 2019-09-26T09:38:24Z

@mbologna have you tested the PSQL version somehow manually ? That would be good to see if both have the same result

mcalmer · 2019-09-26T09:52:43Z

@paususe I have no idea how we could implement a second algorythm. For java it might be possible, but for PSQL we have no identifier which algorithm should be used.

We would need to have an extra flag in the EVR type and this would require to change all the existing types . If we have the case that a Debian and a RPM package share the same EVR, we need to split them in 2 with types and re-assign one package. A lot of errors can happen when we try this.

paususe · 2019-09-26T10:02:50Z

@paususe I have no idea how we could implement a second algorythm. For java it might be possible, but for PSQL we have no identifier which algorithm should be used.

We would need to have an extra flag in the EVR type and this would require to change all the existing types . If we have the case that a Debian and a RPM package share the same EVR, we need to split them in 2 with types and re-assign one package. A lot of errors can happen when we try this.

Unfortunately, a lot of errors will happen if we do not do this. We will need to add some flag in there.

Also: why is the version comparison implementation split in a Java part and a DB part?

mbologna · 2019-09-26T10:21:54Z

@mbologna have you tested the PSQL version somehow manually ? That would be good to see if both have the same result

For our case, yes: #1412 (comment)
For the Debian tests: 8 errors (4 more than the Java part): the added algorithm is identical, I suspect there is a difference in the legacy algorithm between the Java and the SP one.

Details: grep for KO:

OK susemanager=# select * from rpm.rpmstrcmp('1.0-1', '2.0-2');
 rpmstrcmp                                                  
-----------
        -1 
(1 row)   
       
OK susemanager=# select * from rpm.rpmstrcmp('2.2~rc-4', '2.2-1');
 rpmstrcmp                                                  
-----------
        -1 
(1 row)   

OK susemanager=# select * from rpm.rpmstrcmp('2.2-1', '2.2~rc-4');
 rpmstrcmp   
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.0000-1', '1.0-1');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1', '1');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0', '0-0');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('2.5', '7.5');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo', '0foo');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo-0', '0foo');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo', '0foo-0');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo', '0fo');
 rpmstrcmp 
-----------
         1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo-0', '0foo+');
 rpmstrcmp 
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo~1', '0foo');
 rpmstrcmp
-----------
        -1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo~foo+Bar', '0foo~foo+bar');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo~~', '0foo~');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1~', '1');
 rpmstrcmp
-----------
        -1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('12345+that-really-is-some-ver-0', '12345+that-really-is-some-ver-10');
 rpmstrcmp
-----------
         0
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo-0', '0foo-01');
 rpmstrcmp
-----------
         0
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo.bar', '0foobar');
 rpmstrcmp 
-----------
        -1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo.bar', '0foo1bar');
 rpmstrcmp 
-----------
        -1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('0foo.bar', '0foo0bar');
 rpmstrcmp 
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo1bar-1', '0foobar-1');
 rpmstrcmp                                                            
-----------  
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo2.0', '0foo2');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo2.0.0', '0foo2.10.0');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo2.0', '0foo2.0.0');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo2.0', '0foo2.10');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0foo2.1', '0foo2.10');
 rpmstrcmp
-----------
        -1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.09', '1.9');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.0.8+nmu1', '1.0.8');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('3.11', '3.10+nmu1');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0.9j-20080306-4', '0.9i-20070324-2');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.2.0~b7-1', '1.2.0~b6-1');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.011-1', '1.06-2');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0.0.9+dfsg1-1', '0.0.8+dfsg1-3');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('4.6.99+svn6582-1', '4.6.99+svn6496-1');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('53', '52');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('0.9.9~pre122-1', '0.9.9~pre111-1');
 rpmstrcmp
-----------
         1
(1 row)

KO susemanager=# select * from rpm.rpmstrcmp('2.3.2-2+lenny2', '2.3.2-2');
 rpmstrcmp
-----------
         0
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('3.8.1-1', '3.8.GA-1');
 rpmstrcmp
-----------
         1

(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1.0.1+gpl-1', '1.0.1-2');
 rpmstrcmp
-----------
         1
(1 row)

OK susemanager=# select * from rpm.rpmstrcmp('1a', '1000a');
 rpmstrcmp
-----------
        -1
(1 row)

mcalmer · 2019-09-26T10:26:11Z

Also: why is the version comparison implementation split in a Java part and a DB part?

AFAIK it was only in PSQL because the algorithm was needed in java, python and perl in the good old times of spacewalk.

perl went away and I am not sure if python code use it. Some new code was added which just need that calculation in Java and the Java implementation was born.

mbologna · 2019-09-26T10:35:18Z

Also: why is the version comparison implementation split in a Java part and a DB part?

AFAIK it was only in PSQL because the algorithm was needed in java, python and perl in the good old times of spacewalk.

perl went away and I am not sure if python code use it. Some new code was added which just need that calculation in Java and the Java implementation was born.

The SP is still also used in the Java part - db queries: https://github.com/uyuni-project/uyuni/blob/master/java/code/src/com/redhat/rhn/common/db/datasource/xml/Package_queries.xml#L1407

mcalmer · 2019-09-26T10:47:51Z

The SP is still also used in the Java part - db queries: https://github.com/uyuni-project/uyuni/blob/master/java/code/src/com/redhat/rhn/common/db/datasource/xml/Package_queries.xml#L1407

Oh, this is calling it explicite, but we have also these cases:

https://github.com/uyuni-project/uyuni/blob/master/java/code/src/com/redhat/rhn/common/db/datasource/xml/Package_queries.xml#L1182

mbologna · 2019-09-26T15:13:50Z

@paususe - I consulted with @mcalmer
Here is what we discussed.

This PR improves the current situation where we have a problem and an L3 with a customer.
Still, it is not the best solution but at least it would solve the problem for the customer if we ship it.

The complete solution (that fixes all the cases) would require multiple sprints as we need to research around the following topics:

Postgres: can a type evr_t be enriched with an additional parameter o type?
Do we need to drop the table to create an additional type? (This might have the ramification of involving re-syncing all the packages as you asked)
We need to fix the stored procedure in the database and create another type to select the comparator
Implement the new Debian package comparator
Regression testing

This is something that targets 4.1 as it involves a lot of research and development.
Our customer might not want to wait until 4.1 is out to solve his problem.

What I suggest:

provide the PTF for the customer
follow up with a research issue

paususe · 2019-09-26T16:54:21Z

* Postgres: can a type `evr_t` be enriched with an additional parameter o type?

I guess yes, it's defined in here:
https://github.com/uyuni-project/uyuni/blob/master/schema/spacewalk/postgres/class/evr_t.sql

* Do we need to drop the table to create an additional type? (This might have the ramification of involving re-syncing all the packages as you asked)

See above, I'd say no.

* We need to fix the stored procedure in the database and create another type to select the comparator

But that does not seem too difficult if we get the package type (deb vs rpm) in evr_t:
https://github.com/uyuni-project/uyuni/blob/master/schema/spacewalk/postgres/class/evr_t.sql#L32

* Implement the new Debian package comparator

A PL/SQL type, comparison operator, etc is available from Debian itself:
https://salsa.debian.org/postgresql/postgresql-debversion

Specifically:
https://salsa.debian.org/postgresql/postgresql-debversion/blob/master/debversion--1.1.sql

* Regression testing

We will need to do that even after the PTF (in fact, we should do it before the PTF, as this may introduce many new problems).

This is something that targets 4.1 as it involves a lot of research and development.
Our customer might not want to wait until 4.1 is out to solve his problem.

What I suggest:
* provide the PTF for the customer

* follow up with a research issue

In light of my comments above, do you think it would be possible to implement the right solution now?

mcalmer · 2019-09-27T15:02:33Z

* Postgres: can a type `evr_t` be enriched with an additional parameter o type?
I guess yes, it's defined in here:
https://github.com/uyuni-project/uyuni/blob/master/schema/spacewalk/postgres/class/evr_t.sql

Changing it for a new installation is easy, but in a running system with thousends of enties converting it without loosing data is the challenge. And I am not sure of postgresql allows to "alter" a type.

* Implement the new Debian package comparator
A PL/SQL type, comparison operator, etc is available from Debian itself:
https://salsa.debian.org/postgresql/postgresql-debversion

Specifically:
https://salsa.debian.org/postgresql/postgresql-debversion/blob/master/debversion--1.1.sql

Hmm, the C++ part if the important one. We would need to build an extension package but that should be possible.

In light of my comments above, do you think it would be possible to implement the right solution now?

Hmm, It could still become a nightmare. So the question is: what is more important - CaaSP 4 or this?

paususe · 2019-09-27T15:34:10Z

* Postgres: can a type `evr_t` be enriched with an additional parameter o type?
I guess yes, it's defined in here:
https://github.com/uyuni-project/uyuni/blob/master/schema/spacewalk/postgres/class/evr_t.sql
Changing it for a new installation is easy, but in a running system with thousends of enties converting it without loosing data is the challenge. And I am not sure of postgresql allows to "alter" a type.

Who can enlighten us on this?

* Implement the new Debian package comparator
A PL/SQL type, comparison operator, etc is available from Debian itself:
https://salsa.debian.org/postgresql/postgresql-debversion
Specifically:
https://salsa.debian.org/postgresql/postgresql-debversion/blob/master/debversion--1.1.sql
Hmm, the C++ part if the important one. We would need to build an extension package but that should be possible.

C++ part in Uyuni?

There is also an implementation of the comparison in C in dpkg:
https://www.dpkg.org/doc/libdpkg/group__version.html#ga22409ddc0cc1a7590351444342c8b9d5
https://salsa.debian.org/dpkg-team/dpkg/blob/master/lib/dpkg/version.c#L140

In light of my comments above, do you think it would be possible to implement the right solution now?

Hmm, It could still become a nightmare. So the question is: what is more important - CaaSP 4 or this?

Let's proceed with Michele's proposed fix and the CaaSP4 RFC for now but also do the complete fix in 4.0. Each case our version comparison algorithm is not 100% exact is a potential nightmare.

Package version for RPM does not contain any '-': ``` susemanager=# select count(*) from rhnpackage; count -------- 217621 (1 row) susemanager=# select * from rhnpackageevr where version like '%-%'; id | epoch | version | release | evr ----+-------+---------+---------+----- (0 rows) ``` For Debian packages, some packages are named with a '-' in their version, e.g.: ``` gcc-8-base-8-20180414-1ubuntu2.amd64-deb gcc-8-base-8.2.0-1ubuntu2~18.04.amd64-deb lib32gcc1-8-20180414-1ubuntu2:1.amd64-deb lib32gcc1-8.2.0-1ubuntu2~18.04:1.amd64-deb libatomic1-8-20180414-1ubuntu2.amd64-deb libatomic1-8.2.0-1ubuntu2~18.04.amd64-deb libcc1-0-8-20180414-1ubuntu2.amd64-deb libcc1-0-8.2.0-1ubuntu2~18.04.amd64-deb libgcc1-8-20180414-1ubuntu2:1.amd64-deb libgcc1-8.2.0-1ubuntu2~18.04:1.amd64-deb libgomp1-8-20180414-1ubuntu2.amd64-deb libgomp1-8.2.0-1ubuntu2~18.04.amd64-deb libitm1-8-20180414-1ubuntu2.amd64-deb libitm1-8.2.0-1ubuntu2~18.04.amd64-deb liblsan0-8-20180414-1ubuntu2.amd64-deb liblsan0-8.2.0-1ubuntu2~18.04.amd64-deb libmpx2-8-20180414-1ubuntu2.amd64-deb libmpx2-8.2.0-1ubuntu2~18.04.amd64-deb libquadmath0-8-20180414-1ubuntu2.amd64-deb libquadmath0-8.2.0-1ubuntu2~18.04.amd64-deb libstdc++6-8-20180414-1ubuntu2.amd64-deb libstdc++6-8.2.0-1ubuntu2~18.04.amd64-deb libtsan0-8-20180414-1ubuntu2.amd64-deb libtsan0-8.2.0-1ubuntu2~18.04.amd64-deb ``` The version is correctly parsed: ``` susemanager=# select * from rhnpackageevr where id in (select evr_id from rhnpackage where name_id in (select id from rhnpackagename where name like '%gcc-8-base%')); id | epoch | version | release | evr ------+-------+------------+------------------+--------------------------- 316 | | 8-20180414 | 1ubuntu2 | (,8-20180414,1ubuntu2) 1835 | | 8.3.0 | 6ubuntu1~18.04.1 | (,8.3.0,6ubuntu1~18.04.1) (2 rows) ``` This PR adds a special check in the version compare algorithm: if it contains a '-', then we need to check for the `upstream_version` of the Debian package. If the `upstream_version` is equal, then we need to compare for the second part of the string (the part after the '-') and compare the `debian_revision`.

mbologna force-pushed the master-bsc1150113 branch from 138fc18 to 4f57ae0 Compare September 23, 2019 16:55

paususe requested review from paususe and mcalmer September 24, 2019 16:56

paususe suggested changes Sep 24, 2019

View reviewed changes

mbologna force-pushed the master-bsc1150113 branch 2 times, most recently from 354999d to b29b792 Compare September 25, 2019 11:39

mbologna force-pushed the master-bsc1150113 branch 5 times, most recently from 14e7b63 to 35f4703 Compare September 25, 2019 15:36

mbologna marked this pull request as ready for review September 25, 2019 15:40

mbologna force-pushed the master-bsc1150113 branch 2 times, most recently from c2014ca to 50e2e6f Compare September 25, 2019 15:44

mbologna force-pushed the master-bsc1150113 branch from 50e2e6f to a5724e6 Compare September 26, 2019 08:23

mcalmer approved these changes Sep 26, 2019

View reviewed changes

mbologna force-pushed the master-bsc1150113 branch from 987b78e to 7c1fa05 Compare September 26, 2019 16:25

paususe approved these changes Sep 30, 2019

View reviewed changes

mbologna mentioned this pull request Oct 2, 2019

Uyuni 4.0.2 - strange behaviour with Ubuntu 18.04 and channels from spacewalk-common-channels #1277

Open

mbologna changed the title ~~Fix: handle special deb package names (bsc#1150113)~~ Fix: handle special deb package versions (bsc#1150113) Oct 2, 2019

mbologna added 7 commits October 2, 2019 12:56

Tests: add JUnit testing

8b8b659

Fix: add db sp with new compare algo

5d37081

Chore: announce changes in changelog

ede77b7

Fix: handle special case with only one upstream_version

08c6f6c

Test: remove duplicated test

f620a7c

Test: add util to test deb version package comparison

31f34ab

mbologna force-pushed the master-bsc1150113 branch from 7c1fa05 to 31f34ab Compare October 2, 2019 10:57

mbologna merged commit 02da29e into uyuni-project:master Oct 2, 2019

mbologna deleted the master-bsc1150113 branch October 2, 2019 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: handle special deb package versions (bsc#1150113) #1412

Fix: handle special deb package versions (bsc#1150113) #1412

mbologna commented Sep 23, 2019 •

edited

Loading

mbologna commented Sep 23, 2019

paususe commented Sep 23, 2019

mbologna commented Sep 23, 2019 •

edited

Loading

paususe commented Sep 24, 2019 •

edited

Loading

paususe left a comment

mcalmer commented Sep 25, 2019

mbologna commented Sep 25, 2019

mbologna commented Sep 25, 2019

mbologna commented Sep 25, 2019 •

edited

Loading

mcalmer commented Sep 25, 2019

paususe commented Sep 25, 2019

paususe commented Sep 25, 2019

mbologna commented Sep 26, 2019 •

edited

Loading

mcalmer commented Sep 26, 2019

mcalmer commented Sep 26, 2019

paususe commented Sep 26, 2019

mbologna commented Sep 26, 2019

mcalmer commented Sep 26, 2019

mbologna commented Sep 26, 2019

mcalmer commented Sep 26, 2019

mbologna commented Sep 26, 2019

paususe commented Sep 26, 2019 •

edited

Loading

mcalmer commented Sep 27, 2019 •

edited

Loading

paususe commented Sep 27, 2019

Fix: handle special deb package versions (bsc#1150113) #1412

Fix: handle special deb package versions (bsc#1150113) #1412

Conversation

mbologna commented Sep 23, 2019 • edited Loading

What does this PR change?

Test coverage

Links

Re-run a test

mbologna commented Sep 23, 2019

paususe commented Sep 23, 2019

mbologna commented Sep 23, 2019 • edited Loading

paususe commented Sep 24, 2019 • edited Loading

paususe left a comment

Choose a reason for hiding this comment

mcalmer commented Sep 25, 2019

mbologna commented Sep 25, 2019

mbologna commented Sep 25, 2019

mbologna commented Sep 25, 2019 • edited Loading

mcalmer commented Sep 25, 2019

paususe commented Sep 25, 2019

paususe commented Sep 25, 2019

mbologna commented Sep 26, 2019 • edited Loading

mcalmer commented Sep 26, 2019

mcalmer commented Sep 26, 2019

paususe commented Sep 26, 2019

mbologna commented Sep 26, 2019

mcalmer commented Sep 26, 2019

mbologna commented Sep 26, 2019

mcalmer commented Sep 26, 2019

mbologna commented Sep 26, 2019

paususe commented Sep 26, 2019 • edited Loading

mcalmer commented Sep 27, 2019 • edited Loading

paususe commented Sep 27, 2019

mbologna commented Sep 23, 2019 •

edited

Loading

mbologna commented Sep 23, 2019 •

edited

Loading

paususe commented Sep 24, 2019 •

edited

Loading

mbologna commented Sep 25, 2019 •

edited

Loading

mbologna commented Sep 26, 2019 •

edited

Loading

paususe commented Sep 26, 2019 •

edited

Loading

mcalmer commented Sep 27, 2019 •

edited

Loading