Hello Romain,



Thank you for your review on the series.

I will respin. I just changed the series to Changes Requested.



On Wed, Jul 27, 2022 at 09:54 AM, Romain Naour wrote:



> Le 24/07/2022 à 07:49, Ricardo Martincoski a écrit :

>> ... just like check-flake8 already does.

>> 

>> When a new check_function is added to check-package, often there are

>> files in the tree that would generate warnings.

>> 

>> An example is the Sob check_function for patch files:

>> | $ ./utils/check-package --i Sob $(git ls-files) >/dev/null

>> | 369301 lines processed

>> | 46 warnings generated

>> Currently these warnings are listed when calling check-package directly,

>> and also at the output of pkg-stats, but the check_function does not run

>> on 'make check-package' (that is used to catch regressions on GitLab CI

>> 'check-package' job) until all warnings in the tree are fixed.

>> This (theoretically) allows new .patch files be added without SoB,

>> without the GitLab CI catching it.

>> 

>> Since now check-package has an ignore file to list all warnings in the

>> tree, that will eventually be fixed, there is no need to filter the

>> files passed to check-package.

>> So test all files in the tree when 'make check-package' is called.

>> It brings following advantages;

>> - any new check_function added to check-package takes place immediately

>>   for new files;

>> - adding new check_functions is less traumatic to the developer doing

>>   this, since he/she does not need anymore to fix all warnings in the

>>   tree before the new check_function takes effect;

>> - prevent regressions, e.g. ANY new .hash file must have 2 spaces;

>> - as a side-effect, print a single statistics line as output of

>>   'make ckeck-package'.

>> 

>> But just enabling the check would generate many warnings when

>> 'make check-package' is called, so update the ignore file by using:

>> $ ./utils/docker-run

>> br-user@...$ ./utils/check-package --failed-only \

>>                `git ls-tree -r --name-only HEAD` > .checkpackageignore

> 

> I guess the ultimate goal of check-package is to make .checkpackageignore empty,

> so at some point this feature should not be useful :)



I don't think we will reach the goal of 0 warnings.

From time to time we can expect new coding style rules to be added to

check-package.



> 

> checkpackageignore is actually needed to convert smoothly the code base to the

> new coding style in order to not conflict too much with pending patches in

> patchwork.



No. It is needed to decouple:

- the style rules we want to enforce for new patches submitted to the list

- from the fix for all files in the tree that does not follow yet the added rule



For instance, this series enforces all new shell scripts to fix (or explicitly

ignore) shellcheck warnings, but it does not change the hundreds of files in

the tree that do not follow that rule just because the rule was not in place

when they were added.



$ grep Shellcheck .checkpackageignore | wc -l

241



> 

> But patchwork is currently "almost" empty (compared to normal) with less than

> 200 patches, so it may be the good opportunity to apply right now a big patch

> removing all HashSpaces and shellcheck warnings (Sob warning are bit more

> complicated to handle though).



shellcheck is not so trivial too.



With this series applied any new patch applied that:

 - adds a patch file without SoB

 - adds a shell script that does not follow shellcheck

will trigger the warning in the CI build.



Someone could even come up with a git pre-commit hook to be used by maintainers ;-)



> 

> The issue with the big patch approach is that the patch must be applied right

> away. Similar to the big patch adding hashes for SourceForge-hosted packages [1].

> 

> Otherwise we have to not forget to update checkpackageignore file when fixing a

> warning in a package.



For developers, even newcomers, that follow the manual 'make check-package'

will warn about it.



And for big patches, it can be updated mechanically using patch 5.



I could also add a helper. When someone calls:

make .checkpackageignore

the file gets updated.





Regards,

Ricardo