Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Tom de Vries <tdevries@suse.de>
To: Thiago Jung Bauermann <thiago.bauermann@linaro.org>,
	Simon Marchi <simark@simark.ca>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] [pre-commit] Add shellcheck
Date: Mon, 22 Jun 2026 12:02:41 +0200	[thread overview]
Message-ID: <6174eca4-a189-4f38-bd69-a6312c361d20@suse.de> (raw)
In-Reply-To: <874iivunvq.fsf@linaro.org>

On 6/22/26 7:48 AM, Thiago Jung Bauermann wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2026-06-18 11:19, Tom de Vries wrote:
>>> I found a pure python implementation of shellcheck [1].
>>>
>>> Use it to run shellcheck on scripts in the repo.
>>>
>>> Exclude any scripts that are not currently clean.
>>>
>>> Running it seems reasonably fast:
>>> ...
>>> $ pre-commit run shellcheck --all-files -v
>>> shellcheck...............................................................Passed
>>> - hook id: shellcheck
>>> - duration: 0.06s
>>> ...
>>>
>>> For information on other solutions, see this RFC [2].
>>>
>>> [1] https://pypi.org/project/pureshellcheck/0.2.2/
>>> [2] https://sourceware.org/pipermail/gdb-patches/2024-November/213400.html
>>
>> While I'm sympathetic to the use of pre-commit (I added the first
>> hooks), I'm starting to get a bit worried that we kind of blindly pull
>> hooks from random places without paying much attention.  This is going
>> to get executed on the machines of many GDB devs, and probably some CI
>> too.
>>
>> I had this thought because this one has the "random project on github
>> vibes" (it appears to be a vibe coded project started a week ago).  More
>> established projects (like black) are not immune to being compromised,
>> but they are easier to trust I guess.
> 
> I have the same kind of concern, but I'm more paranoid. As a general
> principle I prefer to install distro packages rather than
> language-specific packages (such as the ones from PyPI), on the
> assumption that distros tend to be more careful about incorporating
> packages and updating them (it's a weak assumption though).
> 
> Because of that, in order to run the pre-commit checks I created a
> script (attached) that parses .pre-commit-config.yaml and runs the hooks
> with the tools that are already installed on the system. It doesn't try
> to download anything.
> 
> Today I added support for running each hook in an isolated container
> with access only to the binutils-gdb repo, and in read-only mode at
> that. It uses Guix to set up the containers so when Guix isn't installed
> you need to pass the --no-container option.
> 
> Also when using containers the script can only use the tools packaged in
> Guix, which currently doesn't have some of the tools referenced by our
> .pre-commit-config.yaml, or has older versions of them.
> 
> Another bad news is that the script is in Scheme. :)
> 
> But you get the idea. :)
> 

Hi Thiago,

thanks for sharing your point of view and the script.

My take on this is as follows.

The benefit of git hooks is automation.

The benefit of using the pre-commit framework to manage those hooks is:
- version pinning, and
- seamless installation
in other words, making it easy that everybody does the same check.

That comes with the drawback of a download-heavy approach, but that's 
the trade-off.

Your approach has a different trade-off.

We could do something similar (no downloading) for shellcheck:
...
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 9910bbb82f2..92c56d95977 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -130,3 +130,10 @@ repos:
        language: unsupported_script
        entry: gdb/contrib/check-file-mode.sh
        files: '^(gdb|gdbserver|gdbsupport)/.*$'
+    - id: &id5 shellcheck
+      name: *id5
+      language: unsupported
+      entry: shellcheck
+      files: '^(gdb|gdbsupport|gdbserver)/'
+      types: ['shell']
+      exclude: 
'^(gdb/config/djgpp/(djcheck.sh|djconfig.sh)|gdb/contrib/ari/(create-web-ari-
in-src.sh|gdb_ari.sh|update-web-ari.sh)|gdb/contrib/(cc-with-tweaks.sh|gdb-add-index.sh)|gdb/gdb_builda
ll.sh|gdb/gdb_mbuild.sh|gdb/regformats/regdat.sh|gdb/po/gdbtext|gdb/testsuite/lib/pdtrace.in)$|/configu
re$'
...

This means there's no easy installation, people have to install it 
themselves.

There's also no version pinning.  We could add a script that does a 
version check though, and check for version 0.11.0.

My laptop runs Leap 16.0, which has shellcheck 0.10.0, so I would have 
to run pre-commit in a tumbleweed container.  Or build shellcheck from 
source and add it to my path.

But yeah, I'd prefer to not have to bother with that, and pureshellcheck 
does provide an easy solution.

Thanks,
- Tom

      reply	other threads:[~2026-06-22 10:03 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-18 15:19 Tom de Vries
2026-06-18 17:05 ` Simon Marchi
2026-06-19 11:19   ` Tom de Vries
2026-06-19 17:56     ` Simon Marchi
2026-06-22  8:58       ` Tom de Vries
2026-06-22  5:48   ` Thiago Jung Bauermann
2026-06-22 10:02     ` Tom de Vries [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6174eca4-a189-4f38-bd69-a6312c361d20@suse.de \
    --to=tdevries@suse.de \
    --cc=gdb-patches@sourceware.org \
    --cc=simark@simark.ca \
    --cc=thiago.bauermann@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox