shellcheck.yml: add new tests using shellcheck

[Nowa-Ammerlaan]

I propose to add this new github action using shellcheck, it should hopefully avoid issues such as #488 occurring in a release.

Note this will cause the CI to fail until all existing issues are either fixed or ignored.

[scaronni]

Woah. Thanks!

Can you also add the actual commits that fix the shellcheck errors? :)

[Nowa-Ammerlaan]

Can you also add the actual commits that fix the shellcheck errors? :)

Yes, but that is a bit more work and will take some time.

[scaronni]

Can you also add the actual commits that fix the shellcheck errors? :)

Yes, but that is a bit more work and will take some time.

No pressure! Many thanks.

[anbe42]

Thanks for your effort. Please clear the draft flag once it is ready for review.

[Nowa-Ammerlaan]

Done! CI is green again.

90% of this was just missing quotes. Other common issues were:

• read without -r
• expanding an array without any index, and array<->string mismatches
• missing failure conditions for cd
• calling rm on a variable without :?

There were some more complex issues which I marked as todo for later in a comment. Specifically a couple of cases where ls is used, but find (+ -exec) would be better.

There were also two obvious typos which got flagged as a variable unused warning. found_original (versus found_orginal) in the do_install function. And symlink_modules (versus symlink_module) for the --symlink-modules option. Fixed both of those issues. I think this specifically proves the usefulness of this new github action.

[anbe42]

Now that this is working, can we split up the huge dkms.in commit into a few smaller ones? While reading, the following categories came into my mind:

bugfix: misspelled variable names

bugfix: cd $dest || exit $some-unused-error-code see the list at the beginning, add it there, in order to clean up error code usage some day I don't want to add even more uses of the heavily overloaded ones

reindent - there is at least one big hunk being reindented, please do this separately (git diff -w should be empty on such a change) without any other changes hidden in it

the complex change w.r.t. parameter passing for safe_source() including the changes f
for list splitting, including all the (transitive) calls
(If I got it right, this is the only change where multiple lines at different places need to change together, while all other changes only affect that single line

missing quoting for scalars

array vs scalar misuse

shellcheck ignore without todo

shellcheck ignore with todo

removal of unused variables

lets see what is left over ;-)

what does shellcheck think about [[ $var ]] vs. [[ -n "$var" ]]? I'd prefer the first, but currently both are used inconsistently.

[anbe42]

the complex change w.r.t. parameter passing for safe_source() including the changes f
for list splitting, including all the (transitive) calls
(If I got it right, this is the only change where multiple lines at different places need to change together, while all other changes only affect that single line

While cleaning that up, would it be helpful if dkms_conf_variables etc were arrays instead of strings that needs to be split on whitespace into separate tokens? Not sure what implications that would have ...

[Nowa-Ammerlaan]

While cleaning that up, would it be helpful if dkms_conf_variables etc were arrays instead of strings that needs to be split on whitespace into separate tokens? Not sure what implications that would have ...

Technically this would be better indeed, but since we know that none of these variables contain whitespaces we can safely split the string into an array in the sourcing function. I am not sure if changing this now could have unintended side effects, so I'd rather not do it now and focus instead purely on introducing the new check and making it (mostly) happy with the current code without changing it too much.

[scaronni]

I think we can wait until this pull request is finished and make a new release, we have already a few fixes worth releasing.
@anbe42 ?

[scaronni]

Also, personal experience so far, unfortunately nobody tests or gives any feedback until code is tagged in a release. When people see a new release and start updating it's where you start getting feedback. So no candiates/betas etc.

[Nowa-Ammerlaan]

Now that this is working, can we split up the huge dkms.in commit into a few smaller ones?

Done!

what does shellcheck think about [[ $var ]] vs. [[ -n "$var" ]]? I'd prefer the first, but currently both are used inconsistently.

AFAIK it doesn't complain about either.

[anbe42]

Thanks for splitting it up. Improves readability of the changes a lot.

Nothing obvious that I've spotted, will give it some testing in my module test setup soon.

Let's get this in, as it will change the base for other pending changes.

[scaronni]

Back from holiday. @anbe42 i've merged the pull request, but you're an admin of the project, you can do the same as you please. Thanks.

[anbe42]

I think we made a mistake here with handling $ARCH: previously this expanded to either 0 (usually) or 2 (rare case when $ARCH was set (externally?), then it was rewritten a few lines above to -a $ARCH) arguments. Now it always expands to 1 argument, usually the empty string "" or "-a $ARCH". Both cause errors.

# ./dkms status -m dkms_test -v 1.0 -k 6.13-amd64
dkms_test/1.0, 6.13-amd64, x86_64: built
# ./dkms status -m dkms_test -v 1.0 -k 6.13-amd64 ""
Warning: I do not know how to handle .
dkms_test/1.0, 6.13-amd64, x86_64: built
# ./dkms status -m dkms_test -v 1.0 -k 6.13-amd64 "-a x86_64"

Error! Unknown option: -a x86_64
Usage: ./dkms ...

Should we rather use ${ARCH:+-a "$ARCH"} and avoid injecting the -a ?

[scaronni]

Sounds good to me. Thanks.