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

Revert "FIX dkms built issues for rhel kernel update ,change prerm sc… #77

Merged
merged 1 commit into from
Apr 20, 2019

Conversation

sil2100
Copy link

@sil2100 sil2100 commented Apr 17, 2019

…ript to fix."

This reverts commit 0c19129.

Creating this PR also for discussion, as we think the current behavior is wrong and not fitting the design defined in the manpage.

Per man dkms, the uninstall command should leave the uninstalled modules in the 'built' state and only removed after running remove. After this commit it seems that dkms uninstall started to remove the modules from the built tree, making them no longer stay in the 'built' state (they're not reported as such anymore).

This caused a regression in Ubuntu 19.04 where some of our other packages expected things to work as per the manpage. Quoting:
"Uninstalls an installed module/module-version combo from the kernel/arch passed in the -k option, or the current kernel if the -k option was not passed upon. After uninstall completion, the driver will be left in the built state.
To completely remove a driver, the remove action should be utilized."

Without this change reverted, this does not seem to be the case anymore. After the revert, dkms uninstall seems to work as expected (and as it previously did). What was the rationale for the original change? What use-case was it fixing? The change description wasn't very descriptive.

Thank you!

…ript to fix."

This reverts commit 0c19129.

Per man dkms, the `uninstall` command should leave the uninstalled
modules in the 'built' state and only removed after running `remove`.
@superm1 superm1 requested a review from GoPerry April 17, 2019 15:34
@superm1
Copy link
Contributor

superm1 commented Apr 17, 2019

@yuzaipiaofei can you please review this?

@GoPerry
Copy link
Contributor

GoPerry commented Apr 20, 2019

LGTM

@GoPerry GoPerry merged commit 193d339 into dell:master Apr 20, 2019
@matneh
Copy link

matneh commented Apr 22, 2019

Holy cow, I just ran into this exact issue, I can't believe it's been around for 2 years! Thanks for the fix!

evelikov pushed a commit to evelikov/dkms that referenced this pull request Jan 20, 2020
Add an action "unbuild" to align with the existing "build" one.

This provides for a complete symmetry - add/remove, build/unbuild and
install/uninstall.

Apart from the seeming cosmetic affect, this function is required when
the binary kernel module(s) produced with "dkms build" must be pruned.

Without this command that is not possible since using:
 - "dkms uninstall" will remove the symlinks/copies produced with
"dkms install", while
 - "dkms remove" will erase all references of the dkms module. Hence
"dkms status" will have no references about the module.

This allows for a proper fix for dell#20
while avoiding regressions like dell#77

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
superm1 pushed a commit that referenced this pull request Feb 19, 2020
Add an action "unbuild" to align with the existing "build" one.

This provides for a complete symmetry - add/remove, build/unbuild and
install/uninstall.

Apart from the seeming cosmetic affect, this function is required when
the binary kernel module(s) produced with "dkms build" must be pruned.

Without this command that is not possible since using:
 - "dkms uninstall" will remove the symlinks/copies produced with
"dkms install", while
 - "dkms remove" will erase all references of the dkms module. Hence
"dkms status" will have no references about the module.

This allows for a proper fix for #20
while avoiding regressions like #77

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
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.

5 participants