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

Final round of removals #164

Merged
merged 7 commits into from
Oct 30, 2021
Merged

Final round of removals #164

merged 7 commits into from
Oct 30, 2021

Conversation

evelikov
Copy link
Collaborator

@evelikov evelikov commented Oct 9, 2021

This is the final set of removal/cleanups inspired by #158. In particular:

  • remove moar dead code that I've forgot with Remove deb rpm and driverdisk commands #161
  • removes some code for Suse 10 and earlier
  • removes rpm based detection of distributions -> now we use os-release, falling-back to lsb
    • bonus point: derivative distros correctly pick the override directory
  • removes support for 2.x era kernels, and no longer applicable no-{clean,prepare}-kernel toggles
  • removes support/deprecates REMAKE_INITRD and MODULES_CONF*

Note: while I did have a close look at non-Arch distros, I would appreciate a confirmation from others - Debian, Fedora/RedHat and Suse. Specifically, do you ship dkms modules which:

  • lack both os-release and lsb-release
  • use REMAKE_INITRD and MODULES_CONF*

@jwrdegoede @adrelanos @rhertzog - team, any input on the above questions, plus testing would be greatly appreciated.

Thanks in advance

@evelikov
Copy link
Collaborator Author

I found one DKMS module, which sets REMAKE_INITRD="no". "No" is the default, so sent a MR to remove it.

@scaronni
Copy link
Collaborator

Can you please rebase? I had some conflicting commits already staged. Thanks.

@scaronni scaronni self-assigned this Oct 10, 2021
@evelikov
Copy link
Collaborator Author

evelikov commented Oct 10, 2021

@scaronni I think you've removed the wrong hunks with 7472cbd. I suspect that you missed the ! in the if [[ (! ( $(VER $1) < $(VER 2.6... conditionals.

See for example how I've done it - with a6b6302

Is that correct? Double-checking so I know exactly how to resolve the lot.

Edit: in particular, prior to your commit above, my builds will consistently hit the "Kernel preparation/cleanup unnecessary for this kernel. Skipping...", which are now outright removed 🤔

@scaronni
Copy link
Collaborator

Right, I'm getting confused with your merge requests and the limited time I have, you're too quick 😄. I'll revert it so your applies cleanly. Sorry for that.

Remove a couple of left-over references with the sample files removal.

v2: Rebase (aka drop Makefile changes)

Fixes: 60b4250 ("sample: remove sample spec/conf files")
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
The only user was mkdriverdisk - with that one gone, we can remove it

Fixes: 503e9e3 ("dkms: remove mkdriverdisk command")
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Just for the generic and Debian/Ubuntu messages - they provide good
enough base for everyone.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Suse 10 and earlier have been EOL for 5+ years.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Rework the existing distribution detection, to honour os-release falling
back to LSB, ultimately removing the rpm detection hacks.

The former - os-release - has been a thing for 5+ years across distros.
Its ID is tolower(DISTRIB_ID), while it also provides ID_LIKE indicating
closes possible distribution.

For example: Ubuntu uses ID_LIKE=debian and any derivative of the two
can do the same - ID_LIKE=ubuntu or ID_LIKE=debian.

This means, that now we might trigger distribution-specific behaviour in
a few derivatives - which is perfectly fine.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Drop the various VER 2.6 checks and essentially dead
no-{clean,prepare}-kernel switches.

v2:
 - Use the correct command - SUBDIR= is for < 2.6.6, M= for newer
 - Remove the misleading messages
   "Kernel preparation unnecessary for this kernel. Skipping..."
   "Kernel cleanup unnecessary for this kernel. Skipping..."

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Looking through our initrd and modules.conf handling makes me tremble.
While the latter seems fairly complete, the former covers only a very
limited set of initrd generators in very inflexible way.

Checking through the Archlinux DKMS modules - none of them sets either
REMAKE_INITRD nor MODULES_CONF*. I couldn't find any Fedora packages
which defined them either.

Considering the code size, complexity, lack of users and the fact that
it touched system files I'm inclined that we can remove this.

AFAICT the rest of the dkms codebase does not touch foreign files.

Remove the related and seemingly unused mkkerneldoth.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
@evelikov
Copy link
Collaborator Author

evelikov commented Oct 11, 2021

I'm trying to be quick when I have maintainer's attention - sorry if it was too much.

NOTE: Please wait at least 1-2* weeks before merging this. I would love to double-check/hear from RedHat/Suse people if the deprecation/removal might cause problems for them.

@evelikov
Copy link
Collaborator Author

Found another REMAKE_INITRD user - sent MR to remove the toggle

@scaronni
Copy link
Collaborator

I will make some test for the RedHat / Fedora part. Thanks.

@scaronni
Copy link
Collaborator

All seems to be good, but I will merge in a few days due to holidays. Cheers!

@scaronni scaronni merged commit 32f8293 into dell:master Oct 30, 2021
@evelikov evelikov deleted the final-removals branch October 30, 2021 11:25
@emilyst
Copy link

emilyst commented Apr 30, 2022

If you write a module that replaces an existing Linux kernel module, what's the recommended way to do that now? Add a custom preinstall script that removes the old module and blacklists it?

@evelikov
Copy link
Collaborator Author

evelikov commented May 6, 2022

@emilyst simple - make sure the new module (location) is searched first - see "search" directive in man depmod.d for details.
On my Arch box - cat /usr/lib/depmod.d/search.conf shows search updates extramodules built-in so modules in "updates" are picked before the normal kernel "built-in" ones.

The destination can be overridden in dkms override_dest_module_location() function. If needed you might need to extend it with a distro specific switch case.

@emilyst
Copy link

emilyst commented May 6, 2022

@emilyst simple - make sure the new module (location) is searched first - see "search" directive in man depmod.d for details. On my Arch box - cat /usr/lib/depmod.d/search.conf shows search updates extramodules built-in so modules in "updates" are picked before the normal kernel "built-in" ones.

The destination can be overridden in dkms override_dest_module_location() function. If needed you might need to extend it with a distro specific switch case.

Just to clarify, I was hoping for some mechanism within dkms.conf equivalent to the now-obsolete MODULES_CONF_OBSOLETES. It sounds like you're saying that's not possible anymore.

@evelikov
Copy link
Collaborator Author

evelikov commented May 9, 2022

Set DEST_MODULE_LOCATION to a higher priority location (see the depmod.d comment). Then the modprobe command (explicit or implicit) should load the new/dkms one, instead of the old/built-in.

@ebbcm
Copy link

ebbcm commented Nov 7, 2022

Set DEST_MODULE_LOCATION to a higher priority location (see the depmod.d comment). Then the modprobe command (explicit or implicit) should load the new/dkms one, instead of the old/built-in.

This will help load the DKMS module, if its location is higher priority in the search path, but will not update the INITRD, so for any modules in the distro ramdisk, the DKMS installed module will not be automatically loaded at boot.

Without REMAKE_INITRD, any DKMS module with replaces distro built-in modules now has to make two distro-specific actions:

  1. Use a possibly distro-specific installation location, or alter the system depmod.d conf file
  2. Rebuild the initrd with distro-specific tools and options

I suggest re-adding REMAKE_INITRD and improving it if necessary.

Some drivers certainly escaped your survey of users of REMAKE_INITRD, including ours (https://www.broadcom.com/products/ethernet-connectivity/network-adapters/p2100g). We distribute DKMS drivers as updates to the in-box drivers, and since our drivers (bnxt_en) are usually included in the distro's ramdisk, we need to install to a higher priority depmod path AND rebuild the initrd.

I don't think the proper solution is each DKMS module implementing ramdisk management individually.

@ebbcm
Copy link

ebbcm commented Nov 8, 2022

More information: We scanned our supported OS list for the in-box depmod configured search paths:

CentOS 7.9 search updates extra weak-updates kgraft built-in
RHEL 8.6 search updates extra built-in weak-updates
RHEL 9.0 search updates extra built-in weak-updates
SLES 12u4 search updates extra weak-updates kgraft built-in
SLES 15u3 search updates extra weak-updates kgraft built-in
Ubuntu 20.04 search updates ubuntu built-in

Clearly "updates" is the right target directory for drivers which wish to supersede the built-in version, so that's one less OS-specific decision. We still need to be able to update ramdisk images though.

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.

4 participants