-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add Debian Testing to the tests #356
Add Debian Testing to the tests #356
Conversation
@harshula I would like to second what @HorlogeSkynet is asking. We need a test dummy here, not a real |
02cdf56
to
ca35a1b
Compare
ca35a1b
to
8716f73
Compare
Hi @HorlogeSkynet & @hartwork Apologies, I've pushed the commit again with the correction. |
Thanks for the update @harshula ! I wonder whether we should test/fix the 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 |
Hi @HorlogeSkynet, Your patch results in:
Perhaps, consider having a filter/mapping function that stops characters like "/", ";", etc being returned? Without the patch:
Thanks! |
I get your point but I fear this would end up in a "cat and mouse" issue... |
@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 |
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". |
@harshula I didn't think of testing earlier but I'm getting identical |
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. |
Hi @hartwork, The previous lsb_release (https://sources.debian.org/src/lsb/11.1.0/lsb_release/) written in Python outputs:
|
There's a |
@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 However that's not the code used by the # 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? |
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. |
@harshula that matches what I said above. We're on the same page then 😃 |
What's your specific question? |
@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? |
Hi @hartwork,
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 |
@hartwork, distro.py has heuristics to collect a set of possible version values:
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, |
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. |
@harshula my view is this:
What do you think? |
Hi @hartwork,
'Strawman' argument. No one requested this.
I should have opened this PR and not spent time engaging in the tangential discussion, particularly when the relevant points are simply ignored: Let me know if you need anything for the actual PR. |
👍 for merging this PR as-is.
Bye 👋 |
@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. |
To continue (close ?) the discussion about For the following reasons, I am with @hartwork on this issue :
My two cents. Thanks both of you for your time, see you 👋 |
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.
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.
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! 👋 |
Just to have this on record if it gets edited. |
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 ofdistro
was returning: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!