search

prplfoundation / prplMesh

This repository moved to https://gitlab.com/prpl-foundation/prplmesh/prplMesh
Other
65 stars 32 forks source link

[TASK] Perform shellcheck on shell scripts #915

Closed arnout closed 4 years ago

arnout commented 4 years ago

Shell scripts are risky, but the shellcheck tool can help to find issues with it.

Total lists

Note: Lists below will be updated based on discussion Removal candidates: -./build/common/beerocks/scripts/prplmesh_utils.sh -./build/install/scripts/prplmesh_utils.sh -./build/out/scripts/prplmesh_utils.sh -./common/beerocks/scripts/dhcp.sh -./docker-builder.sh -./docker-runner.sh -./tests/openwrt/test_status.sh -./tools/maptools.sh -./tools/scripts/prplMesh-platform-init.sh -./tools/commands/deploy_rdkb.sh -./tools/commands/deploy_ugw.sh -./framework/platform/bpl/uci/scripts/platform_daemon.sh

No action due to soon removal: -./tests/test_flows.sh -./tests/test_gw_repeater.sh

Scripts not mentioned in any of lists above will be fixed.

abelog commented 4 years ago

Currently, not all scripts comply with shellcheck tool. Result of checking all scripts with the tool: shellcheck_for_all_scripts.txt Can be obtained locally, by: find -name "*.sh" -exec shellcheck {} \; In general, 367 shellcheck complains of any severity level detected (counted using grep -c "line" ~/Downloads/shellcheck_for_all_scripts.txt).

3 way to resolve task available:

  1. Fix all issues, then bring in shellcheck checker script. Block merge until shellcheck output is clear.
  2. Check if shell scripts changed by PR. Check only if new issues are introduced by current PR. If there is any new issue added - block merge.
  3. Check if shell scripts changed by PR. Report all issues (old and new one) and not allow to merge until all issues in changed script are fixed.

Some pros and cons:

I personally favour option 2. Idea with generic flow of script: shellcheck_script_flow Source of UML(plantuml): shellcheck_script_flow.txt But it requires strict PR review as bug in it may block innocent merges (if added to regular CI flow).

abelog commented 4 years ago

Side-note to improve Option 2. Instead of stripping line's which is quite doubtful we may make use of -f checkstyle option to get xml formatted output. Pros here is severity rating, using which we may identify if any new issues were added. Can be done by counting amount of "issues" on type basis.

tomereli commented 4 years ago

I personally think that with a joint effort of 2-3 contributors we can simply fix all issues in a day. I volunteer to be one of those contributors. Besides, shell scripts are now less often changed now that test flows is converted to python, we should spend our efforts enforcing flake8 or alike on python IMO.

rmelotte commented 4 years ago

I agree with @tomereli , it shouldn't take too long to fix all the issues at once, because most are trivial anyway. Also a lot of them are in test_flows.sh, which I think we should just ignore since it's being replaced by test_flows.py.

abelog commented 4 years ago

After fixing the issue, shellcheck may be embedded to GitLab CI as described in link.

arnout commented 4 years ago

Here is a summary of the failing scripts from the attached shellcheck_for_all_scripts.txt

./build/common/beerocks/scripts/beerocks_watchdog.sh

This script can be removed (@tomereli please confirm)

./build/common/beerocks/scripts/prplmesh_utils.sh ./build/install/scripts/prplmesh_utils.sh ./build/out/scripts/prplmesh_utils.sh

3 times the same script, should be fixed.

./clang-format.sh

Should be fixed.

./common/beerocks/scripts/dhcp.sh

Can be removed (@tomereli please confirm)

./docker-builder.sh ./docker-runner.sh

Should be fixed.

./framework/platform/bpl/uci/scripts/platform_daemon.sh

Can be removed.

./tests/openwrt/test_status.sh

Should be fixed.

./tests/test_flows.sh

Will be removed, no need to fix.

./tests/test_gw_repeater.sh

Will be removed, no need to fix.

./tools/commands/deploy_rdkb.sh ./tools/commands/deploy_ugw.sh

I'm not sure. @tomereli ?

./tools/docker/build.sh ./tools/docker/builder/openwrt/build.sh ./tools/docker/builder/rdkb/toolchain/bitbake.sh ./tools/docker/clang-format.sh ./tools/docker/functions.sh ./tools/docker/image-build.sh ./tools/docker/image-pull.sh ./tools/docker/run.sh ./tools/docker/runner/entrypoint.sh ./tools/docker/static-analysis/cppcheck.sh ./tools/docker/stop.sh ./tools/docker/test.sh

All of these should be fixed.

./tools/klocwork/kw.sh

No idea. @tomereli @LiorAmram ?

./tools/maptools.sh

Can be removed I think, @tomereli ?

./tools/scripts/prplMesh-platform-init.sh

Can be removed.

Please first remove the redundant scripts, probably with a separate commit per script (if we need to reinstate a script, we can just revert that commit). Maybe there are also entire directories that can be removed.

tomereli commented 4 years ago

Here is a summary of the failing scripts from the attached shellcheck_for_all_scripts.txt

./build/common/beerocks/scripts/beerocks_watchdog.sh

This script can be removed (@tomereli please confirm)

@morantr @vitalybu Do we use `beerocks_watchdog in RDKB?

./common/beerocks/scripts/dhcp.sh

Can be removed (@tomereli please confirm)

@morantr please confirm that it is not used in UGW/RDKB

./framework/platform/bpl/uci/scripts/platform_daemon.sh

Can be removed.

@adam1985d please approve

./tools/commands/deploy_rdkb.sh ./tools/commands/deploy_ugw.sh

I'm not sure. @tomereli ?

It can be removed. While at it, please also remove all subcommands of the maptools.py script except for build.

./tools/klocwork/kw.sh

No idea. @tomereli @LiorAmram ?

@CoralMalachi @Galvien wasn't this supposed to be ported to python?

./tools/maptools.sh

Can be removed I think, @tomereli ?

Yes - can be removed

adam1985d commented 4 years ago

./framework/platform/bpl/uci/scripts/platform_daemon.sh should be moved from prplmesh to the RDKB receipe - not just removed

in UGW we added the equivalent to the feed

the script is used for reference of platform configuration and may be required in the future when configuring CGR to standalone-repeater-mode

tomereli commented 4 years ago

./framework/platform/bpl/uci/scripts/platform_daemon.sh should be moved from prplmesh to the RDKB receipe - not just removed

in UGW we added the equivalent to the feed

the script is used for reference of platform configuration and may be required in the future when configuring CGR to standalone-repeater-mode

@morantr please move the script. @abelog you can delete it, we can find the script in the repository history.

morantr commented 4 years ago

./framework/platform/bpl/uci/scripts/platform_daemon.sh should be moved from prplmesh to the RDKB receipe - not just removed in UGW we added the equivalent to the feed the script is used for reference of platform configuration and may be required in the future when configuring CGR to standalone-repeater-mode

@morantr please move the script. @abelog you can delete it, we can find the script in the repository history.

I don't think it is needed. We currently don't use it. @adam1985d can we remove it completly?

adam1985d commented 4 years ago

./framework/platform/bpl/uci/scripts/platform_daemon.sh should be moved from prplmesh to the RDKB receipe - not just removed in UGW we added the equivalent to the feed the script is used for reference of platform configuration and may be required in the future when configuring CGR to standalone-repeater-mode

@morantr please move the script. @abelog you can delete it, we can find the script in the repository history.

I don't think it is needed. We currently don't use it. @adam1985d can we remove it completly?

yes, we need it - but not part of prplmesh as it is RDKB related. as already wrote above, it is used as a reference for platform configuration for stand-alone repeater mode please move to the prplmesh recipe