[v5,3/7] insane.bbclass: Make do_qa_staging check shebangs

As reported in the bug report [1], there was no check for shebang
sizes on native scripts and now this is fixed.

The path scope of the qa_staging was increased from just checking
libdir to all the relevant SYSROOT_DIRS.

It is possible to skip this check through INSANE_SKIP.

[1] https://bugzilla.yoctoproject.org/show_bug.cgi?id=11053

Signed-off-by: Paulo Neves <ptsneves@gmail.com>
---
 meta/classes/insane.bbclass | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

On Tue, 2022-06-14 at 17:11 +0200, Paulo Neves wrote:
> As reported in the bug report [1], there was no check for shebang
> sizes on native scripts and now this is fixed.
> 
> The path scope of the qa_staging was increased from just checking
> libdir to all the relevant SYSROOT_DIRS.
> 
> It is possible to skip this check through INSANE_SKIP.
> 
> [1] https://bugzilla.yoctoproject.org/show_bug.cgi?id=11053
> 
> Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> ---
>  meta/classes/insane.bbclass | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 9ca84bace9..b2951a48fe 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -630,6 +630,11 @@ def qa_check_staged(path,d):
>          bb.note("Recipe %s skipping qa checking: pkgconfig" % d.getVar('PN'))
>          skip_pkgconfig = True
>  
> +    skip_shebang_size = False
> +    if 'shebang-size' in skip:
> +        bb.note("Recipe %s skipping qa checkking: shebang-size" % d.getVar('PN'))
> +        skip_shebang_size = True
> +
>      # find all .la and .pc files
>      # read the content
>      # and check for stuff that looks wrong
> @@ -651,6 +656,13 @@ def qa_check_staged(path,d):
>                          error_msg = "%s failed sanity test (tmpdir) in path %s" % (file,root)
>                          oe.qa.handle_error("pkgconfig", error_msg, d)
>  
> +            if not skip_shebang_size:
> +                errors = {}
> +                package_qa_check_shebang_size(path, "", d, None, errors)
> +                for e in errors:
> +                    oe.qa.handle_error(e, errors[e], d)
> +
> +
>  # Run all package-wide warnfuncs and errorfuncs
>  def package_qa_package(warnfuncs, errorfuncs, package, d):
>      warnings = {}
> @@ -1139,7 +1151,9 @@ addtask do_package_qa_setscene
>  
>  python do_qa_staging() {
>      bb.note("QA checking staging")
> -    qa_check_staged(d.expand('${SYSROOT_DESTDIR}${libdir}'), d)
> +    sysroot_destdir = d.expand('${SYSROOT_DESTDIR}')
> +    for sysroot_dir in d.expand('${SYSROOT_DIRS}').split():
> +        qa_check_staged(sysroot_destdir + sysroot_dir, d)
>      oe.qa.exit_with_message_if_errors("QA staging was broken by the package built above", d)
>  }


I'm a little worried about the performance implications of this, we're
going from scanning files in libdir to scanning nearly all files in the
sysroots, reading from many of them. In isolation that doesn't seem
much but I suspect the IO will impact builds overall.

That leaves me a little torn on this change :/

Cheers,

Richard

Yes, when I was making the change I also realized the scope of the check
was quite bigger than before. I am going to deliver the final changes
requested and leave it up to you to decide on the merge. Let me know your
decision so I can close the bug as well.

Paulo Neves

On Fri, Jun 17, 2022, 18:50 Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Tue, 2022-06-14 at 17:11 +0200, Paulo Neves wrote:
> > As reported in the bug report [1], there was no check for shebang
> > sizes on native scripts and now this is fixed.
> >
> > The path scope of the qa_staging was increased from just checking
> > libdir to all the relevant SYSROOT_DIRS.
> >
> > It is possible to skip this check through INSANE_SKIP.
> >
> > [1] https://bugzilla.yoctoproject.org/show_bug.cgi?id=11053
> >
> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> > ---
> >  meta/classes/insane.bbclass | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> > index 9ca84bace9..b2951a48fe 100644
> > --- a/meta/classes/insane.bbclass
> > +++ b/meta/classes/insane.bbclass
> > @@ -630,6 +630,11 @@ def qa_check_staged(path,d):
> >          bb.note("Recipe %s skipping qa checking: pkgconfig" %
> d.getVar('PN'))
> >          skip_pkgconfig = True
> >
> > +    skip_shebang_size = False
> > +    if 'shebang-size' in skip:
> > +        bb.note("Recipe %s skipping qa checkking: shebang-size" %
> d.getVar('PN'))
> > +        skip_shebang_size = True
> > +
> >      # find all .la and .pc files
> >      # read the content
> >      # and check for stuff that looks wrong
> > @@ -651,6 +656,13 @@ def qa_check_staged(path,d):
> >                          error_msg = "%s failed sanity test (tmpdir) in
> path %s" % (file,root)
> >                          oe.qa.handle_error("pkgconfig", error_msg, d)
> >
> > +            if not skip_shebang_size:
> > +                errors = {}
> > +                package_qa_check_shebang_size(path, "", d, None, errors)
> > +                for e in errors:
> > +                    oe.qa.handle_error(e, errors[e], d)
> > +
> > +
> >  # Run all package-wide warnfuncs and errorfuncs
> >  def package_qa_package(warnfuncs, errorfuncs, package, d):
> >      warnings = {}
> > @@ -1139,7 +1151,9 @@ addtask do_package_qa_setscene
> >
> >  python do_qa_staging() {
> >      bb.note("QA checking staging")
> > -    qa_check_staged(d.expand('${SYSROOT_DESTDIR}${libdir}'), d)
> > +    sysroot_destdir = d.expand('${SYSROOT_DESTDIR}')
> > +    for sysroot_dir in d.expand('${SYSROOT_DIRS}').split():
> > +        qa_check_staged(sysroot_destdir + sysroot_dir, d)
> >      oe.qa.exit_with_message_if_errors("QA staging was broken by the
> package built above", d)
> >  }
>
>
> I'm a little worried about the performance implications of this, we're
> going from scanning files in libdir to scanning nearly all files in the
> sysroots, reading from many of them. In isolation that doesn't seem
> much but I suspect the IO will impact builds overall.
>
> That leaves me a little torn on this change :/
>
> Cheers,
>
> Richard
>
>

On Fri, 2022-06-17 at 17:50 +0100, Richard Purdie via
lists.openembedded.org wrote:
> On Tue, 2022-06-14 at 17:11 +0200, Paulo Neves wrote:
> > As reported in the bug report [1], there was no check for shebang
> > sizes on native scripts and now this is fixed.
> > 
> > The path scope of the qa_staging was increased from just checking
> > libdir to all the relevant SYSROOT_DIRS.
> > 
> > It is possible to skip this check through INSANE_SKIP.
> > 
> > [1] https://bugzilla.yoctoproject.org/show_bug.cgi?id=11053
> > 
> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> > ---
> >  meta/classes/insane.bbclass | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> > index 9ca84bace9..b2951a48fe 100644
> > --- a/meta/classes/insane.bbclass
> > +++ b/meta/classes/insane.bbclass
> > @@ -630,6 +630,11 @@ def qa_check_staged(path,d):
> >          bb.note("Recipe %s skipping qa checking: pkgconfig" % d.getVar('PN'))
> >          skip_pkgconfig = True
> >  
> > +    skip_shebang_size = False
> > +    if 'shebang-size' in skip:
> > +        bb.note("Recipe %s skipping qa checkking: shebang-size" % d.getVar('PN'))
> > +        skip_shebang_size = True
> > +
> >      # find all .la and .pc files
> >      # read the content
> >      # and check for stuff that looks wrong
> > @@ -651,6 +656,13 @@ def qa_check_staged(path,d):
> >                          error_msg = "%s failed sanity test (tmpdir) in path %s" % (file,root)
> >                          oe.qa.handle_error("pkgconfig", error_msg, d)
> >  
> > +            if not skip_shebang_size:
> > +                errors = {}
> > +                package_qa_check_shebang_size(path, "", d, None, errors)
> > +                for e in errors:
> > +                    oe.qa.handle_error(e, errors[e], d)
> > +
> > +
> >  # Run all package-wide warnfuncs and errorfuncs
> >  def package_qa_package(warnfuncs, errorfuncs, package, d):
> >      warnings = {}
> > @@ -1139,7 +1151,9 @@ addtask do_package_qa_setscene
> >  
> >  python do_qa_staging() {
> >      bb.note("QA checking staging")
> > -    qa_check_staged(d.expand('${SYSROOT_DESTDIR}${libdir}'), d)
> > +    sysroot_destdir = d.expand('${SYSROOT_DESTDIR}')
> > +    for sysroot_dir in d.expand('${SYSROOT_DIRS}').split():
> > +        qa_check_staged(sysroot_destdir + sysroot_dir, d)
> >      oe.qa.exit_with_message_if_errors("QA staging was broken by the package built above", d)
> >  }
> 
> 
> I'm a little worried about the performance implications of this, we're
> going from scanning files in libdir to scanning nearly all files in the
> sysroots, reading from many of them. In isolation that doesn't seem
> much but I suspect the IO will impact builds overall.
> 
> That leaves me a little torn on this change :/

Sorry about the delay on this. My concern isn't an issue as I wasn't
remembering the exact context of this code correctly. The patches have
therefore merged, thanks!

Cheers,

Richard