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

Makefile improvements #594

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

albfan
Copy link

@albfan albfan commented Feb 3, 2022

This is a series of fixes and improvements for makefile:

  • detect dkms module and version: for me digimend/11 is not correctly splitted
  • avoid set -e
  • uninstall module on install (to reset it effectively)
  • silent execution of dkms (using @)
  • automatize dkms uninstall, install, and set debug mode (new target dkms_reinstall)

Let me know if any of this makes sense to cherry-pick or create new isolate PR

@albfan albfan force-pushed the makefile-improvements branch from a23fff8 to 36714b9 Compare February 5, 2022 07:11
Copy link
Member

@spbnick spbnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Alberto!
There are quite a few useful changes, but I've left a few inline comments asking to drop some things, if possible.

enable_debug:
./digimend-debug 1

dkms_reinstall: dkms_uninstall dkms_install enable_debug
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I assume this would be useful for a development loop, to quickly test your changes. However, enabling debug shouldn't be a part of a general-purpose target. After removing it, its use for debugging becomes superficial, and you can just instead have a compound command in your history to do the same:

make dkms_uninstall dkms_install && digimend-debug 1

All-in-all, I think it's more trouble than it's worth, so could you please drop this patch?

@@ -108,7 +108,7 @@ dkms_modules_uninstall: dkms_check
dkms status $(DKMS_MODULES_NAME) | \
while IFS=':' read -r modules status; do \
echo "$$modules" | { \
IFS=', ' read -r modules_name modules_version \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm curious, though, where does that change come from? Has dkms status output format changed?

@@ -102,6 +102,7 @@ dkms_modules_install: dkms_check
dkms add .
dkms build $(DKMS_MODULES)
dkms install $(DKMS_MODULES)
@! rmmod hid_uclogic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant about loading or unloading modules as part of (un)installation, since we don't really know which version of the module is loaded at that moment. At the very least this should be a part of an uninstall target, but I'd prefer, if you dropped this patch.

@albfan
Copy link
Author

albfan commented Aug 7, 2022

I will rebase on top of #629 and address your suggestions.

Thanks for taking a look

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.

2 participants