utils: checkstyle.py: Warn when no valid Signed-off-by line is found

20756 diff mbox series

All commits to libcamera must include a Signed-off-by line, and that
rule is enforced through git hooks and CI. This however doesn't prevent
patches from being submitted without an SoB tag, as noticed multiple
times in the past. Extend the checkstyle.py trailer checker to issue a
warning when the SoB line is missing to try and improve the situation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/checkstyle.py | 8 ++++++++
 1 file changed, 8 insertions(+)


base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> All commits to libcamera must include a Signed-off-by line, and that
> rule is enforced through git hooks and CI. This however doesn't prevent
> patches from being submitted without an SoB tag, as noticed multiple
> times in the past. Extend the checkstyle.py trailer checker to issue a
> warning when the SoB line is missing to try and improve the situation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

Now how to check that checkstyle.py has been run before a patch is
submitted. ;-)

> ---
>  utils/checkstyle.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 4185c39ac811..7d480bdf4a2f 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -493,6 +493,8 @@ class TrailersChecker(CommitChecker):
>      def check(cls, commit, top_level):
>          issues = []
>  
> +        sob_found = False
> +
>          for trailer in commit.trailers:
>              match = TrailersChecker.trailer_regex.fullmatch(trailer)
>              if not match:
> @@ -515,6 +517,12 @@ class TrailersChecker(CommitChecker):
>                  issues.append(CommitIssue(f"Malformed value '{value}' for commit trailer '{key}'"))
>                  continue
>  
> +            if key == 'Signed-off-by':
> +                sob_found = True
> +
> +        if not sob_found:
> +            issues.append(CommitIssue(f"No valid 'Signed-off-by' trailer found, see Documentation/contributing.rst"))
> +
>          return issues
>  
>  
>
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Hi Milan,

On 8/5/24 12:43 PM, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
>> All commits to libcamera must include a Signed-off-by line, and that
>> rule is enforced through git hooks and CI. This however doesn't prevent
>> patches from being submitted without an SoB tag, as noticed multiple
>> times in the past. Extend the checkstyle.py trailer checker to issue a
>> warning when the SoB line is missing to try and improve the situation.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> 
> Now how to check that checkstyle.py has been run before a patch is
> submitted. ;-)
> 

b4 can do this (though still on client side).

b4 prep --check

if you don't run it once before b4 send is called, it'll complain (but 
not force you to run it).

c.f. b4.prep-perpatch-check-cmd in 
https://b4.docs.kernel.org/en/latest/config.html#contributor-oriented-settings

Cheers,
Quentin

Quoting Quentin Schulz (2024-08-05 11:46:24)
> Hi Milan,
> 
> On 8/5/24 12:43 PM, Milan Zamazal wrote:
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > 
> >> All commits to libcamera must include a Signed-off-by line, and that
> >> rule is enforced through git hooks and CI. This however doesn't prevent
> >> patches from being submitted without an SoB tag, as noticed multiple
> >> times in the past. Extend the checkstyle.py trailer checker to issue a
> >> warning when the SoB line is missing to try and improve the situation.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > 
> > Now how to check that checkstyle.py has been run before a patch is
> > submitted. ;-)
> > 
> 
> b4 can do this (though still on client side).
> 
> b4 prep --check
> 
> if you don't run it once before b4 send is called, it'll complain (but 
> not force you to run it).
> 
> c.f. b4.prep-perpatch-check-cmd in 
> https://b4.docs.kernel.org/en/latest/config.html#contributor-oriented-settings

I always keep the checkstyle hooks as a post-commit hook. I find this the
best way (for me) as it will always run while developing, and provide me
with fast feedback - but not prevent me from continuing in my work.

'cp ./utils/hooks/post-commit .git/hooks/'

Always running, all of the time is a great way IMO to know it's been run
before submitting ;-)

--
Kieran

On Mon, Aug 05, 2024 at 12:25:56PM +0100, Kieran Bingham wrote:
> Quoting Quentin Schulz (2024-08-05 11:46:24)
> > On 8/5/24 12:43 PM, Milan Zamazal wrote:
> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > > 
> > >> All commits to libcamera must include a Signed-off-by line, and that
> > >> rule is enforced through git hooks and CI. This however doesn't prevent
> > >> patches from being submitted without an SoB tag, as noticed multiple
> > >> times in the past. Extend the checkstyle.py trailer checker to issue a
> > >> warning when the SoB line is missing to try and improve the situation.
> > >>
> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > > 
> > > Now how to check that checkstyle.py has been run before a patch is
> > > submitted. ;-)
> > 
> > b4 can do this (though still on client side).
> > 
> > b4 prep --check
> > 
> > if you don't run it once before b4 send is called, it'll complain (but 
> > not force you to run it).
> > 
> > c.f. b4.prep-perpatch-check-cmd in 
> > https://b4.docs.kernel.org/en/latest/config.html#contributor-oriented-settings

I don't think there are easy ways to enforce client-side tests before
patches get submitted, without taking over control of the client
machine. Making sure the tests can be run easily is my favourite
approach. checkstyle.py is easy to use, and when set up as a post-commit
hook, it gets even easier. That's why I try to ensure checkstyle.py will
catch most common issues. If a developer submits a patch with an issue
that checkstyle.py would have caught, we can tell them to install the
git hook.

> I always keep the checkstyle hooks as a post-commit hook. I find this the
> best way (for me) as it will always run while developing, and provide me
> with fast feedback - but not prevent me from continuing in my work.
> 
> 'cp ./utils/hooks/post-commit .git/hooks/'
> 
> Always running, all of the time is a great way IMO to know it's been run
> before submitting ;-)

Quoting Laurent Pinchart (2024-08-04 13:06:56)
> All commits to libcamera must include a Signed-off-by line, and that
> rule is enforced through git hooks and CI. This however doesn't prevent
> patches from being submitted without an SoB tag, as noticed multiple
> times in the past. Extend the checkstyle.py trailer checker to issue a
> warning when the SoB line is missing to try and improve the situation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 4185c39ac811..7d480bdf4a2f 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -493,6 +493,8 @@ class TrailersChecker(CommitChecker):
>      def check(cls, commit, top_level):
>          issues = []
>  
> +        sob_found = False
> +
>          for trailer in commit.trailers:
>              match = TrailersChecker.trailer_regex.fullmatch(trailer)
>              if not match:
> @@ -515,6 +517,12 @@ class TrailersChecker(CommitChecker):
>                  issues.append(CommitIssue(f"Malformed value '{value}' for commit trailer '{key}'"))
>                  continue
>  
> +            if key == 'Signed-off-by':
> +                sob_found = True
> +
> +        if not sob_found:
> +            issues.append(CommitIssue(f"No valid 'Signed-off-by' trailer found, see Documentation/contributing.rst"))
> +

I guess it doesn't make sense to verify that the commiter also has the
signoff in this case as it's the development phase rather than the merge
phase?

But I guess it does catch / help patch submitters know they have to sign
off.

I fear that anyone who /would/ be caught by this warning - wouldn't have
run checkstyle though.

Still, fine with me if you want to add it:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>          return issues
>  
>  
> 
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Mon, Aug 05, 2024 at 05:35:25PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-08-04 13:06:56)
> > All commits to libcamera must include a Signed-off-by line, and that
> > rule is enforced through git hooks and CI. This however doesn't prevent
> > patches from being submitted without an SoB tag, as noticed multiple
> > times in the past. Extend the checkstyle.py trailer checker to issue a
> > warning when the SoB line is missing to try and improve the situation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index 4185c39ac811..7d480bdf4a2f 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -493,6 +493,8 @@ class TrailersChecker(CommitChecker):
> >      def check(cls, commit, top_level):
> >          issues = []
> >  
> > +        sob_found = False
> > +
> >          for trailer in commit.trailers:
> >              match = TrailersChecker.trailer_regex.fullmatch(trailer)
> >              if not match:
> > @@ -515,6 +517,12 @@ class TrailersChecker(CommitChecker):
> >                  issues.append(CommitIssue(f"Malformed value '{value}' for commit trailer '{key}'"))
> >                  continue
> >  
> > +            if key == 'Signed-off-by':
> > +                sob_found = True
> > +
> > +        if not sob_found:
> > +            issues.append(CommitIssue(f"No valid 'Signed-off-by' trailer found, see Documentation/contributing.rst"))
> > +
> 
> I guess it doesn't make sense to verify that the commiter also has the
> signoff in this case as it's the development phase rather than the merge
> phase?

It's a good point. I'll see if I can submit patches, likely on top.

> But I guess it does catch / help patch submitters know they have to sign
> off.
> 
> I fear that anyone who /would/ be caught by this warning - wouldn't have
> run checkstyle though.

Probably, but at least with this we'l have a proof they haven't, and we
can tell them to use checkstyle.py :-)

> Still, fine with me if you want to add it:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >          return issues
> >  
> >  
> > 
> > base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5