| Message ID | 20230812192842.135682-3-dalang@gmx.at |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/8] support/scripts/pkg-stats: fix typos | expand |
On 12/08/2023 21:28, Daniel Lang wrote: > Instead of only checking .mk and Config.in{,.host}, check > all files in a package directory. ("checking" here means "run check-package"). I think we should instead remove the .warnings and .status['pkg-check'] fields entirely. It made sense back in the days, because we weren't running check-package in CI. Now, however, we do, so there's little point to run check-package as part of pkg-stats as well. On a.b.o the warnings column will (almost) always be all 0. There is the point of people using pkg-stats for their br2-external, but: 1. pkg-stats is pretty fragile so users shouldn't be too surprised b some breakage; 2. it's very easy for those users to run 'make check-package' instead. Do other maintainers have the same opinion? Regards, Arnout > > Signed-off-by: Daniel Lang <dalang@gmx.at> > --- > v2 -> v3: > - new patch > --- > support/scripts/pkg-stats | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats > index 1ac538f5f9..c124b8a9cf 100755 > --- a/support/scripts/pkg-stats > +++ b/support/scripts/pkg-stats > @@ -272,8 +272,7 @@ class Package: > self.status['pkg-check'] = ("error", "Missing") > for root, dirs, files in os.walk(pkgdir): > for f in files: > - if f.endswith(".mk") or f.endswith(".hash") or f == "Config.in" or f == "Config.in.host": > - cmd.append(os.path.join(root, f)) > + cmd.append(os.path.join(root, f)) > o = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[1] > lines = o.splitlines() > for line in lines:
On Wed, 30 Aug 2023 22:36:10 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:
> Do other maintainers have the same opinion?
Agreed.
Thomas
Hello Arnout, On 30.08.23 22:36, Arnout Vandecappelle wrote: > > > On 12/08/2023 21:28, Daniel Lang wrote: >> Instead of only checking .mk and Config.in{,.host}, check >> all files in a package directory. > > ("checking" here means "run check-package"). Correct. > > I think we should instead remove the .warnings and .status['pkg-check'] fields entirely. It made sense back in the days, because we weren't running check-package in CI. Now, however, we do, so there's little point to run check-package as part of pkg-stats as well. On a.b.o the warnings column will (almost) always be all 0. It will always be 0 if .checkpackageignore is considered which isn't the case here. Therefore the warning column would show the number of warnings including those ignored. The question of whether or not this information is needed is still relevant. I just wanted to point out that in my testing the value wasn't 0 for all packages. > > There is the point of people using pkg-stats for their br2-external, but: > > 1. pkg-stats is pretty fragile so users shouldn't be too surprised b some breakage; > 2. it's very easy for those users to run 'make check-package' instead. > > > Do other maintainers have the same opinion? Seeing as Thomas already agreed, I will prepare the removal, unless other opinions are raised. > > > Regards, > Arnout Regards, Daniel
On 31/08/2023 21:52, Daniel Lang wrote: > Hello Arnout, > > On 30.08.23 22:36, Arnout Vandecappelle wrote: >> >> >> On 12/08/2023 21:28, Daniel Lang wrote: >>> Instead of only checking .mk and Config.in{,.host}, check >>> all files in a package directory. >> >> ("checking" here means "run check-package"). > > Correct. > >> >> I think we should instead remove the .warnings and .status['pkg-check'] fields entirely. It made sense back in the days, because we weren't running check-package in CI. Now, however, we do, so there's little point to run check-package as part of pkg-stats as well. On a.b.o the warnings column will (almost) always be all 0. > > It will always be 0 if .checkpackageignore is considered which isn't the case > here. Therefore the warning column would show the number of warnings including > those ignored. Oh, very good point, I didn't think of that... On the short term: the reason we introduced .checkpackageignore to begin with is that there are too many errors, so many that we're never going to fix them all. In particular old patches for semi-dead projects. But also the 154 shellcheck errors are probably never going to be fixed. That said, I do see the value in seeing the number of warnings for a package. Right now, if you're looking at the pkg-stats output and decide to e.g. version-bump a package, you'll probably not consider to also fix the shellcheck warnings in the init script. However, if you also see that there's one warning, that may trigger you to update the init script so you're package becomes fully green. In other words, I'm retracting my earlier suggestion of removing the Warnings entirely. Regards, Arnout > The question of whether or not this information is needed is still relevant. > I just wanted to point out that in my testing the value wasn't 0 for all packages. > >> >> There is the point of people using pkg-stats for their br2-external, but: >> >> 1. pkg-stats is pretty fragile so users shouldn't be too surprised b some breakage; >> 2. it's very easy for those users to run 'make check-package' instead. >> >> >> Do other maintainers have the same opinion? > > Seeing as Thomas already agreed, I will prepare the removal, unless other opinions > are raised. > >> >> >> Regards, >> Arnout > > Regards, > Daniel >
diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats index 1ac538f5f9..c124b8a9cf 100755 --- a/support/scripts/pkg-stats +++ b/support/scripts/pkg-stats @@ -272,8 +272,7 @@ class Package: self.status['pkg-check'] = ("error", "Missing") for root, dirs, files in os.walk(pkgdir): for f in files: - if f.endswith(".mk") or f.endswith(".hash") or f == "Config.in" or f == "Config.in.host": - cmd.append(os.path.join(root, f)) + cmd.append(os.path.join(root, f)) o = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[1] lines = o.splitlines() for line in lines:
Instead of only checking .mk and Config.in{,.host}, check all files in a package directory. Signed-off-by: Daniel Lang <dalang@gmx.at> --- v2 -> v3: - new patch --- support/scripts/pkg-stats | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)