[PATCH] gdb: add .pre-commit-config.yaml

[PATCH] gdb: add .pre-commit-config.yaml

[Simon Marchi]

[I'm CCing the binutils list because this patch adds a file at the
 top-level, but it only concerns gdb for now.  But if you are want to use
 pre-commit for some things in binutils too, you are welcome to use that
 file for your needs.]

Add a pre-commit [1] config file, with a single hook to run black
whenever a Python file is modified.  We can always add more hooks if we
find some that are useful.

Using pre-commit to run hooks is opt-in, as in it's not mandatory at all
for development, but it can be useful to run some checks that are easy
to forget (like running black).  The hooks run locally on the
developer's machine when doing `git commit` (although they can also be
configured to run at other stages of the git workflow).

Follow these instructions to install the hooks in your local development
git repository:

 - Install pre-commit the way you prefer.  It can be using your OS
   package manager if it has a recent enough version, or using `pip
   install pre-commit`.
 - Go to the binutils-gdb repository and run `pre-commit install`.

This installs a git hook at `.git/hooks/pre-commit`.

Now, whenever you modify and try to commit a Python file, pre-commit
will run black on it.  For instance, if I try to insert something
misformatted, I get this when doing `git commit`:

    $ git commit
    black....................................................................Failed
    - hook id: black
    - files were modified by this hook

    reformatted gdb/python/lib/gdb/dap/breakpoint.py

    All done! ✨ 🍰 ✨
    1 file reformatted.

At this point, black has already reformatted the files in place, so the
changes that fix the formatting are ready to add and commit.  black is
only ran on files modified in the commit.

The hook defines a black version, which is downloaded at `pre-commit
install` time.  pre-commit manages its own env at
`$HOME/.cache/pre-commit/<some-hash>`, so it won't use the version of
black you have installed already.  This may help ensure that
contributors use the right black version.

The procedure when there is a new version of black (or a new version of
any hook we might be using in the future) is:

 - Modify .pre-commit-config.yaml to change the version number, push to
   the upstream repo.
 - Have contributors run `pre-commit autoupdate` to make their local
   pre-commit installation update the hooks.

It is possible to have pre-commit skip some hooks if needed [2].

I will add these instructions to the wiki if this patch gets merged, so
they are easy to find.  We could perhaps think of having a
gdb/CONTRIBUTING document of some sort checked in the repo with that
kind of information.

I have not used pre-commit in a real project before, but have heard good
things from it.  If we want to give it a try before pushing it to the
repo, some volunteers can copy the .pre-commit-config.yaml file locally
and try it for some time.  However, pushing the file upstream is not
going to impact anybody who doesn't care about it, so I'd say it's
relatively low-risk to push it right now.

[1] https://pre-commit.com
[2] https://pre-commit.com/#temporarily-disabling-hooks

Change-Id: Id00cda882f5140914a670c87e574fa7f2f972099
---
 .pre-commit-config.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 .pre-commit-config.yaml

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
new file mode 100644
index 000000000000..b8d1ef7dc74d
--- /dev/null
+++ b/.pre-commit-config.yaml
@@ -0,0 +1,8 @@
+# See https://pre-commit.com for more information
+# See https://pre-commit.com/hooks.html for more hooks
+repos:
+  - repo: https://github.com/psf/black-pre-commit-mirror
+    rev: 24.2.0
+    hooks:
+      - id: black
+        files: 'gdb/.*'

base-commit: b792eb47f25f577ccef365fc9a5c20d55fad42d5
-- 
2.44.0

Re: [PATCH] gdb: add .pre-commit-config.yaml

[Tom Tromey]

>>>>> Simon Marchi <simon.marchi@efficios.com> writes:

> Using pre-commit to run hooks is opt-in, as in it's not mandatory at all
> for development

Important point for binutils.

> I have not used pre-commit in a real project before, but have heard good
> things from it.

We use it internally at AdaCore.  It seems totally fine, I've rarely had
any issue with it, and it's pretty easy to bypass should the need arise.

So, +1 from me.

thanks,
Tom

Re: [PATCH] gdb: add .pre-commit-config.yaml

[Guinevere Larsen]

I have no real opinion on pre-commit, one way or another, but I like the 
idea of automating easily-forgotten things. could even be upgraded to 
rerunning gdbarch.py at some point!

On 11/03/2024 16:09, Simon Marchi wrote:
> I will add these instructions to the wiki if this patch gets merged, so
> they are easy to find.  We could perhaps think of having a
> gdb/CONTRIBUTING document of some sort checked in the repo with that
> kind of information.

I have positive opinions on this, however! I don't find the contribution 
checklist on the wiki to be a good description of the contribution 
process of GDB. I'm not sure what I know from there and what is 
essential tribal knowledge that I got from internal talks and incorrect 
submissions that were corrected. The maintainers file is the closest we 
have to describing the process, and someone who doesn't know the project 
would not think of looking there, I don't think (I know I didn't).

So I would be very happy to see a contributing file, where the process 
as a whole is described in order, before going down to the minutiae of 
how to make a good contribution. One and done things like pre-commit, 
choosing a way to send email, copyright attribution, can be easily 
viewed by prospecting contributors, and previous contributors can 
refresh their minds or quickly check some recent process change.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH] gdb: add .pre-commit-config.yaml

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> [I'm CCing the binutils list because this patch adds a file at the
>  top-level, but it only concerns gdb for now.  But if you are want to use
>  pre-commit for some things in binutils too, you are welcome to use that
>  file for your needs.]
>
> Add a pre-commit [1] config file, with a single hook to run black
> whenever a Python file is modified.  We can always add more hooks if we
> find some that are useful.
>
> Using pre-commit to run hooks is opt-in, as in it's not mandatory at all
> for development, but it can be useful to run some checks that are easy
> to forget (like running black).  The hooks run locally on the
> developer's machine when doing `git commit` (although they can also be
> configured to run at other stages of the git workflow).
>
> Follow these instructions to install the hooks in your local development
> git repository:
>
>  - Install pre-commit the way you prefer.  It can be using your OS
>    package manager if it has a recent enough version, or using `pip
>    install pre-commit`.
>  - Go to the binutils-gdb repository and run `pre-commit install`.
>
> This installs a git hook at `.git/hooks/pre-commit`.
>
> Now, whenever you modify and try to commit a Python file, pre-commit
> will run black on it.  For instance, if I try to insert something
> misformatted, I get this when doing `git commit`:
>
>     $ git commit
>     black....................................................................Failed
>     - hook id: black
>     - files were modified by this hook
>
>     reformatted gdb/python/lib/gdb/dap/breakpoint.py
>
>     All done! ✨ 🍰 ✨
>     1 file reformatted.
>
> At this point, black has already reformatted the files in place, so the
> changes that fix the formatting are ready to add and commit.  black is
> only ran on files modified in the commit.
>
> The hook defines a black version, which is downloaded at `pre-commit
> install` time.  pre-commit manages its own env at
> `$HOME/.cache/pre-commit/<some-hash>`, so it won't use the version of
> black you have installed already.  This may help ensure that
> contributors use the right black version.

This sounds great.  I got a recent commit wrong because the version of
black was rolled forward and I'd not updated locally, so this would
really help.

+1 from me for making this a thing.

Thanks,
Andrew

Re: [PATCH] gdb: add .pre-commit-config.yaml

[Simon Marchi]

On 3/20/24 09:44, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> [I'm CCing the binutils list because this patch adds a file at the
>>  top-level, but it only concerns gdb for now.  But if you are want to use
>>  pre-commit for some things in binutils too, you are welcome to use that
>>  file for your needs.]
>>
>> Add a pre-commit [1] config file, with a single hook to run black
>> whenever a Python file is modified.  We can always add more hooks if we
>> find some that are useful.
>>
>> Using pre-commit to run hooks is opt-in, as in it's not mandatory at all
>> for development, but it can be useful to run some checks that are easy
>> to forget (like running black).  The hooks run locally on the
>> developer's machine when doing `git commit` (although they can also be
>> configured to run at other stages of the git workflow).
>>
>> Follow these instructions to install the hooks in your local development
>> git repository:
>>
>>  - Install pre-commit the way you prefer.  It can be using your OS
>>    package manager if it has a recent enough version, or using `pip
>>    install pre-commit`.
>>  - Go to the binutils-gdb repository and run `pre-commit install`.
>>
>> This installs a git hook at `.git/hooks/pre-commit`.
>>
>> Now, whenever you modify and try to commit a Python file, pre-commit
>> will run black on it.  For instance, if I try to insert something
>> misformatted, I get this when doing `git commit`:
>>
>>     $ git commit
>>     black....................................................................Failed
>>     - hook id: black
>>     - files were modified by this hook
>>
>>     reformatted gdb/python/lib/gdb/dap/breakpoint.py
>>
>>     All done! ✨ 🍰 ✨
>>     1 file reformatted.
>>
>> At this point, black has already reformatted the files in place, so the
>> changes that fix the formatting are ready to add and commit.  black is
>> only ran on files modified in the commit.
>>
>> The hook defines a black version, which is downloaded at `pre-commit
>> install` time.  pre-commit manages its own env at
>> `$HOME/.cache/pre-commit/<some-hash>`, so it won't use the version of
>> black you have installed already.  This may help ensure that
>> contributors use the right black version.
> 
> This sounds great.  I got a recent commit wrong because the version of
> black was rolled forward and I'd not updated locally, so this would
> really help.
> 
> +1 from me for making this a thing.

Thanks!  Given that I only got positive feedback and this file won't
hurt anybody who doesn't care about it, I went ahead and pushed the
patch.  I added Acked-Bys for all those who provided feedback.

I'll now go ahead and updated the wiki.

Simon