| Message ID | 20240501112938.931452-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add shellcheck support | expand |
On Wed, May 01, 2024 at 09:29:29PM GMT, Nicholas Piggin wrote: > This is based on upstream directly now, not ahead of the powerpc > series. > > Since v2: > - Rebased to upstream with some patches merged. > - Just a few comment typos and small issues (e.g., quoting > `make shellcheck` in docs) that people picked up from the > last round. > > `make shellcheck` passes with no warnings for me after this series. > (You should be able to put patch 1 at the end without conflicts if > you prefer to only introduce the shellcheck Makefile target when the > tree is clean.) > > Thanks, > Nick > > Nicholas Piggin (5): > Add initial shellcheck checking > shellcheck: Fix SC2155 > shellcheck: Fix SC2124 > shellcheck: Fix SC2294 > shellcheck: Suppress various messages > > .shellcheckrc | 30 ++++++++++++++++++++++++++++++ > Makefile | 4 ++++ > README.md | 3 +++ > configure | 2 ++ > run_tests.sh | 3 +++ > scripts/arch-run.bash | 33 ++++++++++++++++++++++++++------- > scripts/common.bash | 5 ++++- > scripts/mkstandalone.sh | 2 ++ > scripts/runtime.bash | 6 +++++- > 9 files changed, 79 insertions(+), 9 deletions(-) > create mode 100644 .shellcheckrc > > -- > 2.43.0 > Merged. Thanks, drew
On 01/05/2024 13.29, Nicholas Piggin wrote: > This is based on upstream directly now, not ahead of the powerpc > series. Thanks! ... maybe you could also rebase the powerpc series on this now? (I haven't forgotten about it, just did not find enough spare time for more reviewing yet) > Since v2: > - Rebased to upstream with some patches merged. > - Just a few comment typos and small issues (e.g., quoting > `make shellcheck` in docs) that people picked up from the > last round. When I now run "make shellcheck", I'm still getting an error: In config.mak line 16: AR=ar ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to assign string). Not sure why it's complaining about "ar" but not about the other lines in there? Also, it only seems to work for in-tree builds. If I run it from an out-of-tree build directory, I get: */efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory) Thomas
On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: > On 01/05/2024 13.29, Nicholas Piggin wrote: > > This is based on upstream directly now, not ahead of the powerpc > > series. > > Thanks! ... maybe you could also rebase the powerpc series on this now? (I > haven't forgotten about it, just did not find enough spare time for more > reviewing yet) > > > Since v2: > > - Rebased to upstream with some patches merged. > > - Just a few comment typos and small issues (e.g., quoting > > `make shellcheck` in docs) that people picked up from the > > last round. > > When I now run "make shellcheck", I'm still getting an error: > > In config.mak line 16: > AR=ar > ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to > assign string). I didn't see this one when testing. I have shellcheck version 0.9.0. > > Not sure why it's complaining about "ar" but not about the other lines in there? > > Also, it only seems to work for in-tree builds. If I run it from an > out-of-tree build directory, I get: > > */efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory) I'm glad you checked this. I wish I had before merging :-/ Thanks, drew
On 02/05/2024 10.56, Andrew Jones wrote: > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: >> On 01/05/2024 13.29, Nicholas Piggin wrote: >>> This is based on upstream directly now, not ahead of the powerpc >>> series. >> >> Thanks! ... maybe you could also rebase the powerpc series on this now? (I >> haven't forgotten about it, just did not find enough spare time for more >> reviewing yet) >> >>> Since v2: >>> - Rebased to upstream with some patches merged. >>> - Just a few comment typos and small issues (e.g., quoting >>> `make shellcheck` in docs) that people picked up from the >>> last round. >> >> When I now run "make shellcheck", I'm still getting an error: >> >> In config.mak line 16: >> AR=ar >> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to >> assign string). > > I didn't see this one when testing. I have shellcheck version 0.9.0. I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config? Anyway, I'm in favor of turning this warning of in the config file, it does not seem to be really helpful in my eyes. What do you think? Thomas
On Thu, May 02, 2024 at 11:34:24AM GMT, Thomas Huth wrote: > On 02/05/2024 10.56, Andrew Jones wrote: > > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: > > > On 01/05/2024 13.29, Nicholas Piggin wrote: > > > > This is based on upstream directly now, not ahead of the powerpc > > > > series. > > > > > > Thanks! ... maybe you could also rebase the powerpc series on this now? (I > > > haven't forgotten about it, just did not find enough spare time for more > > > reviewing yet) > > > > > > > Since v2: > > > > - Rebased to upstream with some patches merged. > > > > - Just a few comment typos and small issues (e.g., quoting > > > > `make shellcheck` in docs) that people picked up from the > > > > last round. > > > > > > When I now run "make shellcheck", I'm still getting an error: > > > > > > In config.mak line 16: > > > AR=ar > > > ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to > > > assign string). > > > > I didn't see this one when testing. I have shellcheck version 0.9.0. > > I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config? Yeah, I tested with AArch64, which sets AR to aarch64-linux-gnu-ar. I just tried x86 and see the warning. > > Anyway, I'm in favor of turning this warning of in the config file, it does > not seem to be really helpful in my eyes. What do you think? I agree. The 2209 description says this warning will only appear for commands that are in a hard coded list, so it's an odd check anyway. Thanks, drew
On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote: > On 02/05/2024 10.56, Andrew Jones wrote: > > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: > >> On 01/05/2024 13.29, Nicholas Piggin wrote: > >>> This is based on upstream directly now, not ahead of the powerpc > >>> series. > >> > >> Thanks! ... maybe you could also rebase the powerpc series on this now? (I > >> haven't forgotten about it, just did not find enough spare time for more > >> reviewing yet) > >> > >>> Since v2: > >>> - Rebased to upstream with some patches merged. > >>> - Just a few comment typos and small issues (e.g., quoting > >>> `make shellcheck` in docs) that people picked up from the > >>> last round. > >> > >> When I now run "make shellcheck", I'm still getting an error: > >> > >> In config.mak line 16: > >> AR=ar > >> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to > >> assign string). > > > > I didn't see this one when testing. I have shellcheck version 0.9.0. > > I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config? I have 0.10.0 from Debian with no changes to config defaults and no warning. > Anyway, I'm in favor of turning this warning of in the config file, it does > not seem to be really helpful in my eyes. What do you think? Maybe it would be useful. I don't mind quoting strings usually, although for this kind of pattern it's a bit pointless and config.mak is also Makefile so that has its own issues. Maybe just disable it for this file? Thanks, Nick
On Thu May 2, 2024 at 6:56 PM AEST, Andrew Jones wrote: > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: > > On 01/05/2024 13.29, Nicholas Piggin wrote: > > > This is based on upstream directly now, not ahead of the powerpc > > > series. > > > > Thanks! ... maybe you could also rebase the powerpc series on this now? (I > > haven't forgotten about it, just did not find enough spare time for more > > reviewing yet) > > > > > Since v2: > > > - Rebased to upstream with some patches merged. > > > - Just a few comment typos and small issues (e.g., quoting > > > `make shellcheck` in docs) that people picked up from the > > > last round. > > > > When I now run "make shellcheck", I'm still getting an error: > > > > In config.mak line 16: > > AR=ar > > ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to > > assign string). > > I didn't see this one when testing. I have shellcheck version 0.9.0. > > > > > Not sure why it's complaining about "ar" but not about the other lines in there? > > > > Also, it only seems to work for in-tree builds. If I run it from an > > out-of-tree build directory, I get: > > > > */efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory) > > I'm glad you checked this. I wish I had before merging :-/ I'll send a patch. Possibly some other targets (check-kerneldoc, ctags?) may not work out of tree either. I'll fix this one up first though. Thanks, Nick
On 03/05/2024 07.02, Nicholas Piggin wrote: > On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote: >> On 02/05/2024 10.56, Andrew Jones wrote: >>> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: >>>> On 01/05/2024 13.29, Nicholas Piggin wrote: >>>>> This is based on upstream directly now, not ahead of the powerpc >>>>> series. >>>> >>>> Thanks! ... maybe you could also rebase the powerpc series on this now? (I >>>> haven't forgotten about it, just did not find enough spare time for more >>>> reviewing yet) >>>> >>>>> Since v2: >>>>> - Rebased to upstream with some patches merged. >>>>> - Just a few comment typos and small issues (e.g., quoting >>>>> `make shellcheck` in docs) that people picked up from the >>>>> last round. >>>> >>>> When I now run "make shellcheck", I'm still getting an error: >>>> >>>> In config.mak line 16: >>>> AR=ar >>>> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to >>>> assign string). >>> >>> I didn't see this one when testing. I have shellcheck version 0.9.0. >> >> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config? > > I have 0.10.0 from Debian with no changes to config defaults and no > warning. If I understood it correctly, it warns for AR=ar but it does not warn for AR=powerpc-linux-gnu-ar ... could you try the first term, too, if you haven't done so yet? >> Anyway, I'm in favor of turning this warning of in the config file, it does >> not seem to be really helpful in my eyes. What do you think? > > Maybe it would be useful. I don't mind quoting strings usually, although > for this kind of pattern it's a bit pointless and config.mak is also > Makefile so that has its own issues. Maybe just disable it for this > file? Yes, either for this file only, or globally ... I don't mind. Could you send a patch, please? Thanks, Thomas
On Fri May 3, 2024 at 3:13 PM AEST, Thomas Huth wrote: > On 03/05/2024 07.02, Nicholas Piggin wrote: > > On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote: > >> On 02/05/2024 10.56, Andrew Jones wrote: > >>> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote: > >>>> On 01/05/2024 13.29, Nicholas Piggin wrote: > >>>>> This is based on upstream directly now, not ahead of the powerpc > >>>>> series. > >>>> > >>>> Thanks! ... maybe you could also rebase the powerpc series on this now? (I > >>>> haven't forgotten about it, just did not find enough spare time for more > >>>> reviewing yet) > >>>> > >>>>> Since v2: > >>>>> - Rebased to upstream with some patches merged. > >>>>> - Just a few comment typos and small issues (e.g., quoting > >>>>> `make shellcheck` in docs) that people picked up from the > >>>>> last round. > >>>> > >>>> When I now run "make shellcheck", I'm still getting an error: > >>>> > >>>> In config.mak line 16: > >>>> AR=ar > >>>> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to > >>>> assign string). > >>> > >>> I didn't see this one when testing. I have shellcheck version 0.9.0. > >> > >> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config? > > > > I have 0.10.0 from Debian with no changes to config defaults and no > > warning. > > If I understood it correctly, it warns for AR=ar but it does not warn for > AR=powerpc-linux-gnu-ar ... could you try the first term, too, if you > haven't done so yet? Ah you're right, that does warn here too. Thanks, Nick