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.

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

rootless: update to 26.1.2 #413

Merged
merged 2 commits into from
May 9, 2024
Merged

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented May 9, 2024

rootless: update to 26.1.2

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland requested a review from a team May 9, 2024 10:59
@thaJeztah
Copy link
Member

LOL, looks like shellcheck may have been updated?

In build/install.sh line 197:
version_compare() (
^-- SC2329 (info): This function is never invoked. Check usage (or ignored if invoked indirectly).

For more information:
  https://www.shellcheck.net/wiki/SC2329 -- This function is never invoked. C...
make: *** [Makefile:20: shellcheck] Error 1
Error: Process completed with exit code 2.

@vvoland
Copy link
Contributor Author

vvoland commented May 9, 2024

Yeah, and it looks like a false positive, because it's used in version_gte which is used all around the script? 🤔

@thaJeztah
Copy link
Member

Looks like a false positive though; version_gte calls version_compare;

docker-install/install.sh

Lines 178 to 183 in 6e61729

version_gte() {
if [ -z "$VERSION" ]; then
return 0
fi
eval version_compare "$VERSION" "$1"
}

@thaJeztah
Copy link
Member

Oh! Race condition; yup; found the same indeed.

Maybe it doesn't like the eval (can we do it without? not sure)

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland
Copy link
Contributor Author

vvoland commented May 9, 2024

Getting rid of the eval doesn't help:

$ git diff --cached                                                                                                                                                                                                
diff --git a/install.sh b/install.sh
index 0734c02..dc7540c 100755
--- a/install.sh
+++ b/install.sh
@@ -179,7 +179,7 @@ version_gte() {
        if [ -z "$VERSION" ]; then
                        return 0
        fi
-       eval version_compare "$VERSION" "$1"
+       version_compare "$VERSION" "$1"
 }

 # version_compare compares two version strings (either SemVer (Major.Minor.Path),

$ make shellcheck                                                                                                                                                                                              
docker run --rm -v "/Users/pawel/c/docker-install":/v -w /v koalaman/shellcheck -eSC1091 -eSC1117 -eSC2317 build/install.sh

In build/install.sh line 197:
version_compare() (
^-- SC2329 (info): This function is never invoked. Check usage (or ignored if invoked indirectly).

For more information:
  https://www.shellcheck.net/wiki/SC2329 -- This function is never invoked. C...
make: *** [shellcheck] Error 1

@thaJeztah
Copy link
Member

bummer! we should probably check if there's a ticket for this, or report it as bug (with a minimal reproducer).

Also wondering if it perhaps doesn't like that version_gte is defined before version_compare (ISTR I ran into something like that somewhere)

@thaJeztah
Copy link
Member

nope; no dice with that either. yup, sounds like just a plain'ol bug then

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 6d9743e into docker:master May 9, 2024
9 checks passed
@vvoland
Copy link
Contributor Author

vvoland commented May 9, 2024

Looks like it's a related to the fix for: koalaman/shellcheck#2966

In previous versions we also had this issue, but it emitted a SC2317 for every line of this function:

In build/install.sh line 198:
        set +x
        ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 200:
        yy_a="$(echo "$1" | cut -d'.' -f1)"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                ^-----------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 201:
        yy_b="$(echo "$2" | cut -d'.' -f1)"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                ^-----------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 202:
        if [ "$yy_a" -lt "$yy_b" ]; then
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
           ^---------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 203:
                return 1
                ^------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 205:
        if [ "$yy_a" -gt "$yy_b" ]; then
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
           ^---------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 206:
                return 0
                ^------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 208:
        mm_a="$(echo "$1" | cut -d'.' -f2)"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                ^-----------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 209:
        mm_b="$(echo "$2" | cut -d'.' -f2)"
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
                ^-----------------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 212:
        mm_a="${mm_a#0}"
        ^--------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 213:
        mm_b="${mm_b#0}"
        ^--------------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 215:
        if [ "${mm_a:-0}" -lt "${mm_b:-0}" ]; then
        ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
           ^-- SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 216:
                return 1
                ^------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 219:
        return 0
        ^------^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).


In build/install.sh line 736:
        exit 1
        ^----^ SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

and we're ignoring the SC2317 completely:

SHELLCHECK_EXCLUSIONS=$(addprefix -e, SC1091 SC1117 SC2317)

After koalaman/shellcheck@4f81dbe this was fixed and now it produces one SC2329 for the whole function.

So there's still a false positive I think, but we already were ignoring that one, so just ignoring the SC2329 should give us the same behavior as before.

@vvoland vvoland mentioned this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants