Fix Shellcheck violations #443

ixti · 2023-02-21T14:44:17Z

Resolves: #442

postmodern

Spotted a few things. We may want to disable a few more of the ShellCheck rules, since they might not correctly be matching the code.

postmodern · 2023-02-22T02:35:37Z

share/ruby-install/util.sh

@@ -12,7 +12,7 @@ function fetch()

 	while IFS="" read -r line; do
 		if [[ "$line" == "$key:"* ]]; then
-			echo "${line##$key:*([[:space:]])}"
+			echo "${line##"$key":*([[:space:]])}"


Are quotes really needed within bash variable substitutions?

var1="foo bar" var2="foo bar baz" echo "${var2#$var1 }"

I don't think so, but shellcheck disagrees :D

Wait, which version of ShellCheck are you using? I'm on 0.7.2, which might be older than your version, which might explain why I'm not seeing those warnings?

Mine is 0.8.0 :D

postmodern · 2023-02-22T02:40:52Z

share/ruby-install/functions.sh

+	ruby_dependencies=()
+	while IFS='' read -r line; do
+		ruby_dependencies+=("$line")
+	done < <(fetch "$ruby/$file" "$package_manager" || return $?)


We actually need the implicit shell-splitting here. fetch "$ruby/$file" "$package_manager" will read the dependencies.txt file and return the apt: pkg1 pkg2 ... line that matches the "$package_manager", but without the apt: prefix. It is then implicitly splatted into an Array of individual package names. Reading each line would cause ruby_dependencies to become a singleton Array that contains the whole line (ex: ("pkg1 pkg2 pkg3")).

Hmm, agree. I just followed (blindly) what Shellcheck WIKI was proposing :D

postmodern · 2023-02-22T02:43:08Z

share/ruby-install/package_manager.sh

-			local missing_pkgs=($(pacman -T "$@"))
+			local missing_pkgs=()
+			while IFS='' read -r line; do
+				missing_pkgs+=("$line")


I'm not a Arch user so I can't verify this, can you double check if pacman -T outputs the missing packages one per-line or a single-line with the package names separated by spaces?
https://archlinux.org/pacman/pacman.8.html

Don't have Arch installed, either. I'm just maintaining Gentoo ebuild in ::guru overlay, and currently I simply disable make check :D

Wait, I forgot I can check using the archlinux docker image.

postmodern · 2023-02-22T02:47:24Z

Looking at some of the ShellCheck errors, I think SC2155 might be wrong here:

In share/ruby-install/checksums.sh line 33:
	local output="$(grep "  $file" "$checksums")"
              ^----^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/checksums.sh line 58:
	local output="$($program "$file")"
              ^----^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/checksums.sh line 74:
	local actual_checksum="$(compute_checksum "$algorithm" "$file")"
              ^-------------^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/functions.sh line 22:
	ruby_dependencies=($(fetch "$ruby/$file" "$package_manager" || return $?))
                           ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).


In share/ruby-install/package_manager.sh line 32:
			local brew_owner="$(/usr/bin/stat -f %Su "$(command -v brew)")"
                              ^--------^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/package_manager.sh line 43:
			local missing_pkgs=($(pacman -T "$@"))
                                            ^---------------^ SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).


In share/ruby-install/ruby-install.sh line 275:
	local fully_qualified_version="$(lookup_ruby_version "$ruby" "$ruby_version")"
              ^---------------------^ SC2155: Declare and assign separately to avoid masking return values.

When you put a command inside of $() the shell will implicitly ignore any returned error codes since it only cares about the output of the command. We might be able to just exclude SC2155 as well. Or we could rewrite all of those spots to use while IFS='' ... so that command being read could fail and also cause the while to fail as well.

ixti · 2023-02-23T04:43:56Z

I'm fine if those checks will be disabled (globally or locally via comments). Just went the lazy approach by doing what SC proposes and making sure tests pass after that on my machine :D

ixti added 3 commits February 21, 2023 15:32

lint: Fix SC2155 violations

360ad4a

lint: Fix SC2295 violations

f2a5b39

lint: Fix SC2207 violations

e1c7cea

ixti force-pushed the fix-make-check branch from 357fe91 to e1c7cea Compare February 21, 2023 14:46

ixti changed the title ~~Fix outstanding Shellcheck violations~~ Fix Shellcheck violations Feb 21, 2023

postmodern requested changes Feb 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Shellcheck violations #443

Fix Shellcheck violations #443

ixti commented Feb 21, 2023

postmodern left a comment

postmodern Feb 22, 2023 •

edited

Loading

ixti Feb 23, 2023

postmodern Feb 23, 2023

ixti Feb 23, 2023

postmodern Feb 22, 2023

ixti Feb 23, 2023

postmodern Feb 22, 2023

ixti Feb 23, 2023

postmodern Feb 23, 2023

postmodern commented Feb 22, 2023 •

edited

Loading

ixti commented Feb 23, 2023

Fix Shellcheck violations #443

Are you sure you want to change the base?

Fix Shellcheck violations #443

Conversation

ixti commented Feb 21, 2023

postmodern left a comment

Choose a reason for hiding this comment

postmodern Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postmodern commented Feb 22, 2023 • edited Loading

ixti commented Feb 23, 2023

postmodern Feb 22, 2023 •

edited

Loading

postmodern commented Feb 22, 2023 •

edited

Loading