[ANN] pylint and shellcheck

[ANN] pylint and shellcheck

[Jakub Kicinski]

Hi!

It's merge window time so I have a bit of time to catch up on random
things. I added shellcheck, yamllint and pylint:
https://github.com/linux-netdev/nipa/commit/c0fe53ae533d19c19d2e00955403fb57c3679084
https://github.com/linux-netdev/nipa/commit/255ee0295a096ee7096bebd9d640388acc590da0
https://github.com/linux-netdev/nipa/commit/54e060c9094e33bffe356b5d3e25853e22235d49
to the netdev patchwork checks.

They will likely be pretty noisy so please take them with a grain of
salt (pretty much like checkpatch). Using the NIPA scripts from the
commits above could be useful to find the delta of new warnings, since
there will be quite a few existing ones.

I suspect as we get more experience we will find the warning types to
disable, and we will drive the number of existing errors down to make
checking for new ones less of a pain. As I said, for now please don't
take these checks failing at face value.

Re: [ANN] pylint and shellcheck

[Simon Horman]

On Tue, Jun 03, 2025 at 12:06:39PM -0700, Jakub Kicinski wrote:
> Hi!
> 
> It's merge window time so I have a bit of time to catch up on random
> things. I added shellcheck, yamllint and pylint:
> https://github.com/linux-netdev/nipa/commit/c0fe53ae533d19c19d2e00955403fb57c3679084
> https://github.com/linux-netdev/nipa/commit/255ee0295a096ee7096bebd9d640388acc590da0
> https://github.com/linux-netdev/nipa/commit/54e060c9094e33bffe356b5d3e25853e22235d49
> to the netdev patchwork checks.
> 
> They will likely be pretty noisy so please take them with a grain of
> salt (pretty much like checkpatch). Using the NIPA scripts from the
> commits above could be useful to find the delta of new warnings, since
> there will be quite a few existing ones.
> 
> I suspect as we get more experience we will find the warning types to
> disable, and we will drive the number of existing errors down to make
> checking for new ones less of a pain. As I said, for now please don't
> take these checks failing at face value.

Thanks Jakub,

I agree this is a good step.

Anecdotally, my feeling from running shellcheck over patches for a little
while now is that the feedback it gives mainly relates to stricter coding
practices which aren't generally followed. And yet the scripts seem to run
reliably in the environments they are intended to run in.

So I'll be interested to see if we end up go for some mix of disabling
warnings and updating (creating!) our preferred coding style for shell
scripts.

</2c>

Re: [ANN] pylint and shellcheck

[Przemek Kitszel]

On 6/4/25 09:27, Simon Horman wrote:
> On Tue, Jun 03, 2025 at 12:06:39PM -0700, Jakub Kicinski wrote:
>> Hi!
>>
>> It's merge window time so I have a bit of time to catch up on random
>> things. I added shellcheck, yamllint and pylint:
>> https://github.com/linux-netdev/nipa/commit/c0fe53ae533d19c19d2e00955403fb57c3679084
>> https://github.com/linux-netdev/nipa/commit/255ee0295a096ee7096bebd9d640388acc590da0
>> https://github.com/linux-netdev/nipa/commit/54e060c9094e33bffe356b5d3e25853e22235d49
>> to the netdev patchwork checks.
>>
>> They will likely be pretty noisy so please take them with a grain of
>> salt (pretty much like checkpatch). Using the NIPA scripts from the
>> commits above could be useful to find the delta of new warnings, since
>> there will be quite a few existing ones.
>>
>> I suspect as we get more experience we will find the warning types to
>> disable, and we will drive the number of existing errors down to make
>> checking for new ones less of a pain. As I said, for now please don't
>> take these checks failing at face value.
> 
> Thanks Jakub,
> 
> I agree this is a good step.
> 
> Anecdotally, my feeling from running shellcheck over patches for a little
> while now is that the feedback it gives mainly relates to stricter coding
> practices which aren't generally followed. And yet the scripts seem to run
> reliably in the environments they are intended to run in.
> 
> So I'll be interested to see if we end up go for some mix of disabling
> warnings and updating (creating!) our preferred coding style for shell
> scripts.
> 
> </2c>
> 

other than quoting rules (shellcheck knows/assumes less than a human
could) I find all other shellcheck warnings to be a good thing to avoid
in all codebases (mixed level of bash familiarity of participants)

Re: [ANN] pylint and shellcheck

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> Hi!
>
> It's merge window time so I have a bit of time to catch up on random
> things. I added shellcheck, yamllint and pylint:
> https://github.com/linux-netdev/nipa/commit/c0fe53ae533d19c19d2e00955403fb57c3679084
> https://github.com/linux-netdev/nipa/commit/255ee0295a096ee7096bebd9d640388acc590da0
> https://github.com/linux-netdev/nipa/commit/54e060c9094e33bffe356b5d3e25853e22235d49
> to the netdev patchwork checks.
>
> They will likely be pretty noisy so please take them with a grain of
> salt (pretty much like checkpatch). Using the NIPA scripts from the
> commits above could be useful to find the delta of new warnings, since
> there will be quite a few existing ones.
>
> I suspect as we get more experience we will find the warning types to
> disable, and we will drive the number of existing errors down to make
> checking for new ones less of a pain. As I said, for now please don't
> take these checks failing at face value.

This is a possible config for yamllint:

extends: default
rules:
  document-start: disable
  brackets:
    max-spaces-inside: 1
  comments:
    min-spaces-from-content: 1
  line-length:
    max: 96

Re: [ANN] pylint and shellcheck

[Jakub Kicinski]

On Wed, 04 Jun 2025 10:41:14 +0100 Donald Hunter wrote:
> This is a possible config for yamllint:
> 
> extends: default
> rules:
>   document-start: disable
>   brackets:
>     max-spaces-inside: 1
>   comments:
>     min-spaces-from-content: 1
>   line-length:
>     max: 96

This fits our current style pretty nicely!

One concern I have is that yamllint walks down the filesystem
CWD down to root or home dir. So if we put this in
Documentation/netlink/.yamllint people running yamllint from main dir:

 $ yamllint Documentation/netlink/specs/netdev.yaml

will be given incorrect warnings, no? Is there a workaround?

Re: [ANN] pylint and shellcheck

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 04 Jun 2025 10:41:14 +0100 Donald Hunter wrote:
>> This is a possible config for yamllint:
>> 
>> extends: default
>> rules:
>>   document-start: disable
>>   brackets:
>>     max-spaces-inside: 1
>>   comments:
>>     min-spaces-from-content: 1
>>   line-length:
>>     max: 96
>
> This fits our current style pretty nicely!
>
> One concern I have is that yamllint walks down the filesystem
> CWD down to root or home dir. So if we put this in
> Documentation/netlink/.yamllint people running yamllint from main dir:
>
>  $ yamllint Documentation/netlink/specs/netdev.yaml
>
> will be given incorrect warnings, no? Is there a workaround?

I don't see a workaround without some kind of wrapper.

Maybe just add a makefile? Looks like that was the approach taken for
Documentation/devicetree/bindings

Re: [ANN] pylint and shellcheck

[Jakub Kicinski]

On Thu, 05 Jun 2025 10:02:10 +0100 Donald Hunter wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> 
> > On Wed, 04 Jun 2025 10:41:14 +0100 Donald Hunter wrote:  
> >> This is a possible config for yamllint:
> >> 
> >> extends: default
> >> rules:
> >>   document-start: disable
> >>   brackets:
> >>     max-spaces-inside: 1
> >>   comments:
> >>     min-spaces-from-content: 1
> >>   line-length:
> >>     max: 96  
> >
> > This fits our current style pretty nicely!
> >
> > One concern I have is that yamllint walks down the filesystem
> > CWD down to root or home dir. So if we put this in
> > Documentation/netlink/.yamllint people running yamllint from main dir:
> >
> >  $ yamllint Documentation/netlink/specs/netdev.yaml
> >
> > will be given incorrect warnings, no? Is there a workaround?  
> 
> I don't see a workaround without some kind of wrapper.
> 
> Maybe just add a makefile? Looks like that was the approach taken for
> Documentation/devicetree/bindings

If we can live with the document-start annoyance I was wondering 
if we can stick to the "default" as is? We can fix existing
docs slowly, the patchwork script will ignore pre-existing warnings.

Re: [ANN] pylint and shellcheck

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 05 Jun 2025 10:02:10 +0100 Donald Hunter wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> 
>> > On Wed, 04 Jun 2025 10:41:14 +0100 Donald Hunter wrote:  
>> >> This is a possible config for yamllint:
>> >> 
>> >> extends: default
>> >> rules:
>> >>   document-start: disable
>> >>   brackets:
>> >>     max-spaces-inside: 1
>> >>   comments:
>> >>     min-spaces-from-content: 1
>> >>   line-length:
>> >>     max: 96  
>> >
>> > This fits our current style pretty nicely!
>> >
>> > One concern I have is that yamllint walks down the filesystem
>> > CWD down to root or home dir. So if we put this in
>> > Documentation/netlink/.yamllint people running yamllint from main dir:
>> >
>> >  $ yamllint Documentation/netlink/specs/netdev.yaml
>> >
>> > will be given incorrect warnings, no? Is there a workaround?  
>> 
>> I don't see a workaround without some kind of wrapper.
>> 
>> Maybe just add a makefile? Looks like that was the approach taken for
>> Documentation/devicetree/bindings
>
> If we can live with the document-start annoyance I was wondering 
> if we can stick to the "default" as is? We can fix existing
> docs slowly, the patchwork script will ignore pre-existing warnings.

TBH I could go through and fix all the warnings, including adding the
document start markers. I can automate most of them with perl
one-liners. The rest would be a few minutes of editing.

I could have patches ready for the merge window opening. What's best, a
patch per type of error, or some other split?

Re: [ANN] pylint and shellcheck

[Jakub Kicinski]

On Fri, 06 Jun 2025 14:06:56 +0100 Donald Hunter wrote:
> I could have patches ready for the merge window opening. What's best, a
> patch per type of error, or some other split?

Maybe, say, patch per error type for those that have > 2 hits,
and for the less common ones.. improvise? :)