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

common.postinst and AUTOINSTALL="" #368

Closed
anbe42 opened this issue Nov 29, 2023 · 6 comments
Closed

common.postinst and AUTOINSTALL="" #368

anbe42 opened this issue Nov 29, 2023 · 6 comments

Comments

@anbe42
Copy link
Contributor

anbe42 commented Nov 29, 2023

If a dkms package does not set AUTOINSTALL=yes in its dkms.conf (yes, we have one such package in Debian, but I don't know whether this omission is intentional... ), what should common.postinst do upon installation?
Should it really build and install the module?

Right now we have inconsistent behavior depending on the installation order:
case a)

  1. foo-dkms -> common.postinst finds no headers -> no module built
  2. linux-headers-bar -> triggers dkms autoinstall -> no module built due to AUTOINSTALL=""

case b)

  1. linux-headers-bar -> no modules installed, none built
  2. foo-dkms -> common.postinst finds headers -> module gets built

IMO common.postinst should honor the AUTOINSTALL setting and not try to build and install the module.

@anbe42
Copy link
Contributor Author

anbe42 commented Dec 6, 2023

I'll test the following in dkms_common.postinst:

autoinstall=$(AUTOINSTALL=; . "/var/lib/dkms/$NAME/$VERSION/source/dkms.conf" >/dev/null 2>&1; echo $AUTOINSTALL)
if [ -z "$autoinstall" ]; then
    echo "Not building the $NAME module which does not have AUTOINSTALL enabled."
    exit 0
fi

@evelikov
Copy link
Collaborator

evelikov commented Dec 6, 2023

The current code skips the module if $AUTOINSTALL is empty even if set. Aka:

autoinstall()
{
  ...
  if [[ ! $AUTOINSTALL ]]; then
    continue;
  fi
  ...
}

As mentioned previously I would love us to nuke dkms_common.postinst. In order to have no breaking changes in Debian, it could simply calls to kernel_postinst.d_dkms.in and/or kernel_prerm.d_dkms.

Can I offer some virtual 🍪 to sketch and test how it works from Debian POV? Thanks in advance

@anbe42
Copy link
Contributor Author

anbe42 commented Dec 6, 2023

dkms_common.postinst and kernel_postinst.d_dkms.in are two orthogonal things:
dkms_common.postinst runs after installation of foo-dkms and builds the foo module for all kernels
kernel_postinst.d_dkms.in runs after installation of a hernel (or its headers) and builds all modules for this kernel

if dkms_common.postinst is not used elsewhere I have no problem moving it to the debian packaging, but we cannot get rid of it for a few years at least since it is referenced in the maintainer scripts of all *-dkms packages

@evelikov
Copy link
Collaborator

evelikov commented Dec 7, 2023

dkms_common.postinst is used outside of Debian (+ derivatives) so having it upstream is fine. Although I would love to trim it down, or document at the very least.

Notable bits:

  • sources debconf a file - is that still relevant?
  • duplicates existing dkms logic - framework.conf, no-autoinstall, kernel-headers ...
  • while we request autoinstall_all_kernels we install only on latest one (determined via dpkg, rpm) + current kernel ... seemingly not "all" as per the request
  • takes 5 args ... (at a glance) all users provide only 2 + bunch of additional logic for the 3-5 args

I would love to see AUTOINSTALL fixed, but IMHO we should looked into at least some of the above first.

anbe42 added a commit to anbe42/dkms that referenced this issue Feb 8, 2024
AUTOINSTALL is disabled by default (value is unset/empty)

Closes: dell#368
@anbe42
Copy link
Contributor Author

anbe42 commented Feb 8, 2024

Ideally this common.postinst functionality ("build $module for all kernels") should be part of dkms (so it could reuse the existing logic) and common.postinst simply calls dkms like kernel_postinst.d_dkms calls dkms autoinstall.

@evelikov
Copy link
Collaborator

evelikov commented Feb 8, 2024

Fully agreed - it seems like others not using common.postinst (like Arch) will also benefit from such functionality being in core dkms.

Opened a separate task with some details and pointers. Thanks o/

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

No branches or pull requests

2 participants