refactor(shellcheck): fix all remaining warnings/errors #1019
Conversation
gforsyth
added
improvement
|
@jameslamb one more for you here |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
I left a couple suggestions... leaving a blocking review because I think one is a correctness problem. The others are just small suggestions for improvement.
@ me whenever you want another review.
build.sh
Outdated
| if [[ ${CMAKE_TARGET} != "" ]]; then | ||
| echo "-- Compiling targets: ${CMAKE_TARGET}, verbose=${VERBOSE_FLAG}" | ||
| if [[ ${CMAKE_TARGET[*]} != "" ]]; then | ||
| echo "-- Compiling targets: ${CMAKE_TARGET[*]}, verbose=${VERBOSE_FLAG}" |
There was a problem hiding this comment.
| echo "-- Compiling targets: ${CMAKE_TARGET[*]}, verbose=${VERBOSE_FLAG}" | |
| echo "-- Compiling targets: ${CMAKE_TARGET[@]}, verbose=${VERBOSE_FLAG}" |
Similar to my comment above... I think here you want the values, not the keys (because there are no keys).
There was a problem hiding this comment.
Yeah, I think this is another weird implicit vs. explicit array concatenation thing -- shellcheck doesn't like it when strings and arrays are mixed: https://github.com/koalaman/shellcheck/wiki/SC2145
There was a problem hiding this comment.
huh, well, I'm definitely wrong here because that didn't output any values
There was a problem hiding this comment.
Hmm, or maybe CMAKE_TARGET was just empty? I've pushed up a different way of spelling this, in any case, but I think this should work.
> CMAKE_TARGET=('one' 'two')
> echo "-- Compiling targets: ${CMAKE_TARGET[*]}"
-- Compiling targets: one twoCo-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
There was a problem hiding this comment.
Would it make sense (if it's possible) to exclude this file from cpp-ownership?
gforsyth
force-pushed
the
moar_shellcheck
branch
from
437e909 to
b30809a
Compare
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Approving, I think all of my comments have been addressed.
Thanks for finishing this one up! This project has a bit more shell code than RAPIDS repos, because it has bindings for so many languages and scripts to build those bindings. I think we'll be really glad to have shellcheck helping us with development here 😁
|
/merge |
4 participants
Finishes up the shellcheck work tracked in rapidsai/build-planning#135