pachooks icon indicating copy to clipboard operation
pachooks copied to clipboard

Add dkms-install and dkms-remove hooks

Open alucryd opened this issue 8 years ago • 6 comments

These are generic dkms hooks to install/remove modules upon kernel install/upgrade/remove. Individual dkms packages should still have a hook or install script to handle this when they get installed/removed.

alucryd avatar Feb 07 '16 13:02 alucryd

The shebangs should be #!/bin/bash because the scripts contain bashisms. This also has some portability issues. I don't like enumerating preset package names in what is intended to be a portable hook and querying pacman's db is not guaranteed to be possible, because neither the local db nor even pacman itself are guaranteed to be available from inside the installation root. Could we instead use File triggers pointed at usr/lib/modules/* to detect kernel changes and get the kernel version from the path(s) triggered?

andrewgregory avatar Feb 15 '16 00:02 andrewgregory

Listing packages was suboptimal indeed, but the problem with file triggers in my first attempt was that there were a lot of them and the tests took quite a long time. I managed to drastically cut down the time by rearranging the tests, it's quite fast now. Also changed the shebang, thanks!

About the scripts path, pacman comes with /usr/share/libalpm rather than /usr/share/alpm, shouldn't the hooks.bin directory reside inside the former?

alucryd avatar Feb 16 '16 23:02 alucryd

We can make it even faster. Trigger targets can be negated, so by using

Target = usr/lib/modules/*/
Target = !usr/lib/modules/extramodules-*
Target = !usr/lib/modules/*/?*

we can let pacman filter out all the extraneous paths instead of having the script do it. That should let you remove the outer two ifs completely. Then, getting the kernel name is as simple as $(basename "$path") (or you can use bash string manipulation, your choice).

Please quote all variables; I'm not sure which, if any, can actually contain spaces in this case, but I prefer it for consistency.

Does dkms not provide a better way to determine if a module is installed? If not, I think I'd prefer it written as if dkms status ${module/-//} -k $kernel | grep -q 'installed'; then to use grep's return code rather than its output. Also, is dkms output translated? If so you'll need to make sure it's in English for the grep. Alternatively, could we safely run dkms whether or not the module is installed and just let it fail?

Does the remove hook still need to trigger on upgrades? Now that we're using a File trigger I would think it would only need to be run on Remove.

About the scripts path, pacman comes with /usr/share/libalpm rather than /usr/share/alpm, shouldn't the hooks.bin directory reside inside the former?

Probably. I'm still not entirely satisfied with my choices for those paths. Stick with the existing paths for now and I'll look at changing them after this gets merged.

andrewgregory avatar Feb 17 '16 00:02 andrewgregory

Negative targets, thx for the elegant solution!

dkms' output doesn't seem to be translated, and there's no other way to check for modules beyond the status command. But you're right it doesn't harm to let it print an error (plus exit code 3) if the module doesn't exist, plus it's possible to do it in one line that way. That being said, would you like for the output to be redirected to /dev/null, there's quite a lot of it actually (plus the annoying broken pipe messages that have occured for a couple years with no fix on the horizon).

There is no need for the remove hook to be run on upgrades indeed, and as a bonus it doesn't remove modules if you reinstall the same kernel or headers.

All in all, the script is now very much simpler, thx!

alucryd avatar Feb 17 '16 18:02 alucryd

I put this on hold for a while to see what happened with Arch's dkms hook. Do you still think this is good to be merged as-is?

andrewgregory avatar Mar 17 '16 16:03 andrewgregory

The dkms hook covers this and more, I believe this can be dropped and the PR can be closed.

alucryd avatar Mar 17 '16 17:03 alucryd