Make the BLIS configure script pass all shellcheck checks by leekillough · Pull Request #729 · flame/blis

leekillough · 2023-03-16T23:22:52Z

Make the BLIS configure script pass all shellcheck checks, disabling ones which we violate but which are just stylistic, or are special cases in our code.

# shellcheck disable=2001,2249,2034,2154,2181,2312,2250,2292 disables the following checks for the entire configure script:

SC2001: See if you can use ${variable//search/replace} instead.
SC2249: Consider adding a default *) case, even if it just exits with error.
SC2034: foo appears unused. Verify it or export it.
SC2154: var is referenced but not assigned. (configure uses eval to assign to variables whose names are runtime variables.)
SC2181: Check exit code directly with e.g. if mycmd;, not indirectly with $?.
SC2312: Consider invoking this command separately to avoid masking its return value (or use || true to ignore).
SC2250: Prefer putting braces around variable references even when not strictly required.
SC2292: Prefer [[ ]] over [ ] for tests in Bash/Ksh.

A bug was found in pass_config_kernel_registries() which prevented a list of more than one configuration from working:

        for item in "${list}"; do

The "${list}" is taken as one item, and word splitting is not performed, so the loop only executes once. It should be:

        for item in ${list}; do

or it should use Bash array variables.

A bug was found in main():

       while $found = true; do

should be:

       while [[ $found = true ]]; do

Address SC2268: Avoid x-prefix in comparisons as it no longer serves a purpose. (e.g., [ "x$var" = "xvalue"]).
Address SC2086: Double quote to prevent globbing and word splitting.

There are many places in the code where variables are expanded but are not quoted, which causes word splitting, and globbing if the variable contains wildcard characters, and can lead to syntax errors if the variable is empty and used in [ ].

When the variable (or command substitution $( ... )) appears on the LHS or as a unary operand of a [ ] test, then simply replacing [ ] with [[ ]] will avoid the problem, because inside [[ ]], the expansion is syntactically always present in the test, even if it expands to empty.

When a variable possibly contains spaces, and it needs to be passed or treated as a single value, it should be double-quoted. As a test, I tried building BLIS in a directory whose name contained spaces.

On the other hand, there are a few places where word splitting is desired, such as building command lines with multiple options separated by spaces, and for those cases, this check has been disabled with # shellcheck disable=2086.

As part of #5, the variables were placed inside double quotes to avoid SC2086. But I noticed that when printing the final installation directories, configure was sometimes printing symbolic names like ${prefix}. To fix this, I added eval echo, two for libdir in case it defaults to ${exec_prefix} and exec_prefix defaults to ${prefix}:

       echo "${script_name}:   prefix:      "${prefix}
       echo "${script_name}:   exec_prefix: "${exec_prefix}
       echo "${script_name}:   libdir:      "${libdir}
       echo "${script_name}:   includedir:  "${includedir}
       echo "${script_name}:   sharedir:    "${sharedir}

becomes:

       echo "${script_name}:   prefix:      $(eval echo "${prefix}")"
       echo "${script_name}:   exec_prefix: $(eval echo "${exec_prefix}")"
       echo "${script_name}:   libdir:      $(eval echo "$(eval echo "${libdir}")")"
       echo "${script_name}:   includedir:  $(eval echo "${includedir}")"
       echo "${script_name}:   sharedir:    $(eval echo "${sharedir}")"

It now prints out like this:

configure: final installation directories:
configure:   prefix:      /usr/local
configure:   exec_prefix: /usr/local
configure:   libdir:      /usr/local/lib
configure:   includedir:  /usr/local/include
configure:   sharedir:    /usr/local/share

Address SC2162: read without -r will mangle backslashes.

There are a few places where read is used without -r. Unless it is the intention to interpret backslashes as escape characters in files read by configure, read -r should be used.

Address SC2196: egrep is non-standard and deprecated. Use grep -E instead.
Address SC2244: Prefer explicit -n to check non-empty string (or use =/-ne to check boolean/integer).
Address SC2295: Expansions inside ${..} need to be quoted separately, otherwise they will match as a pattern. (Unclear?)

       dist_path=${0%/${script_name}}

       dist_path=${0%"/${script_name}"}

Address SC2004: $/${} is unnecessary on arithmetic variables.

       shift $(($OPTIND - 1))

becomes:

       shift $((OPTIND - 1))

Address SC2006: Use $(...) notation instead of legacy backticked `...`.

                                       var=`expr "$1" : '\([^=]*\)='`
                                       value=`expr "$1" : '[^=]*=\(.*\)'`
                                       eval $var=\$value
                                       export $var

becomes:

                                       var=$(expr "$1" : '\([^=]*\)=')
                                       value=$(expr "$1" : '[^=]*=\(.*\)')
                                       eval "export $var=\$value"

Address SC2002: Useless cat. Consider cmd < file.

       so_version_major=$(cat ${so_version_filepath} | sed -n "1p")

becomes:

       so_version_major=$(sed -n "1p" < "${so_version_filepath}")

and:

       so_version_minorbuild=$(cat ${so_version_filepath} | sed -n "2p")

becomes:

       so_version_minorbuild=$(sed -n "2p" < "${so_version_filepath}")

and others.

Address SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.

Also, for style, use [[ ]] which has && and || operators, instead of [ ] && [ ] and [ ] || [ ].

Replace:

       if [ "${cc_vendor}" = "icc" -o \
            "${cc_vendor}" = "gcc" ]; then

with:

       if [[ ${cc_vendor} = icc || ${cc_vendor} = gcc ]]; then

Similarly, replace:

               if   [ "${env_str}" = "AR" -o \
                      "${env_str}" = "RANLIB" ]; then

with:

               if   [[ ${env_str} = AR || ${env_str} = RANLIB ]]; then

(Style.) Replace:

               if [ "$(echo ${vendor_string} | grep -o Apple)" = "Apple" ]; then

with:

               if [[ ${vendor_string} = *Apple* ]]; then

(Style.) Replace:

               if [[ "${list}" == *[/]* ]]; then

with:

               if [[ ${list} = */* ]]; then

(Style.) Replace long pipelines of sed and perl filters with multiple -e scriptlets. Faster and creates fewer processes. Need to add ; at the end of each Perl scriptlet, since they get concatenated into one large script.
(Style.) Replace 7 long ${gen_make_frags_sh} calls, with all but 3 arguments the same in each case, and with an echo before each call, with short calls to a newcreate_makefile_fragment() function.
(Style.) Replace repeated code which creates Makefile, blis.pc.in, common.mk and config symlinks, with one loop which iterates over them. Also address SC2226: This ln has no destination. Check the arguments, or specify . explicitly.
(Style.) Rather than setting a variable with a grep command substitution and then testing whether it is empty, test the assignment command itself, which will return a nonzero exit code if the grep fails to find a match:

       aocc_grep=$(echo "${vendor_string}" | grep 'AOCC')
       if [ -n "${aocc_grep}" ]; then

becomes:

       if aocc_grep=$(echo "${vendor_string}" | grep 'AOCC'); then

and:

               aocc_ver21=$(echo "${vendor_string}" | grep 'AOCC.LLVM.2')
               if [ -n "${aocc_ver21}" ]; then

becomes:

               if aocc_ver21=$(echo "${vendor_string}" | grep 'AOCC.LLVM.2'); then

This allows the program to continue in case set -e is enabled, because it's not considered a fatal error if the grep fails inside an if.

(Style.) Use bash pattern-matching instead of echo and grep (supersedes #22):

               uconf=$(echo "${config_name}" | grep -c 'zen\|amd64')

               if [[ $uconf == 0 ]]; then

becomes:

               if [[ ${config_name} != *zen* && ${config_name} != *amd64* ]]; then

and:

       aocc_grep=$(echo "${vendor_string}" | grep 'AOCC')
       if [ -n "${aocc_grep}" ]; then

becomes:

       if [[ ${vendor_string} = *AOCC* ]]; then

and:

       armclang_grep=$(echo "${vendor_string}" | grep 'Arm C/C++/Fortran Compiler')
       if [ -n "${armclang_grep}" ]; then

becomes:

       if [[ ${vendor_string} = *'Arm C/C++/Fortran Compiler'* ]]; then

Make the configure script work when errors are fatal (set -e). Add || : in a few places, such as:

                       vendor_string="$(${FC} --version 2>/dev/null || :)"

this needs to be tested on all supported platforms, perhaps in the CI, to ensure that set -e can been enabled in configure.

Edit: In case it wasn't clear, set -e has not been added to the configure script, since it may break it on certain platforms I cannot test; however, clearly obvious places where set -e would cause configure to fail have been identified and remedied. With the || : change above, configure works fine to completion on x86-64 even if set -e is added before main is called. I consider it good practice to make sure a script properly handles every command which returns a non-zero exit code.

(Style.) Use $(<file) instead of $(cat file) for loading the contents of a file into a shell variable.

leekillough · 2023-03-19T06:45:30Z

configure

-			for item in "${list}"; do
+
+			for item in ${list}; do



This is a bug -- quotes makes the loop only execute once, even if ${list} contains space-separated items.

If the items in ${list} may need to contain spaces, then declaring ${list} a Bash array variable and using it such as with "${list[@]}" will work.

Agreed, this was a bug. I think we might have been lucky in that the config_registry didn't make use of syntax that would have needed the loop.

leekillough · 2023-03-19T06:53:20Z

configure

 	# -- Command line option/argument parsing ----------------------------------

 	found=true
-	while $found = true; do


devinamatthews · 2023-03-19T15:41:22Z

@leekillough thanks for going through and fixing all of these! I'll let Field comment on individual changes (@fgvanzee unless you want me to go through it, no problem). I'll test on MacOS since it is stuck on GPLv2 bash.

…range redirections in long sed/perl pipes to look more natural

… to use only tabs

fgvanzee · 2023-03-20T16:26:22Z

@leekillough Thank you for this exhaustive review. I sincerely appreciate your efforts. I'll go through your list more closely and see if I have any comments.

fgvanzee · 2023-03-21T02:42:32Z

This looks good, @leekillough. I only made a few trivial whitespace/formatting changes. I'll merge shortly. Thanks again for your contribution.

fgvanzee · 2023-03-23T20:54:42Z

@leekillough Lol, sorry. I got distracted while waiting for CI to finish. I mean it this time! 😅

Details: - Modified configure so that it passes all 'shellcheck' checks, disabling ones which we violate but which are just stylistic, or are special cases in our code. - Miscellaneous other minor changes, such as rearranged redirections in long sed/perl pipes to look more natural. - Whitespace tweaks.

Details: - Updated common.mk so that when --disable-shared option is given to configure: 1. The -fPIC compiler flag is omitted from the individual configuration family members' CPICFLAGS variables (which are initialized in each subconfig's make_defs.mk file); and 2. The BUILD_SYMFLAGS variable, which contains compiler flags needed to control the symbol export behavior, is left blank. - The net result of these changes is that flags specific to shared library builds are only used when a shared library is actually scheduled to be built. Thanks to Nick Knight for reporting this issue. - CREDITS file update. - (cherry picked from commit 5f84130) Updated configure to pass all shellcheck checks. (#729) Details: - Modified configure so that it passes all 'shellcheck' checks, disabling ones which we violate but which are just stylistic, or are special cases in our code. - Miscellaneous other minor changes, such as rearranged redirections in long sed/perl pipes to look more natural. - Whitespace tweaks. - (cherry picked from 72c37eb) Fixed bugs in scal2v ref kernel when alpha == 1. (#728) Details: - Fixed a typo bug in ref_kernels/1/bli_scal2v_ref.c where the conditional that was supposed to be checking for cases when alpha is equal to 1.0 (so that copyv could be used instead of scal2v) was instead erroneously comparing alpha against 0.0. - Fixed another bug in the same function whereby BLIS_NO_CONJUGATE was erroneously being passed into copyv instead of the kernel's conjx parameter. This second bug was inert, however, due to the first bug since the "alpha == 0.0" case was already being handled, resulting in the code block never executing. - (cherry picked from 60f3634) Use 'void*' datatypes in kernel APIs. (#727) Details: - Migrated all kernel APIs to use void* pointers instead of float*, double*, scomplex*, and dcomplex* pointers. This allows us to define many fewer kernel function pointer types, which also makes it much easier to know which function pointer type to use at any given time. (For example, whereas before there was ?axpyv_ker_ft, ?axpyv_ker_vft, and axpyv_ker_vft, now there is just axpyv_ker_ft, which is equivalent so what axpyv_ker_vft used to be.) - Refactored how kernel function prototypes and kernel function types are defined so as to reduce redundant code. Specifically, the function signatures (excluding cntx_t* and, in the case of level-3 microkernels, auxinfo_t*) are defined in new headers named, for example, bli_l1v_ker_params.h. Those signatures are reused via macro instantiation when defining both kernel prototypes and kernel function types. This will hopefully make it a little easier to update, add, and manage kernel APIs going forward. - Updated all reference kernels according to the aforementioned switch to void* pointers. - Updated all optimzied kernels according to the aforementioned switch to void* pointers. This sometimes required renaming variables, inserting typecasting so that pointer arithmetic could continue to function as intended, and related tweaks. - Updated sandbox/gemmlike according to the aforementioned switch to void* pointers. - Renamed: - frame/1/bli_l1v_ft_ker.h -> frame/1/bli_l1v_ker_ft.h - frame/1f/bli_l1f_ft_ker.h -> frame/1f/bli_l1f_ker_ft.h - frame/1m/bli_l1m_ft_ker.h -> frame/1m/bli_l1m_ker_ft.h - frame/3/bli_l1m_ft_ukr.h -> frame/3/bli_l1m_ukr_ft.h - frame/3/bli_l3_sup_ft_ker.h -> frame/3/bli_l3_sup_ker_ft.h to better align with naming of neighboring files. - Added the missing "void* params" argument to bli_?packm_struc_cxk() in frame/1m/packm/bli_packm_struc_cxk.c. This argument is being passed into the function from bli_packm_blk_var1(), but wasn't being "caught" by the function definition itself. The function prototype for bli_?packm_struc_cxk() also needed updating. - Reordered the last two parameters in bli_?packm_struc_cxk(). (Previously, the "void* params" was passed in after the "const cntx_t* cntx", although because of the above bug the params argument wasn't actually present in the function definition.) - (cherry picked from fab18dc) Use 'const' pointers in kernel APIs. (#722) Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels. - (cherry picked from 93c63d1)

Details: - Updated common.mk so that when --disable-shared option is given to configure: 1. The -fPIC compiler flag is omitted from the individual configuration family members' CPICFLAGS variables (which are initialized in each subconfig's make_defs.mk file); and 2. The BUILD_SYMFLAGS variable, which contains compiler flags needed to control the symbol export behavior, is left blank. - The net result of these changes is that flags specific to shared library builds are only used when a shared library is actually scheduled to be built. Thanks to Nick Knight for reporting this issue. - CREDITS file update. - (cherry picked from 5f84130) Updated configure to pass all shellcheck checks. (#729) Details: - Modified configure so that it passes all 'shellcheck' checks, disabling ones which we violate but which are just stylistic, or are special cases in our code. - Miscellaneous other minor changes, such as rearranged redirections in long sed/perl pipes to look more natural. - Whitespace tweaks. - (cherry picked from 72c37eb) Fixed bugs in scal2v ref kernel when alpha == 1. (#728) Details: - Fixed a typo bug in ref_kernels/1/bli_scal2v_ref.c where the conditional that was supposed to be checking for cases when alpha is equal to 1.0 (so that copyv could be used instead of scal2v) was instead erroneously comparing alpha against 0.0. - Fixed another bug in the same function whereby BLIS_NO_CONJUGATE was erroneously being passed into copyv instead of the kernel's conjx parameter. This second bug was inert, however, due to the first bug since the "alpha == 0.0" case was already being handled, resulting in the code block never executing. - (cherry picked from 60f3634) Use 'void*' datatypes in kernel APIs. (#727) Details: - Migrated all kernel APIs to use void* pointers instead of float*, double*, scomplex*, and dcomplex* pointers. This allows us to define many fewer kernel function pointer types, which also makes it much easier to know which function pointer type to use at any given time. (For example, whereas before there was ?axpyv_ker_ft, ?axpyv_ker_vft, and axpyv_ker_vft, now there is just axpyv_ker_ft, which is equivalent so what axpyv_ker_vft used to be.) - Refactored how kernel function prototypes and kernel function types are defined so as to reduce redundant code. Specifically, the function signatures (excluding cntx_t* and, in the case of level-3 microkernels, auxinfo_t*) are defined in new headers named, for example, bli_l1v_ker_params.h. Those signatures are reused via macro instantiation when defining both kernel prototypes and kernel function types. This will hopefully make it a little easier to update, add, and manage kernel APIs going forward. - Updated all reference kernels according to the aforementioned switch to void* pointers. - Updated all optimzied kernels according to the aforementioned switch to void* pointers. This sometimes required renaming variables, inserting typecasting so that pointer arithmetic could continue to function as intended, and related tweaks. - Updated sandbox/gemmlike according to the aforementioned switch to void* pointers. - Renamed: - frame/1/bli_l1v_ft_ker.h -> frame/1/bli_l1v_ker_ft.h - frame/1f/bli_l1f_ft_ker.h -> frame/1f/bli_l1f_ker_ft.h - frame/1m/bli_l1m_ft_ker.h -> frame/1m/bli_l1m_ker_ft.h - frame/3/bli_l1m_ft_ukr.h -> frame/3/bli_l1m_ukr_ft.h - frame/3/bli_l3_sup_ft_ker.h -> frame/3/bli_l3_sup_ker_ft.h to better align with naming of neighboring files. - Added the missing "void* params" argument to bli_?packm_struc_cxk() in frame/1m/packm/bli_packm_struc_cxk.c. This argument is being passed into the function from bli_packm_blk_var1(), but wasn't being "caught" by the function definition itself. The function prototype for bli_?packm_struc_cxk() also needed updating. - Reordered the last two parameters in bli_?packm_struc_cxk(). (Previously, the "void* params" was passed in after the "const cntx_t* cntx", although because of the above bug the params argument wasn't actually present in the function definition.) - (cherry picked from fab18dc) Use 'const' pointers in kernel APIs. (#722) Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels. - (cherry picked from 93c63d1)

Make the BLIS configure script pass all shellcheck checks

64690e0

leekillough commented Mar 19, 2023

View reviewed changes

leekillough added 2 commits March 19, 2023 12:58

Fix typo, tab indentation; remove essential changes in comments; rear…

36e365b

…range redirections in long sed/perl pipes to look more natural

add back quotes in comments to reduce differences; change indentation…

b979add

… to use only tabs

Whitespace tweaks.

4447bf9

fgvanzee requested a review from devinamatthews March 21, 2023 16:35

fgvanzee merged commit 72c37eb into flame:master Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the BLIS configure script pass all shellcheck checks#729

Make the BLIS configure script pass all shellcheck checks#729
fgvanzee merged 4 commits intoflame:masterfrom
leekillough:shellcheck_configure

leekillough commented Mar 16, 2023 •

edited

Loading

Uh oh!

leekillough Mar 19, 2023 •

edited

Loading

Uh oh!

fgvanzee Mar 20, 2023

Uh oh!

leekillough Mar 19, 2023

Uh oh!

devinamatthews commented Mar 19, 2023

Uh oh!

fgvanzee commented Mar 20, 2023

Uh oh!

fgvanzee commented Mar 21, 2023

Uh oh!

fgvanzee commented Mar 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

leekillough commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leekillough Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgvanzee Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

leekillough Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

devinamatthews commented Mar 19, 2023

Uh oh!

fgvanzee commented Mar 20, 2023

Uh oh!

fgvanzee commented Mar 21, 2023

Uh oh!

fgvanzee commented Mar 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leekillough commented Mar 16, 2023 •

edited

Loading

leekillough Mar 19, 2023 •

edited

Loading