Skip to content
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

dkms_common.postinst: Add "Tuxedo" distributor id #257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Matombo
Copy link

@Matombo Matombo commented Oct 17, 2022

TUXEDO OS 1 is based on Ubuntu but modifies /usr/lib/os-release. DKMS however should do the same things as on Ubuntu. In most places this can be archived by the ID_LIKE field, however in the common postinstall script, not the ID_LIKE field is checked, but the ID field.

This patch adds the Tuxedo ID to the one if-case where it is relevant.

Signed-off-by: Werner Sembach [email protected]

@evelikov
Copy link
Collaborator

I'm a Debian/Ubuntu noob, so perhaps a silly question: Why don't we use os-release in the dkms_common.postinst script but instead we build on the legacy (I believe) lsb_release one? Can we bribe into doing that?

Nit: please prefix the commit with the file that's changed - dkms_common.postinst: bla bla

@Matombo
Copy link
Author

Matombo commented Oct 19, 2022

I'm a Debian/Ubuntu noob, so perhaps a silly question: Why don't we use os-release in the dkms_common.postinst script but instead we build on the legacy (I believe) lsb_release one? Can we bribe into doing that?

the lsb_release -s -i is actually parsing /usr/lib/os-release if it exists

/etc/lsb_release also exists on Ubuntu, and is parsed directly in another part in DKMS. There however not only the ID but also the ID_LIKE variable is checked, that's why it is no problem.

Nit: please prefix the commit with the file that's changed - dkms_common.postinst: bla bla

Done

@Matombo Matombo changed the title Add "Tuxedo" distributor id dkms_common.postinst: Add "Tuxedo" distributor id Oct 19, 2022
@evelikov
Copy link
Collaborator

The official lsb_release (I believe) does not check os-release - see https://github.com/LinuxStandardBase/lsb-samples/blob/master/lsb_release/src/lsb_release

At a glance Debian has their own minimal version for some reasons. So instead of depending on the fork/variant, it will be better to use os-release directly - this will allow Debian (and others) to effectively drop the seemingly unmaintained LSB thingy.

@Matombo
Copy link
Author

Matombo commented Oct 26, 2022

Ubuntu is using this python based implementation of the lsb_release command: https://salsa.debian.org/debian/lsb

by using os-release directly do you mean manually parsing /usr/lib/os-release?

@Matombo
Copy link
Author

Matombo commented Oct 26, 2022

Ok reading the manual https://www.freedesktop.org/software/systemd/man/os-release.html standard is to only check /etc/os-release (which may or may not be a symlink to /usr/lib/os-release), and only if it doesn't exists checking /usr/lib/os-release.

The python implementation of lsb_release I linked (and ubuntu is using) is violating that as it acesses /usr/lib/os-release directly.

@Matombo
Copy link
Author

Matombo commented Oct 26, 2022

Maybe a better proposal, unifing the loggic in dkms a little bit: #270

@evelikov
Copy link
Collaborator

evelikov commented Oct 26, 2022

Looking at the details: we're overriding the ARCH field, which seems to be a non-issue. Namely it is used effectively as a maker marker - it can be anything "amd64" "x86_64" or even "potatoe". Ubuntu and Debian share the arch naming convention yet Debian is not handled above.

Code was added to fix https://bugs.launchpad.net/ubuntu/+source/dkms/+bug/497149, but admittedly I don't grasp Debian/Ubuntu enough to judge if this is a genuine issue/fix.

@anbe42 (feel free to tag other Debian devs) in your experience, have you ever seen similar patch for dkms in Debian? Does the issue sound something relevant to Debian - aka should we also add you lot in the "amd64" -> "x86_64" mapping, alternatively does it look like something which should be dropped?

@evelikov
Copy link
Collaborator

I've proposed removing the common.postinst script with #339. If you think that may cause issue please reply in there.

If we end up removing it, this MR will no longer be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants