Add Debian Testing to the tests #356

harshula · 2023-03-07T05:45:56Z

Debian Testing's version metadata is a different format to the Debian Release version metadata. I hope having this in your test suite will be helpful.

I found that function version() in v1.6.0 of distro was returning:

(Pdb) p versions
['', 'n/a', '', '', '', '']

in https://github.com/spack/spack/ v0.19.1. Spack was then taking the first element of "n/a" (i.e. "n") and using it at as the version!

HorlogeSkynet · 2023-03-11T10:48:51Z

Hi @harshula ! Thanks for your PR.

Could the lsb_release script you add be simpler (as the ones already present among other distros tests, i.e. Debian 8) ? 🙂

hartwork · 2023-03-11T17:11:42Z

@harshula I would like to second what @HorlogeSkynet is asking. We need a test dummy here, not a real lsb_release implementation, unfortunately.

harshula · 2023-03-12T07:52:47Z

Hi @HorlogeSkynet & @hartwork Apologies, I've pushed the commit again with the correction.

HorlogeSkynet · 2023-03-12T08:01:40Z

Thanks for the update @harshula !

I wonder whether we should test/fix the n/a case (see OP) here too. WDYT @hartwork ? 🙂

EDIT : maybe something like (pretty naive) :

         if best:
             # This algorithm uses the last version in priority order that has
             # the best precision. If the versions are not in conflict, that
             # does not matter; otherwise, using the last one instead of the
             # first one might be considered a surprise.
             for v in versions:
-             if v.count(".") > version.count(".") or version == "":
+             if v.count(".") > version.count(".") or version.casefold() in ("", "n/a"):
                     version = v
         else:
             for v in versions:
-                 if v != "":
+                 if v.casefold() not in ("", "n/a"):
                     version = v
                     break

harshula · 2023-03-12T09:53:32Z

Hi @HorlogeSkynet, Your patch results in:

(Pdb) p distname, version
('debian', 'bookworm/sid')

Perhaps, consider having a filter/mapping function that stops characters like "/", ";", etc being returned?

Without the patch:

(Pdb) p distname, version
('debian', 'n/a')

Thanks!

HorlogeSkynet · 2023-03-12T10:21:35Z

Perhaps, consider having a filter/mapping function that stops characters like "/", ";", etc being returned?

I get your point but I fear this would end up in a "cat and mouse" issue... N/A literal is pretty explicit though. Let's wait for @hartwork inputs on this 😉

hartwork · 2023-03-12T20:09:10Z

@HorlogeSkynet @harshula given that for Debian bullseye version is "11"…

# lsb_release -a 2>/dev/null | grep Release
Release:        11

# distro -j | grep -w version
    "version": "11",

…I believe that "n/a" is better here than "bookworm" or "bookworm/sid" because that is the codename of the (major) version, not a version. If master is serving n/a for a version then maybe that's good and users can check for that value to know they have sid if Debian. What do you think?

harshula · 2023-03-13T00:03:51Z

Hi @hartwork , Debian Testing does not have an integer version. The most accurate way to describe the current Debian Testing is that it is a mixture of the next release ("bookworm") and unstable ("sid"). Therefore, "bookworm/sid" is the most accurate option.

Once Debian 12 (bookworm) is released, Debian Testing is most accurately described as "trixie/sid".

hartwork · 2023-03-13T00:21:41Z

@harshula I didn't think of testing earlier but I'm getting identical cat /etc/os-release output from Debian containers for testing and unstable now. I guess that means that testing and unstable cannot be told apart without looking into /etc/apt/sources.list*?

harshula · 2023-03-13T02:07:35Z

Hi @hartwork , Debian's behaviour, w.r.t. Testing and Unstable, apparently changed around September 2022. Prior to that, my understanding is that distro.py was returning "testing" on Debian Testing. I'm now trying to understand what happened.

harshula · 2023-03-13T08:04:58Z

Hi @hartwork, The previous lsb_release (https://sources.debian.org/src/lsb/11.1.0/lsb_release/) written in Python outputs:

$ ./lsb_release -a
No LSB modules are available.
Distributor ID:	Debian
Description:	Debian GNU/Linux bookworm/sid
Release:	testing
Codename:	bookworm

harshula · 2023-03-13T09:01:18Z

Background: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1008735

harshula · 2023-03-13T10:25:22Z

I guess that means that testing and unstable cannot be told apart without looking into /etc/apt/sources.list*?

There's a guess_release_from_apt() function in the old version.

hartwork · 2023-03-13T17:16:39Z

@harshula I think it's worth pointing out that both the old and the new version have two rather similar Python scripts:

Old

New

The call to guess_release_from_apt seems to be in both versions but only on the .py file side of things.

However that's not the code used by the lsb_release command on Debian, that would be package lsb-release rather than lsb. The lsb version says Release: testing but lsb_release says Release: n/a. To reproduce inside docker run --rm -it debian:testing bash -i:

# apt-get update && apt-get install --yes wget ca-certificates python3 distro-info-data
# wget https://sources.debian.org/data/main/l/lsb/11.1.0/lsb_release
# wget https://sources.debian.org/data/main/l/lsb/11.1.0/lsb_release.py
# python3 ./lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux bookworm/sid
Release:        testing
Codename:       bookworm
# python3 ./lsb_release.py
{'ID': 'Debian', 'OS': 'GNU/Linux', 'DESCRIPTION': 'Debian GNU/Linux bookworm/sid', 'RELEASE': 'testing', 'CODENAME': 'bookworm'}
[]
# rm lsb_release lsb_release.py
# wget https://sources.debian.org/data/main/l/lsb/11.6/lsb_release.py
# wget https://sources.debian.org/data/main/l/lsb/11.6/lsb_release
# python3 ./lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux bookworm/sid
Release:        testing
Codename:       bookworm
# python3 ./lsb_release.py
{'ID': 'Debian', 'OS': 'GNU/Linux', 'DESCRIPTION': 'Debian GNU/Linux bookworm/sid', 'RELEASE': 'testing', 'CODENAME': 'bookworm'}
[]
# rm lsb_release lsb_release.py
# wget https://salsa.debian.org/gioele/lsb-release-minimal/-/raw/master/lsb_release
# bash ./lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux bookworm/sid
Release:        n/a
Codename:       bookworm

What do you think?

harshula · 2023-03-13T23:56:44Z

I think it's worth pointing out that both the old and the new version have two rather similar Python scripts:

Hi @hartwork , No, the new version is a shell script: https://sources.debian.org/src/lsb-release-minimal/12.0-1/lsb_release/ . The shell script does not attempt to infer from apt.

hartwork · 2023-03-14T00:00:32Z

@harshula that matches what I said above. We're on the same page then 😃

harshula · 2023-03-14T00:01:40Z

What do you think?

What's your specific question?

hartwork · 2023-03-14T00:15:46Z

@harshula I was trying to ask for suggestions, for your view, for things I maybe was misunderstanding or missing. My own current view is that this may need a fix in Debian not distro. What is your view?

harshula · 2023-03-14T01:18:12Z

Hi @hartwork,

This PR was opened to add Debian Testing metadata to the test suite.
Function version() in distro.py v1.8.0 infers the following:

(Pdb) p versions
['', 'n/a', '', '', '', '', 'bookworm/sid']

Then it chooses "n/a" over "bookworm/sid". That is a bug in distro.py, in my opinion.

It appears Debian has introduced regressions when it moved from the old Python-based lsb_release script to the new Shell-based lsb_release script.

hartwork · 2023-03-14T01:38:53Z

Then it chooses "n/a" over "bookworm/sid". That is a bug in distro.py, in my opinion.

@harshula I think that's where I'm not in agreement so far. Why is it a bug in distro if lsb_release -a also says "n/a"?

harshula · 2023-03-14T03:11:24Z

Why is it a bug in distro if lsb_release -a also says "n/a"?

@hartwork, distro.py has heuristics to collect a set of possible version values:

        versions = [
            self.os_release_attr("version_id"),
            self.lsb_release_attr("release"),
            self.distro_release_attr("version_id"),
            self._parse_distro_release_content(self.os_release_attr("pretty_name")).get(
                "version_id", ""
            ),
            self._parse_distro_release_content(
                self.lsb_release_attr("description")
            ).get("version_id", ""),
            self.uname_attr("release"),
        ]
...
        elif self.id() == "debian" or "debian" in self.like().split():
            # On Debian-like, add debian_version file content to candidates list.
            versions.append(self._debian_version)

The reason to use this technique is to NOT rely on a single method.

In this discussion, you appear to be giving undue weight to one single method, lsb_release, and ignoring another method that provides a more accurate result. Why?

harshula · 2023-03-14T03:15:16Z

Hi @hartwork , More generally, libraries should be clear about expected return values. Ideally, a library should protect users by preventing unexpected return values. I suspect a user of this library would not be expecting "n/a" has a valid return value.

hartwork · 2023-03-14T03:35:33Z

@harshula my view is this:

If lsb_release -a doesn't contain the word "testing" and nothing better than "n/a" then distro should not say "testing" and it's the job of lsb_release to do better for Debian in general.
The ideal amount of distro-specific code in distro is zero. A healthy amount would be little, simple, robust code with a big impact. Maybe it's worth it, maybe it's not.
If Debian need's to fix it anyway, maybe we should just wait for Debian's fix. Else we may have to adjust twice and also have more work than potentially needed looking back in the future.
Maybe the best place for progress is the Debian bugtracker not this pull request (which I mean as encouragement not as silencing)

What do you think?

harshula · 2023-03-14T04:23:28Z

Hi @hartwork,

If lsb_release -a doesn't contain the word "testing" and nothing better than "n/a" then distro should not say "testing" and it's the job of lsb_release to do better for Debian in general.

'Strawman' argument. No one requested this.

What do you think?

I should have opened this PR and not spent time engaging in the tangential discussion, particularly when the relevant points are simply ignored:
#356 (comment)
#356 (comment)

Let me know if you need anything for the actual PR.

HorlogeSkynet · 2023-03-14T07:22:14Z

👍 for merging this PR as-is.
The idea was not to start a debate about an upstream behavior change. Maybe we could :

copy the different pointers included here in a separate issue dedicated to the subject
join an upstream discussion on the Debian's bug tracker

Bye 👋

hartwork · 2023-03-14T14:19:04Z

@harshula alright, this is not going well. I'm not in with fixing the wrong thing just because it's easy. @HorlogeSkynet discussing upstream behavior is exactly what this needs. And discussing @harshula's recent tone. I'm out, @HorlogeSkynet go ahead as you please.

HorlogeSkynet · 2023-03-14T19:05:15Z

To continue (close ?) the discussion about n/a, I read the Debian bug (until now I hadn't, sorry), and it's definitely an upstream regression. Actually, I even experienced it some months ago but quickly worked-around that as, eventually, testing and sid are unstable.

For the following reasons, I am with @hartwork on this issue :

Despite n/a is explicit (as I said earlier), I don't think this would be a good idea to maintain a list of "known tokens" (what if other rolling distributions end up using unset, unknown, TBD, or whatever else). I hope this does not seem a "strawman argument", but you will get my point ;
If lsb_release gives us n/a, as distro honors the back-ends it uses (see package documentation), we should give it back to the user ("power to the user" ?), even if it is actually inconsistent ;
In Supports debian_version file content for version(best=True) calls #333 we added support for Debian's /etc/debian_version file as distro version(best=...) feature was broken on Debian 9+ (a distro-specific feature was broken, not Debian's fault on this), and we hoped this would be the last (?) "distribution-specific" code to implement/maintain here ;
It's an upstream regression, this has to be reported (fortunately for us, it's already done) and fixed... upstream.

My two cents.

Thanks both of you for your time, see you 👋

harshula · 2023-03-15T06:15:44Z

Hi @HorlogeSkynet ,

Thanks for merging the test case! I appreciate that you were trying to be helpful when you started the tangential discussion. There are reasons why I didn't open an Issue to work-around the Debian Regression.

Despite n/a is explicit (as I said earlier), I don't think this would be a good idea to maintain a list of "known tokens" (what if other rolling distributions end up using unset, unknown, TBD, or whatever else). I hope this does not seem a "strawman argument", but you will get my point ;

Unfortunately, that's what libraries sometimes have to do to protect their users. No, that is not a "strawman argument". The other participant in the PR created a fictitious proposition, i.e. returning "testing", and then argued against it. That is considered very poor etiquette.

"power to the user" ?

That explains the philosophical chasm! My approach would be to avoid the user shooting themselves in the foot. 😉 If I have time, I'll open an issue on the tangential discussion.

Thanks! 👋

hartwork · 2023-03-15T16:33:22Z

Hi @HorlogeSkynet ,

Thanks for merging the test case! I appreciate that you were trying to be helpful when you started the tangential discussion. There are reasons why I didn't open an Issue to work-around the Debian Regression.

Despite n/a is explicit (as I said earlier), I don't think this would be a good idea to maintain a list of "known tokens" (what if other rolling distributions end up using unset, unknown, TBD, or whatever else). I hope this does not seem a "strawman argument", but you will get my point ;

Unfortunately, that's what libraries sometimes have to do to protect their users. No, that is not a "strawman argument". The other participant in the PR created a fictitious proposition, i.e. returning "testing", and then argued against it. That is considered very poor etiquette.

"power to the user" ?

That explains the philosophical chasm! My approach would be to avoid the user shooting themselves in the foot. wink If I have time, I'll open an issue on the tangential discussion.

Thanks! wave

Just to have this on record if it gets edited.

HorlogeSkynet requested a review from a team March 11, 2023 10:47

HorlogeSkynet assigned harshula and HorlogeSkynet Mar 11, 2023

HorlogeSkynet added enhancement area: test labels Mar 11, 2023

harshula force-pushed the add-debiantesting-to-tests branch from 02cdf56 to ca35a1b Compare March 12, 2023 06:33

Add Debian Testing to the tests

8716f73

harshula force-pushed the add-debiantesting-to-tests branch from ca35a1b to 8716f73 Compare March 12, 2023 07:46

HorlogeSkynet approved these changes Mar 14, 2023

View reviewed changes

HorlogeSkynet merged commit 59e6e07 into python-distro:master Mar 14, 2023

harshula deleted the add-debiantesting-to-tests branch March 15, 2023 06:40

HorlogeSkynet added this to the 1.8.1 milestone Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Debian Testing to the tests #356

Add Debian Testing to the tests #356

harshula commented Mar 7, 2023

HorlogeSkynet commented Mar 11, 2023

hartwork commented Mar 11, 2023

harshula commented Mar 12, 2023

HorlogeSkynet commented Mar 12, 2023 •

edited

Loading

harshula commented Mar 12, 2023

HorlogeSkynet commented Mar 12, 2023

hartwork commented Mar 12, 2023

harshula commented Mar 13, 2023 •

edited

Loading

hartwork commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

hartwork commented Mar 13, 2023

harshula commented Mar 13, 2023

hartwork commented Mar 14, 2023

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023 •

edited

Loading

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023

harshula commented Mar 14, 2023

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023 •

edited

Loading

harshula commented Mar 14, 2023

HorlogeSkynet commented Mar 14, 2023

hartwork commented Mar 14, 2023

HorlogeSkynet commented Mar 14, 2023

harshula commented Mar 15, 2023

hartwork commented Mar 15, 2023

Add Debian Testing to the tests #356

Add Debian Testing to the tests #356

Conversation

harshula commented Mar 7, 2023

HorlogeSkynet commented Mar 11, 2023

hartwork commented Mar 11, 2023

harshula commented Mar 12, 2023

HorlogeSkynet commented Mar 12, 2023 • edited Loading

harshula commented Mar 12, 2023

HorlogeSkynet commented Mar 12, 2023

hartwork commented Mar 12, 2023

harshula commented Mar 13, 2023 • edited Loading

hartwork commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

harshula commented Mar 13, 2023

hartwork commented Mar 13, 2023

Old

New

harshula commented Mar 13, 2023

hartwork commented Mar 14, 2023

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023 • edited Loading

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023

harshula commented Mar 14, 2023

harshula commented Mar 14, 2023

hartwork commented Mar 14, 2023 • edited Loading

harshula commented Mar 14, 2023

HorlogeSkynet commented Mar 14, 2023

hartwork commented Mar 14, 2023

HorlogeSkynet commented Mar 14, 2023

harshula commented Mar 15, 2023

hartwork commented Mar 15, 2023

HorlogeSkynet commented Mar 12, 2023 •

edited

Loading

harshula commented Mar 13, 2023 •

edited

Loading

hartwork commented Mar 14, 2023 •

edited

Loading

hartwork commented Mar 14, 2023 •

edited

Loading