search

dell / dkms

Dynamic Kernel Module Support
GNU General Public License v2.0
657 stars 150 forks source link

Update to use the Google shell styleguide #268

Open evelikov opened 1 year ago

evelikov commented 1 year ago

Due to its age (et al) dkms has various styles mixed in. Most prominently - it does not use safe(r) bash constructs line [[ and ((, set -euo pipefail, while in other cases the code is borderline bonkers.

Please keep changes separated logically and do not clump everything into a mega-commit or two.

Ideally we'll also get a CI stage/action that ensures these are consistent, somewhat at least. Can be done alongside the proposed shellcheck https://github.com/dell/dkms/issues/267 or via other means.

siddhpant commented 1 year ago

Maybe the Linux kernel style (like using tabs etc.) should be followed since this is something related to it?

Also, maybe the huge bash script could be split into multiple files for modularity... 80 char limitation will make the script code longer, so if the file is split, reading might become easier. Bash's source can "import" functions from other files.

evelikov commented 1 year ago

I don't understand what you mean - kernel is written in C, so their styleguide follows that, while dkms is in shell/bash. Sounds like trying to push a square peg through a round hole, with this suggestion.

Yes, dkms is big, but chunking it into smaller pieces mostly increases the odds of breaking things. Not unless we get an order of magnitude more tests. Nevertheless - that's not the concern raised here.