codefactor-io/codefactor

[shellcheck] Question about/support for source/source-path

MichaIng opened this issue ยท 9 comments

shellcheck allows the source and source-path directives (inside .shellcheck) to allow sourcing a specific file or searching the defined path for files sourced by the shell script.

I tried using this with codefactor via paths relative to the repo root, but this didn't work. Is this supported at all, and if yes, how can a path be given relative to the project root? From what I know relative paths are always relative to the working directory from where shellcheck was executed. Most convenient would be if this was the repository root.

If this is currently not supported, take this issue as a feature request ๐Ÿ™‚.

@MichaIng thanks again for the feedback. Could you describe - or point to commits that shows how your tried using this feature (via paths relative to the repo root)?

Here is the .shellcheck file history: https://github.com/MichaIng/DietPi/commits/dev/.shellcheckrc
The file that is sourced by most of our scripts on the final system is located at /boot/dietpi/func/dietpi-globals but in dietpi/func/dietpi-globals relative to the repository root: https://github.com/MichaIng/DietPi/blob/dev/dietpi/func/dietpi-globals

Setting only the source-path directive in .shellcheck leads this being ignored by codefactor and a related syntax error is shown in web UI. So I added some disable= directives (we anyway needed) to the first line which made the .shellcheck being recognised and used, but source-path stays without effect.

Related shellcheck docs: https://github.com/koalaman/shellcheck/blob/master/shellcheck.1.md#directives
To be true, I did not try using the special SCRIPTDIR path but this means that I'd need to add it to every script or multiple times in .shellcheck for every possible relative-to-script path. It would be most convenient to only use a single entry that is valid for all scripts that source the same file.


EDIT: I should have made some research first. The problem is that those directives to not include -x/--external-sources as one would probably expect. So the shellcheck command still requires this argument to support external sources: koalaman/shellcheck#1818
Probably codefactor can invoke shellcheck with -x/--external-sources, like super-liner did: https://github.com/github/super-linter/pull/537

@MichaIng thanks for the thorough info. We've now enabled -x,\ --external-sources option for ShellCheck process so in theory this feature should work.

In practice, there seems to be a limitation related to paths. Results after testing https://github.com/MichaIng/DietPi repo (dev branch):

  • for .shellcheckrc both source-path=dietpi/func and source-path=./dietpi/func are working as expected
  • for source files, absolute paths (e.g. /boot/dietpi/func/dietpi-globals) are not working. Working values were one of: . dietpi-globals, . dietpi/func/dietpi-globals, . ./dietpi/func/dietpi-globals, . SCRIPTDIR/func/dietpi-globals. There could be more.

As a side note, file PREP_SYSTEM_FOR_DIETPI.sh includes globals because of https://github.com/MichaIng/DietPi/blob/dev/PREP_SYSTEM_FOR_DIETPI.sh#L202

Wow, that was fast, many thanks! Will test it ASAP.

for source files, absolute paths (e.g. /boot/dietpi/func/dietpi-globals) are not working.

That makes sense as your test systems are not DietPi systems. Probably we should add some nice CI/code check bundles to make DietPi suitable for such tasks ๐Ÿ˜„. Makes sense for security reasons, or simply the case that .shellcheck is used for codefactor + manual checks on own systems with two source-path entries, absolute and relative, to limit even read access for shellcheck on the test/check system to the git/repo directory only.

As a side note, file PREP_SYSTEM_FOR_DIETPI.sh includes globals because of

Yes exactly, this script turns Debian systems into DietPi systems, so the dietpi-globals script is downloaded and sourced from current working dir initially.

I added source-path=dietpi/func to our .shellcheckrc but so far it's not yet working, the related "variable appears unused" issues still appear on CodeFactor for all scripts.

I just verified that it's working on a local system with our current .shellcheckrc: https://github.com/MichaIng/DietPi/blob/dev/.shellcheckrc

curl -sSfLO https://github.com/MichaIng/DietPi/archive/dev.tar.gz
tar xf dev.tar.gz
rm dev.tar.gz
cd DietPi-dev
shellcheck -x PREP_SYSTEM_FOR_DIETPI.sh

Thanks, there was still an issue on our side, you should now see updated results for PREP_SYSTEM_FOR_DIETPI.sh file.

Okay for scripts inside the repo root it works now, for the others not yet. Does the checker navigate through the directories?
Locally I can do:

shellcheck -x dietpi/dietpi-autostart
shellcheck -x dietpi/func/dietpi-optimal_mtu
shellcheck -x rootfs/var/lib/dietpi/services/dietpi-firstboot.bash

The path dietpi/func, relative to where the shellcheck command was called, stays valid and dietpi-globals is hence sourced as expected. So for .shellcheckrc to work nicely, the checker must not navigate through the repo but stay in repo root instead. Would be great, as long as this does not imply any negative effects on all the other test tools, otherwise it is no problem for us to add a few more relativ paths to source-path to cover all scripts ๐Ÿ˜‰.

@MichaIng the checker does stay at the root, but e.g. currently file dietpi/dietpi-autostart does not exclude false positives (unused variable warnings) due to absolute path import - https://github.com/MichaIng/DietPi/blob/dev/dietpi/dietpi-autostart#L20

Okay, I misunderstood the the source-path works and on last test I forgot to move the /boot/dietpi/func/dietpi-globals from the system out of the way.

The shellcheck docs say "Absolute paths will also be rooted in these paths." but this means that e.g. . /boot/dietpi/func/dietpi-globals with source-path=dietpi/func leads to . dietpi/func/boot/dietpi/func/dietpi-globals being searched for, so the sourced file path in the script is basically prefixed with the source-path.

In this case I need to use source=dietpi/func/dietpi-globals to source this file explicitly. Just tested it and indeed works as expected. So many thanks for this quick implementation, I close this issue now.