Re: [gentoo-dev] [PATCH 06/12] python-utils-r1.eclass: Don't die in python_fix_shebang if all fixed

[James Le Cuirot]

On Sat, 05 Jan 2019 22:42:27 +0100
Michał Górny <mgorny@g.o> wrote:

> On Sat, 2019-01-05 at 21:38 +0000, James Le Cuirot wrote:
> > On Fri, 04 Jan 2019 19:26:34 +0100
> > Michał Górny <mgorny@g.o> wrote:
> >
> > > On Fri, 2019-01-04 at 13:10 -0500, Mike Gilbert wrote:
> > > > On Fri, Jan 4, 2019 at 10:55 AM Michał Górny <mgorny@g.o> wrote:
> > > > >
> > > > > On Thu, 2019-01-03 at 21:39 +0000, James Le Cuirot wrote:
> > > > > > Shebangs may need fixing on prefix systems or when cross-building but
> > > > > > not at other times.
> > > > > >
> > > > > > Signed-off-by: James Le Cuirot <chewi@g.o>
> > > > > > ---
> > > > > >  eclass/python-utils-r1.eclass | 8 ++------
> > > > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/eclass/python-utils-r1.eclass b/eclass/python-utils-r1.eclass
> > > > > > index 19cfaf2798ab..91e457f3cf14 100644
> > > > > > --- a/eclass/python-utils-r1.eclass
> > > > > > +++ b/eclass/python-utils-r1.eclass
> > > > > > @@ -1328,16 +1328,12 @@ python_fix_shebang() {
> > > > > >                       fi
> > > > > >               done < <(find -H "${path}" -type f -print0 || die)
> > > > > >
> > > > > > -             if [[ ! ${any_fixed} ]]; then
> > > > > > +             if [[ ! ${any_fixed} && ! ${any_correct} ]]; then
> > > > > >                       local cmd=eerror
> > > > > >                       [[ ${EAPI:-0} == [012345] ]] && cmd=eqawarn
> > > > > >
> > > > > >                       "${cmd}" "QA warning: ${FUNCNAME}, ${path#${D%/}} did not match any fixable files."
> > > > > > -                     if [[ ${any_correct} ]]; then
> > > > > > -                             "${cmd}" "All files have ${EPYTHON} shebang already."
> > > > > > -                     else
> > > > > > -                             "${cmd}" "There are no Python files in specified directory."
> > > > > > -                     fi
> > > > > > +                     "${cmd}" "There are no Python files in specified directory."
> > > > > >
> > > > > >                       [[ ${cmd} == eerror ]] && die "${FUNCNAME} did not match any fixable files (QA warning fatal in EAPI ${EAPI})"
> > > > > >               fi
> > > > >
> > > > > Sounds like you're introducing breakage, then abusing a function to fix
> > > > > your breakage, then killing a useful diagnostic because you've just
> > > > > broken it.
> > > >
> > > > I'm unable to make sense of what you are trying to say here. Is there
> > > > something wrong with this patch, or are you making a general comment
> > > > on the patch series?
> > > >
> > >
> > > Original usage: rewrite 'python' to 'pythonX.Y', etc. on static files.
> > > Throws an error if you try to use for files that have correct shebang
> > > already.
> > >
> > > Now:
> > >
> > > 1. Due to PYTHON override, generated files frequently have wrong
> > > shebangs.
> > >
> > > 2. python_fix_shebang is modified to fix this -- orthogonal
> > > behavior is introduced.
> > >
> > > 3. Now diagnostic on files with correct shebang is gone because it
> > > conflicts with the orthogonal behavior added here.
> >
> > The diagnostic is a problem even without my changes.
> >
> > It's quite likely upstream could hardcode a shebang like
> > #!/usr/bin/python2.7, which does not need to be modified on normal
> > systems but does on prefixed systems.
>
> Fixing prefix was never the goal.

And fixing prefix is a bad thing? The prefix team does seem interested
in this work. There is a good deal of overlap here and in making sure I
wasn't making prefix worse, I had to stop and consider how this would
help them too.

> > What about hardcoding #!/usr/bin/python3.6? This would need correcting
> > against 3.7 but would fall foul of the diagnostic when using 3.6.
>
> No, it would fail as mismatched shebang.  By design.

Okay, I only thought of this case while writing the mail, having
forgotten the precise logic since working on it. It doesn't take
anything away from the point above though.

> python_fix_shebang is meant to fix common cases of generic shebangs
> in a safe way, not serve as a generic shebang replacement tool.

Allow me to suggest some options for a compromise. We can't just make
the diagnostic conditional on cross/prefix cases as it is the other
cases where it would blow up. This leaves us with:

(a) In cross/prefix cases only, call python_fix_shebang on all files
that look like they have a Python-like shebang in pkg_postinst.
Unfortunately the Python eclasses don't currently export a pkg_postinst
phase function so existing ebuilds using other eclasses may need
fixing. These files could be efficiently found with help from awk using
something along the lines of this:

find "${D}" -type f -exec awk '/^#!.*python/ {print FILENAME} {nextfile}' {} +

(b) Same as (a) but I'll invoke this from cross-boss using bashrc
instead of relying on the eclasses. Prefix would need to do its own thing.

(c) Create a second variant of python_fix_shebang (name TBD) that would
only do something in cross/prefix cases. This would be called against
the files that only fail in these cases. It would have to be gradually
added to ebuilds as problems are found but that was also the case in my
original proposal.

-- 
James Le Cuirot (chewi)
Gentoo Linux Developer