Skip to content

Shellcheck#7

Merged
dra27 merged 7 commits intometastack:masterfrom
MisterDA:path-blowup-shellcheck
Jun 26, 2020
Merged

Shellcheck#7
dra27 merged 7 commits intometastack:masterfrom
MisterDA:path-blowup-shellcheck

Conversation

@MisterDA
Copy link
Copy Markdown
Contributor

I ran Shellcheck on the scripts and fixed warnings on top of #6 (I’ll rebase if you push more commits).
There are some warnings remaining. Those about quoting seem false-positives, you may want to look at the others.

In msvs-detect line 217:
if [[ $@ != "" ]] ; then
      ^-- SC2199: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).


In msvs-detect line 425:
SDK52_KEY='HKLM\SOFTWARE\Microsoft\MicrosoftSDK\InstalledSDKs\8F9E5EF3-A9A5-491B-A889-C58EFFECE8B3'
^-------^ SC2034: SDK52_KEY appears unused. Verify use (or export if used externally).


In msvs-detect line 483:
  ["SDK5.2"]='(
             ^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.


In msvs-detect line 903:
  TRIM_PATH=$(echo "$PATH" | sed -e 's|\([^:]\)/\+\(:\|$\)|\1\2|g')
  ^-------^ SC2034: TRIM_PATH appears unused. Verify use (or export if used externally).


In msvs-detect line 938:
             $(cygpath "$COMSPEC") ${SWITCH_PREFIX}v:on ${SWITCH_PREFIX}c $COMMAND 2>/dev/null | grep -F XMARKER -A 3 | tr -d '\015' | tail -3)
                                                                          ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
             $(cygpath "$COMSPEC") ${SWITCH_PREFIX}v:on ${SWITCH_PREFIX}c "$COMMAND" 2>/dev/null | grep -F XMARKER -A 3 | tr -d '\015' | tail -3)


In msvs-detect line 1059:
    TEST_ARCH=$TARGET_ARCH
    ^-------^ SC2034: TEST_ARCH appears unused. Verify use (or export if used externally).

For more information:
  https://www.shellcheck.net/wiki/SC2199 -- Arrays implicitly concatenate in ...
  https://www.shellcheck.net/wiki/SC2034 -- SDK52_KEY appears unused. Verify ...
  https://www.shellcheck.net/wiki/SC2016 -- Expressions don't expand in singl...

Shellcheck doesn’t parse the construct eval array+=(element). Temporary applying this patch is needed to make it parse the file:

diff --git a/msvs-detect b/msvs-detect
index e920ea9..eca6b53 100755
--- a/msvs-detect
+++ b/msvs-detect
@@ -791,8 +791,8 @@ for i in $MSVS_PREFERENCE ; do
           fi
         done
         INSTANCES="$(sort -r <<< "${INSTANCES// /$'\n'}")"
-        eval TEST+=($INSTANCES)
-        eval PREFERENCE+=($INSTANCES)
+        # eval TEST+=($INSTANCES)
+        # eval PREFERENCE+=($INSTANCES)
       fi
     else
       if [[ -n ${CANDIDATES["VS$i"]+x} ]] ; then
@@ -813,8 +813,8 @@ for i in $MSVS_PREFERENCE ; do
       SDKS=${SDKS% }
       SDKS="$(sort -r <<< "${SDKS// /$'\n'}")"
       SDKS=${SDKS//$'\n'/ }
-      eval TEST+=($SDKS)
-      eval PREFERENCE+=($SDKS)
+      # eval TEST+=($SDKS)
+      # eval PREFERENCE+=($SDKS)
     fi
   fi
 done

Copy link
Copy Markdown
Contributor

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

There're various idioms in msvs-promote-path where I simply can't remember whether the idiom is me being silly, Cygwin being silly or Windows-over-Cygwin being weird! It makes me very cautious to adjust without a clear bug being fixed, though.

I don't particularly want to update msvs-promote-path without an actual bug to fix, so I'll skip b70ce0f, though I'll note that I have no idea why I used -a and && [ … ] in the same if 🤦‍♂️

Likewise, the changes in 22b3762 may be correct, but I don't relish having to fire up the test VM with VS 2002, VS 2003, VS 2005, .., 2015 to be sure it's behaving correctly on all those code paths! The whitespace fix is good, though 😊

07be037, 85fd26c, 33f323b and da5e8f9 are all good to go, though, thanks!

Would you remind rebasing without b70ce0f and with 22b3762 reduced to a whitespace-only fix, please?

@MisterDA MisterDA force-pushed the path-blowup-shellcheck branch from b70ce0f to 3d0ea1f Compare June 26, 2020 11:45
@MisterDA
Copy link
Copy Markdown
Contributor Author

Done, rebased against master. For reference, the old branch is archived in https://github.com/MisterDA/msvs-tools/tree/path-blowup-shellcheck-bak.

@dra27
Copy link
Copy Markdown
Contributor

dra27 commented Jun 26, 2020

My comment about b70ce0f is daft, if you want to push that back to the branch, and I'll merge

@MisterDA MisterDA force-pushed the path-blowup-shellcheck branch from eaa72d4 to 084a2d6 Compare June 26, 2020 12:45
@dra27 dra27 merged commit 8f4bd73 into metastack:master Jun 26, 2020
@dra27
Copy link
Copy Markdown
Contributor

dra27 commented Jun 26, 2020

Thank you!

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