Skip to content

Correct searching for rpm in PATH#259

Merged
evelikov merged 1 commit intodkms-project:masterfrom
hadogenes:fix_rpm_postinst
Nov 4, 2022
Merged

Correct searching for rpm in PATH#259
evelikov merged 1 commit intodkms-project:masterfrom
hadogenes:fix_rpm_postinst

Conversation

@hadogenes
Copy link

Signed-off-by: Jacek Szafarkiewicz szafar@linux.pl

@evelikov
Copy link
Collaborator

evelikov commented Oct 18, 2022

The commit message is very misleading - the searching is the same, the output redirection is changed.
Something like "dkms: redirect stderr to null during rpm detection" would be a better name. @hadogenes can you please re-spin?

@xuzhen
Copy link
Collaborator

xuzhen commented Oct 19, 2022

The elif [ `which rpm 2>/dev/null` ]; then just checks if rpm file exists. We can use elif which rpm &>/dev/null; then or elif command -v rpm &>/dev/null; then instead

@hadogenes
Copy link
Author

hadogenes commented Oct 19, 2022

The commit message is very misleading - the searching is the same, the output redirection is changed. Something like "dkms: redirect stderr to null during rpm detection" would be a better name. @hadogenes can you please re-spin?

But [ `which rpm >/dev/null` ] is always false ([ ] test is checking if string is not empty and when we add > /dev/null, then test is always false)

$ which true                                                                                                                                                                                   
/usr/bin/true
$ [ `which true > /dev/null` ] && echo ok || echo fail
fail
$ [ `which true 2> /dev/null` ] && echo ok || echo fail
ok
[ `which not_exists 2> /dev/null` ] && echo ok || echo fail 
fail

@evelikov
Copy link
Collaborator

@hadogenes sorry for misleading you there - you're right this fixes the check, but as a whole the existing code is subpar. Can you please follow xuzhen's advice - thanks in advance.

v2 (Emil)
 - use a proper check (thanks xuzhen)

Signed-off-by: Jacek Szafarkiewicz <szafar@linux.pl>
@evelikov
Copy link
Collaborator

evelikov commented Nov 4, 2022

Fixed up myself, thanks for the great suggestion xuzhen.

@evelikov evelikov merged commit 86ead5c into dkms-project:master Nov 4, 2022
@hadogenes hadogenes deleted the fix_rpm_postinst branch January 19, 2023 08:23
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.

3 participants