Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass --enable=all to shellcheck within contrib/ #12760

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Pass --enable=all to shellcheck within contrib/ #12760

merged 1 commit into from
Nov 30, 2021

Conversation

szubersk
Copy link
Contributor

Motivation and Context

Leverage static code analysis to catch bugs in areas that are not easily testable, like initramfs/dracut scripts. If this change gets pulled in other areas could also use it. Shellcheck error SC2250 has been suppressed as its reporting has low value, unless the code base is consistent in $var vs. ${var} dilemma (OpenZFS is not).

Description

  • Remove SHELLCHECK_IGNORE in favor of inline suppressions
    and more general SHELLCHECK_OPTS.

  • Exclude SC2250 (turned on by --enable=all) globally.

  • Pass --enable=all to shellcheck for scripts in contrib/: it's
    very important to catch errors early in areas that are not easily
    testable.

How Has This Been Tested?

Run make checkstyle on Ubuntu 20.04 and Debian 12.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@szubersk
Pass --enable=all to shellcheck within contrib/
e674c7b 
- Remove `SHELLCHECK_IGNORE` in favor of inline suppressions
  and more general `SHELLCHECK_OPTS`.

- Exclude `SC2250` (turned on by `--enable=all`) globally

- Pass `--enable=all` to shellcheck for scripts in contrib/: it's
  very important to catch errors early in areas that are not easily
  testable.

Signed-off-by: szubersk <[email protected]>
@szubersk
Copy link
Contributor Author

@behlendorf Sorry for tagging explicitly you to draw your attention, I saw you accepting previous major shellcheck introduction. I'm not sure what the process of getting approving review is.

@behlendorf behlendorf merged commit 34eef3e into openzfs:master Nov 30, 2021
@behlendorf
Copy link
Contributor

@szubersk looks good, thanks for the nudge and sorry about the delay.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Nov 30, 2021
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 11, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
36fceaf 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

There's no harm to this, of course, but hey

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 11, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
7945730 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 12, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
d97bfed 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 14, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
aac3de0 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 17, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
718aa16 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Dec 21, 2021
@nabijaczleweli
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
a0effda 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
behlendorf pushed a commit that referenced this pull request Dec 21, 2021
@nabijaczleweli @behlendorf
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
153f7c9 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12835
@szubersk szubersk deleted the szubersk-shellcheck branch December 22, 2021 12:47
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 15, 2022
@nabijaczleweli @tonyhutter
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
472c5fe 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12835
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 16, 2022
@nabijaczleweli @tonyhutter
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
734d126 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12835
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 17, 2022
@nabijaczleweli @tonyhutter
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
0cb2d8a 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12835
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@szubersk @nicman23
Pass --enable=all to shellcheck within contrib/
262c5a1 
- Remove `SHELLCHECK_IGNORE` in favor of inline suppressions
  and more general `SHELLCHECK_OPTS`.

- Exclude `SC2250` (turned on by `--enable=all`) globally

- Pass `--enable=all` to shellcheck for scripts in contrib/: it's
  very important to catch errors early in areas that are not easily
  testable.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: szubersk <[email protected]>
Closes openzfs#12760
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nabijaczleweli @nicman23
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
332fc18 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12835
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@szubersk @nicman23
Pass --enable=all to shellcheck within contrib/
13601db 
- Remove `SHELLCHECK_IGNORE` in favor of inline suppressions
  and more general `SHELLCHECK_OPTS`.

- Exclude `SC2250` (turned on by `--enable=all`) globally

- Pass `--enable=all` to shellcheck for scripts in contrib/: it's
  very important to catch errors early in areas that are not easily
  testable.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: szubersk <[email protected]>
Closes openzfs#12760
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nabijaczleweli @nicman23
contrib/initrd hooks: properly quote @LIBFETCH_SONAME@
c183b87 
Bullseye shellcheck picks these up as SC2140, and it's right!
@LIBFETCH_SONAME@ is already quoted, so dracut had
  "$d/"libcurl.so.4""
and i-t had
  ""libcurl.so.4""

Partially reverts 34eef3e (openzfs#12760),
which broke this

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmelikov gmelikov gmelikov approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@szubersk @behlendorf @gmelikov