diff mbox series

[oe,1/2] lib/oe/patch.py: apply patches from src_uri specified directories

Message ID 20211210130458.39716-2-max.krummenacher@toradex.com
State New
Headers show
Series implement applying patches from a directory | expand

Commit Message

Max Krummenacher Dec. 10, 2021, 1:04 p.m. UTC 
The current developer manual specifies that patches that are
part of a directory which is given in SRC_URI are applied by
the do_patch task. However that is not implemented in the
current code.

Implement part of it with two differences:
- The implementation requires the parameter "apply=yes" and
  adds all files as patches, not only files ending in .patch
  and .diff.
  This keeps recipes which depend on the current implementation
  working.
- The possibility to exclude a file in that directory from being
  applied by a follow up entry with "apply=no" is dropped.

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
---
 meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
 1 file changed, 59 insertions(+), 39 deletions(-)

Comments

Konrad Weihmann Dec. 10, 2021, 1:29 p.m. UTC | #1 
On 10.12.21 14:04, Max Krummenacher wrote:
> The current developer manual specifies that patches that are
> part of a directory which is given in SRC_URI are applied by
> the do_patch task. However that is not implemented in the
> current code.
> 
> Implement part of it with two differences:
> - The implementation requires the parameter "apply=yes" and
>    adds all files as patches, not only files ending in .patch
>    and .diff.
>    This keeps recipes which depend on the current implementation
>    working.
> - The possibility to exclude a file in that directory from being
>    applied by a follow up entry with "apply=no" is dropped.
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> ---
>   meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
>   1 file changed, 59 insertions(+), 39 deletions(-)
> 
> diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> index 950fe723dc..0d2afab2f9 100644
> --- a/meta/lib/oe/patch.py
> +++ b/meta/lib/oe/patch.py
> @@ -794,68 +794,88 @@ class UserResolver(Resolver):
>               raise
>           os.chdir(olddir)
>   
> -
> -def patch_path(url, fetch, workdir, expand=True):
> -    """Return the local path of a patch, or return nothing if this isn't a patch"""
> -
> -    local = fetch.localpath(url)
> -    if os.path.isdir(local):
> -        return
> +def is_patch(local, workdir, apply_all, expand):
>       base, ext = os.path.splitext(os.path.basename(local))
>       if ext in ('.gz', '.bz2', '.xz', '.Z'):
>           if expand:
>               local = os.path.join(workdir, base)
>           ext = os.path.splitext(base)[1]
>   
> -    urldata = fetch.ud[url]
> +    if ext in (".diff", ".patch") or apply_all:
> +        return True
> +    return False
> +
> +def patch_path(local, urldata, fetch, workdir, expand=True):
> +    """Return a list of local paths of patches or return an empty list if there are no patches"""
> +    patches = []
> +
> +    apply_all = False
>       if "apply" in urldata.parm:
>           apply = oe.types.boolean(urldata.parm["apply"])
>           if not apply:
> -            return
> -    elif ext not in (".diff", ".patch"):
> -        return
> +            return patches
> +        else:
> +            apply_all = True
> +
> +    if os.path.isdir(local) and apply_all:
> +        for f in sorted(os.listdir(local)):

This assumes that patches can be ordered (in here alphabetically) - 
which isn't the case for most of the projects I worked with - I think it 
was also a reason why one has to specify the order and inclusion in SRC_URI.

> +            sublocal = local + '/' + f

Wouldn't be a sorted glob be more suitable here?

> +            # Also search in subdirs
> +            if os.path.isdir(sublocal):
> +                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
> +            else:
> +                if is_patch(sublocal, workdir, apply_all, expand):
> +                    patches.append(sublocal)
> +    else:
> +        if is_patch(local, workdir, apply_all, expand):
> +            patches.append(local)
>   
> -    return local
> +    return patches
>   
>   def src_patches(d, all=False, expand=True):
> +    """Return a list of local paths from SRC_URI. With all=False all patches targeting do_patch, with all=True all other local paths"""
>       workdir = d.getVar('WORKDIR')
>       fetch = bb.fetch2.Fetch([], d)
>       patches = []
>       sources = []
> -    for url in fetch.urls:
> -        local = patch_path(url, fetch, workdir, expand)
> -        if not local:
> -            if all:
> -                local = fetch.localpath(url)
> -                sources.append(local)
> -            continue
>   
> +    for url in fetch.urls:
> +        local = fetch.localpath(url)
>           urldata = fetch.ud[url]
> -        parm = urldata.parm
> -        patchname = parm.get('pname') or os.path.basename(local)
> +        locals = []
> +        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
>   
> -        apply, reason = should_apply(parm, d)
> -        if not apply:
> -            if reason:
> -                bb.note("Patch %s %s" % (patchname, reason))
> +        if not locals:
> +            if all:
> +                sources.append(local)
>               continue
> -
> -        patchparm = {'patchname': patchname}
> -        if "striplevel" in parm:
> -            striplevel = parm["striplevel"]
> -        elif "pnum" in parm:
> -            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> -            striplevel = parm["pnum"]
>           else:
> -            striplevel = '1'
> -        patchparm['striplevel'] = striplevel
> +            for patch in locals:
> +                parm = urldata.parm
> +                patchname = parm.get('pname') or os.path.basename(patch)
> +
> +                apply, reason = should_apply(parm, d)
> +                if not apply:
> +                    if reason:
> +                        bb.note("Patch %s %s" % (patchname, reason))
> +                    continue
> +
> +                patchparm = {'patchname': patchname}
> +                if "striplevel" in parm:
> +                    striplevel = parm["striplevel"]
> +                elif "pnum" in parm:
> +                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> +                    striplevel = parm["pnum"]
> +                else:
> +                    striplevel = '1'
> +                patchparm['striplevel'] = striplevel
>   
> -        patchdir = parm.get('patchdir')
> -        if patchdir:
> -            patchparm['patchdir'] = patchdir
> +                patchdir = parm.get('patchdir')
> +                if patchdir:
> +                    patchparm['patchdir'] = patchdir
>   
> -        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
> -        patches.append(localurl)
> +                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
> +                patches.append(localurl)
>   
>       if all:
>           return sources
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#159519): https://lists.openembedded.org/g/openembedded-core/message/159519
> Mute This Topic: https://lists.openembedded.org/mt/87635233/3647476
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [kweihmann@outlook.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Bruce Ashfield Dec. 10, 2021, 2:01 p.m. UTC | #2 
On Fri, Dec 10, 2021 at 8:29 AM Konrad Weihmann <kweihmann@outlook.com> wrote:
>
>
>
> On 10.12.21 14:04, Max Krummenacher wrote:
> > The current developer manual specifies that patches that are
> > part of a directory which is given in SRC_URI are applied by
> > the do_patch task. However that is not implemented in the
> > current code.
> >
> > Implement part of it with two differences:
> > - The implementation requires the parameter "apply=yes" and
> >    adds all files as patches, not only files ending in .patch
> >    and .diff.
> >    This keeps recipes which depend on the current implementation
> >    working.
> > - The possibility to exclude a file in that directory from being
> >    applied by a follow up entry with "apply=no" is dropped.
> >
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > ---
> >   meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
> >   1 file changed, 59 insertions(+), 39 deletions(-)
> >
> > diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> > index 950fe723dc..0d2afab2f9 100644
> > --- a/meta/lib/oe/patch.py
> > +++ b/meta/lib/oe/patch.py
> > @@ -794,68 +794,88 @@ class UserResolver(Resolver):
> >               raise
> >           os.chdir(olddir)
> >
> > -
> > -def patch_path(url, fetch, workdir, expand=True):
> > -    """Return the local path of a patch, or return nothing if this isn't a patch"""
> > -
> > -    local = fetch.localpath(url)
> > -    if os.path.isdir(local):
> > -        return
> > +def is_patch(local, workdir, apply_all, expand):
> >       base, ext = os.path.splitext(os.path.basename(local))
> >       if ext in ('.gz', '.bz2', '.xz', '.Z'):
> >           if expand:
> >               local = os.path.join(workdir, base)
> >           ext = os.path.splitext(base)[1]
> >
> > -    urldata = fetch.ud[url]
> > +    if ext in (".diff", ".patch") or apply_all:
> > +        return True
> > +    return False
> > +
> > +def patch_path(local, urldata, fetch, workdir, expand=True):
> > +    """Return a list of local paths of patches or return an empty list if there are no patches"""
> > +    patches = []
> > +
> > +    apply_all = False
> >       if "apply" in urldata.parm:
> >           apply = oe.types.boolean(urldata.parm["apply"])
> >           if not apply:
> > -            return
> > -    elif ext not in (".diff", ".patch"):
> > -        return
> > +            return patches
> > +        else:
> > +            apply_all = True
> > +
> > +    if os.path.isdir(local) and apply_all:
> > +        for f in sorted(os.listdir(local)):
>
> This assumes that patches can be ordered (in here alphabetically) -
> which isn't the case for most of the projects I worked with - I think it
> was also a reason why one has to specify the order and inclusion in SRC_URI.

This was immediately my concern as well when I read the 0/N for the series.

I'd be more inclined to remove the documentation, rather than trying
to deal with sorting and sequencing issues. Since it hasn't been
working until now, there wouldn't be any users to annoy with dropped
functionality.

(That being said, we could make the argument that anyone attempting to
just point at a directory full of patches knows enough that they need
to have them named in a way to keep them sorted and applied in order).

Cheers,

Bruce


>
> > +            sublocal = local + '/' + f
>
> Wouldn't be a sorted glob be more suitable here?
>
> > +            # Also search in subdirs
> > +            if os.path.isdir(sublocal):
> > +                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
> > +            else:
> > +                if is_patch(sublocal, workdir, apply_all, expand):
> > +                    patches.append(sublocal)
> > +    else:
> > +        if is_patch(local, workdir, apply_all, expand):
> > +            patches.append(local)
> >
> > -    return local
> > +    return patches
> >
> >   def src_patches(d, all=False, expand=True):
> > +    """Return a list of local paths from SRC_URI. With all=False all patches targeting do_patch, with all=True all other local paths"""
> >       workdir = d.getVar('WORKDIR')
> >       fetch = bb.fetch2.Fetch([], d)
> >       patches = []
> >       sources = []
> > -    for url in fetch.urls:
> > -        local = patch_path(url, fetch, workdir, expand)
> > -        if not local:
> > -            if all:
> > -                local = fetch.localpath(url)
> > -                sources.append(local)
> > -            continue
> >
> > +    for url in fetch.urls:
> > +        local = fetch.localpath(url)
> >           urldata = fetch.ud[url]
> > -        parm = urldata.parm
> > -        patchname = parm.get('pname') or os.path.basename(local)
> > +        locals = []
> > +        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
> >
> > -        apply, reason = should_apply(parm, d)
> > -        if not apply:
> > -            if reason:
> > -                bb.note("Patch %s %s" % (patchname, reason))
> > +        if not locals:
> > +            if all:
> > +                sources.append(local)
> >               continue
> > -
> > -        patchparm = {'patchname': patchname}
> > -        if "striplevel" in parm:
> > -            striplevel = parm["striplevel"]
> > -        elif "pnum" in parm:
> > -            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> > -            striplevel = parm["pnum"]
> >           else:
> > -            striplevel = '1'
> > -        patchparm['striplevel'] = striplevel
> > +            for patch in locals:
> > +                parm = urldata.parm
> > +                patchname = parm.get('pname') or os.path.basename(patch)
> > +
> > +                apply, reason = should_apply(parm, d)
> > +                if not apply:
> > +                    if reason:
> > +                        bb.note("Patch %s %s" % (patchname, reason))
> > +                    continue
> > +
> > +                patchparm = {'patchname': patchname}
> > +                if "striplevel" in parm:
> > +                    striplevel = parm["striplevel"]
> > +                elif "pnum" in parm:
> > +                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> > +                    striplevel = parm["pnum"]
> > +                else:
> > +                    striplevel = '1'
> > +                patchparm['striplevel'] = striplevel
> >
> > -        patchdir = parm.get('patchdir')
> > -        if patchdir:
> > -            patchparm['patchdir'] = patchdir
> > +                patchdir = parm.get('patchdir')
> > +                if patchdir:
> > +                    patchparm['patchdir'] = patchdir
> >
> > -        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
> > -        patches.append(localurl)
> > +                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
> > +                patches.append(localurl)
> >
> >       if all:
> >           return sources
> >
> >
> >
> >
> >
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#159521): https://lists.openembedded.org/g/openembedded-core/message/159521
> Mute This Topic: https://lists.openembedded.org/mt/87635233/1050810
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>


--
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II
Max Krummenacher Dec. 10, 2021, 2:07 p.m. UTC | #3 
Am Freitag, den 10.12.2021, 09:01 -0500 schrieb Bruce Ashfield:
> On Fri, Dec 10, 2021 at 8:29 AM Konrad Weihmann <kweihmann@outlook.com> wrote:
> > 
> > 
> > On 10.12.21 14:04, Max Krummenacher wrote:
> > > The current developer manual specifies that patches that are
> > > part of a directory which is given in SRC_URI are applied by
> > > the do_patch task. However that is not implemented in the
> > > current code.
> > > 
> > > Implement part of it with two differences:
> > > - The implementation requires the parameter "apply=yes" and
> > >    adds all files as patches, not only files ending in .patch
> > >    and .diff.
> > >    This keeps recipes which depend on the current implementation
> > >    working.
> > > - The possibility to exclude a file in that directory from being
> > >    applied by a follow up entry with "apply=no" is dropped.
> > > 
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > ---
> > >   meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
> > >   1 file changed, 59 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> > > index 950fe723dc..0d2afab2f9 100644
> > > --- a/meta/lib/oe/patch.py
> > > +++ b/meta/lib/oe/patch.py
> > > @@ -794,68 +794,88 @@ class UserResolver(Resolver):
> > >               raise
> > >           os.chdir(olddir)
> > > 
> > > -
> > > -def patch_path(url, fetch, workdir, expand=True):
> > > -    """Return the local path of a patch, or return nothing if this isn't a patch"""
> > > -
> > > -    local = fetch.localpath(url)
> > > -    if os.path.isdir(local):
> > > -        return
> > > +def is_patch(local, workdir, apply_all, expand):
> > >       base, ext = os.path.splitext(os.path.basename(local))
> > >       if ext in ('.gz', '.bz2', '.xz', '.Z'):
> > >           if expand:
> > >               local = os.path.join(workdir, base)
> > >           ext = os.path.splitext(base)[1]
> > > 
> > > -    urldata = fetch.ud[url]
> > > +    if ext in (".diff", ".patch") or apply_all:
> > > +        return True
> > > +    return False
> > > +
> > > +def patch_path(local, urldata, fetch, workdir, expand=True):
> > > +    """Return a list of local paths of patches or return an empty list if there are no
> > > patches"""
> > > +    patches = []
> > > +
> > > +    apply_all = False
> > >       if "apply" in urldata.parm:
> > >           apply = oe.types.boolean(urldata.parm["apply"])
> > >           if not apply:
> > > -            return
> > > -    elif ext not in (".diff", ".patch"):
> > > -        return
> > > +            return patches
> > > +        else:
> > > +            apply_all = True
> > > +
> > > +    if os.path.isdir(local) and apply_all:
> > > +        for f in sorted(os.listdir(local)):
> > 
> > This assumes that patches can be ordered (in here alphabetically) -
> > which isn't the case for most of the projects I worked with - I think it
> > was also a reason why one has to specify the order and inclusion in SRC_URI.
> 
> This was immediately my concern as well when I read the 0/N for the series.
> 
> I'd be more inclined to remove the documentation, rather than trying
> to deal with sorting and sequencing issues. Since it hasn't been
> working until now, there wouldn't be any users to annoy with dropped
> functionality.
> 
> (That being said, we could make the argument that anyone attempting to
> just point at a directory full of patches knows enough that they need
> to have them named in a way to keep them sorted and applied in order).
> 

This now implemented feature is not meant to replace the current way
of specifying patches. So if you cannot change the patch filenames
to specify the order then don't use it.

One use case I envision is a patchset in flight while upstreaming to
the upstream project. Apply such a patchset as the content of a
directory. For that use case the patches are sorted with their
000x-*.patch naming from generating them by default.
If you want to mix apples and orange patches, then I agree that the
directory approach is probably the wrong one.

> Cheers,
> 
> Bruce
> 
> 
> > > +            sublocal = local + '/' + f
> > 
> > Wouldn't be a sorted glob be more suitable here?

I'll look into it.

Max

> > 
> > > +            # Also search in subdirs
> > > +            if os.path.isdir(sublocal):
> > > +                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
> > > +            else:
> > > +                if is_patch(sublocal, workdir, apply_all, expand):
> > > +                    patches.append(sublocal)
> > > +    else:
> > > +        if is_patch(local, workdir, apply_all, expand):
> > > +            patches.append(local)
> > > 
> > > -    return local
> > > +    return patches
> > > 
> > >   def src_patches(d, all=False, expand=True):
> > > +    """Return a list of local paths from SRC_URI. With all=False all patches targeting
> > > do_patch, with all=True all other local paths"""
> > >       workdir = d.getVar('WORKDIR')
> > >       fetch = bb.fetch2.Fetch([], d)
> > >       patches = []
> > >       sources = []
> > > -    for url in fetch.urls:
> > > -        local = patch_path(url, fetch, workdir, expand)
> > > -        if not local:
> > > -            if all:
> > > -                local = fetch.localpath(url)
> > > -                sources.append(local)
> > > -            continue
> > > 
> > > +    for url in fetch.urls:
> > > +        local = fetch.localpath(url)
> > >           urldata = fetch.ud[url]
> > > -        parm = urldata.parm
> > > -        patchname = parm.get('pname') or os.path.basename(local)
> > > +        locals = []
> > > +        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
> > > 
> > > -        apply, reason = should_apply(parm, d)
> > > -        if not apply:
> > > -            if reason:
> > > -                bb.note("Patch %s %s" % (patchname, reason))
> > > +        if not locals:
> > > +            if all:
> > > +                sources.append(local)
> > >               continue
> > > -
> > > -        patchparm = {'patchname': patchname}
> > > -        if "striplevel" in parm:
> > > -            striplevel = parm["striplevel"]
> > > -        elif "pnum" in parm:
> > > -            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use
> > > 'striplevel'" % url)
> > > -            striplevel = parm["pnum"]
> > >           else:
> > > -            striplevel = '1'
> > > -        patchparm['striplevel'] = striplevel
> > > +            for patch in locals:
> > > +                parm = urldata.parm
> > > +                patchname = parm.get('pname') or os.path.basename(patch)
> > > +
> > > +                apply, reason = should_apply(parm, d)
> > > +                if not apply:
> > > +                    if reason:
> > > +                        bb.note("Patch %s %s" % (patchname, reason))
> > > +                    continue
> > > +
> > > +                patchparm = {'patchname': patchname}
> > > +                if "striplevel" in parm:
> > > +                    striplevel = parm["striplevel"]
> > > +                elif "pnum" in parm:
> > > +                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use
> > > 'striplevel'" % url)
> > > +                    striplevel = parm["pnum"]
> > > +                else:
> > > +                    striplevel = '1'
> > > +                patchparm['striplevel'] = striplevel
> > > 
> > > -        patchdir = parm.get('patchdir')
> > > -        if patchdir:
> > > -            patchparm['patchdir'] = patchdir
> > > +                patchdir = parm.get('patchdir')
> > > +                if patchdir:
> > > +                    patchparm['patchdir'] = patchdir
> > > 
> > > -        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
> > > -        patches.append(localurl)
> > > +                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
> > > +                patches.append(localurl)
> > > 
> > >       if all:
> > >           return sources
> > > 
> > > 
> > > 
> > > 
> > > 
> > 
> > -=-=-=-=-=-=-=-=-=-=-=-
> > Links: You receive all messages sent to this group.
> > View/Reply Online (#159521): https://lists.openembedded.org/g/openembedded-core/message/159521
> > Mute This Topic: https://lists.openembedded.org/mt/87635233/1050810
> > Group Owner: openembedded-core+owner@lists.openembedded.org
> > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
> > -=-=-=-=-=-=-=-=-=-=-=-
> > 
> 
> --
> - Thou shalt not follow the NULL pointer, for chaos and madness await
> thee at its end
> - "Use the force Harry" - Gandalf, Star Trek II
Bruce Ashfield Dec. 10, 2021, 2:13 p.m. UTC | #4 
On Fri, Dec 10, 2021 at 9:07 AM Max <max.oss.09@gmail.com> wrote:
>
> Am Freitag, den 10.12.2021, 09:01 -0500 schrieb Bruce Ashfield:
> > On Fri, Dec 10, 2021 at 8:29 AM Konrad Weihmann <kweihmann@outlook.com> wrote:
> > >
> > >
> > > On 10.12.21 14:04, Max Krummenacher wrote:
> > > > The current developer manual specifies that patches that are
> > > > part of a directory which is given in SRC_URI are applied by
> > > > the do_patch task. However that is not implemented in the
> > > > current code.
> > > >
> > > > Implement part of it with two differences:
> > > > - The implementation requires the parameter "apply=yes" and
> > > >    adds all files as patches, not only files ending in .patch
> > > >    and .diff.
> > > >    This keeps recipes which depend on the current implementation
> > > >    working.
> > > > - The possibility to exclude a file in that directory from being
> > > >    applied by a follow up entry with "apply=no" is dropped.
> > > >
> > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > ---
> > > >   meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
> > > >   1 file changed, 59 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> > > > index 950fe723dc..0d2afab2f9 100644
> > > > --- a/meta/lib/oe/patch.py
> > > > +++ b/meta/lib/oe/patch.py
> > > > @@ -794,68 +794,88 @@ class UserResolver(Resolver):
> > > >               raise
> > > >           os.chdir(olddir)
> > > >
> > > > -
> > > > -def patch_path(url, fetch, workdir, expand=True):
> > > > -    """Return the local path of a patch, or return nothing if this isn't a patch"""
> > > > -
> > > > -    local = fetch.localpath(url)
> > > > -    if os.path.isdir(local):
> > > > -        return
> > > > +def is_patch(local, workdir, apply_all, expand):
> > > >       base, ext = os.path.splitext(os.path.basename(local))
> > > >       if ext in ('.gz', '.bz2', '.xz', '.Z'):
> > > >           if expand:
> > > >               local = os.path.join(workdir, base)
> > > >           ext = os.path.splitext(base)[1]
> > > >
> > > > -    urldata = fetch.ud[url]
> > > > +    if ext in (".diff", ".patch") or apply_all:
> > > > +        return True
> > > > +    return False
> > > > +
> > > > +def patch_path(local, urldata, fetch, workdir, expand=True):
> > > > +    """Return a list of local paths of patches or return an empty list if there are no
> > > > patches"""
> > > > +    patches = []
> > > > +
> > > > +    apply_all = False
> > > >       if "apply" in urldata.parm:
> > > >           apply = oe.types.boolean(urldata.parm["apply"])
> > > >           if not apply:
> > > > -            return
> > > > -    elif ext not in (".diff", ".patch"):
> > > > -        return
> > > > +            return patches
> > > > +        else:
> > > > +            apply_all = True
> > > > +
> > > > +    if os.path.isdir(local) and apply_all:
> > > > +        for f in sorted(os.listdir(local)):
> > >
> > > This assumes that patches can be ordered (in here alphabetically) -
> > > which isn't the case for most of the projects I worked with - I think it
> > > was also a reason why one has to specify the order and inclusion in SRC_URI.
> >
> > This was immediately my concern as well when I read the 0/N for the series.
> >
> > I'd be more inclined to remove the documentation, rather than trying
> > to deal with sorting and sequencing issues. Since it hasn't been
> > working until now, there wouldn't be any users to annoy with dropped
> > functionality.
> >
> > (That being said, we could make the argument that anyone attempting to
> > just point at a directory full of patches knows enough that they need
> > to have them named in a way to keep them sorted and applied in order).
> >
>
> This now implemented feature is not meant to replace the current way
> of specifying patches. So if you cannot change the patch filenames
> to specify the order then don't use it.

Well yes, that is obvious.  I can't imagine why anyone would want to
even use potentially unordered directories of patches, so making it
a replacement would be unwise :D

>
> One use case I envision is a patchset in flight while upstreaming to
> the upstream project. Apply such a patchset as the content of a
> directory. For that use case the patches are sorted with their
> 000x-*.patch naming from generating them by default.
> If you want to mix apples and orange patches, then I agree that the
> directory approach is probably the wrong one.

Agreed. If someone is dumping patches from git, they get numerical
ordering. Practice shows that even nicely ordered patches start to
get a strange ordering as more are added, some are removed (and
people forget to specify start-number when dumping the new patches).

I'm sure someone might use the functionality, and it is true that it has
no impact on the main workflows, so there's no risk there. It is just
more code to support (and we should probably have a test added for
it) .. and Richard probably knows better than me the risks and if it
will actually be used.

Cheers,

Bruce

>
> > Cheers,
> >
> > Bruce
> >
> >
> > > > +            sublocal = local + '/' + f
> > >
> > > Wouldn't be a sorted glob be more suitable here?
>
> I'll look into it.
>
> Max
>
> > >
> > > > +            # Also search in subdirs
> > > > +            if os.path.isdir(sublocal):
> > > > +                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
> > > > +            else:
> > > > +                if is_patch(sublocal, workdir, apply_all, expand):
> > > > +                    patches.append(sublocal)
> > > > +    else:
> > > > +        if is_patch(local, workdir, apply_all, expand):
> > > > +            patches.append(local)
> > > >
> > > > -    return local
> > > > +    return patches
> > > >
> > > >   def src_patches(d, all=False, expand=True):
> > > > +    """Return a list of local paths from SRC_URI. With all=False all patches targeting
> > > > do_patch, with all=True all other local paths"""
> > > >       workdir = d.getVar('WORKDIR')
> > > >       fetch = bb.fetch2.Fetch([], d)
> > > >       patches = []
> > > >       sources = []
> > > > -    for url in fetch.urls:
> > > > -        local = patch_path(url, fetch, workdir, expand)
> > > > -        if not local:
> > > > -            if all:
> > > > -                local = fetch.localpath(url)
> > > > -                sources.append(local)
> > > > -            continue
> > > >
> > > > +    for url in fetch.urls:
> > > > +        local = fetch.localpath(url)
> > > >           urldata = fetch.ud[url]
> > > > -        parm = urldata.parm
> > > > -        patchname = parm.get('pname') or os.path.basename(local)
> > > > +        locals = []
> > > > +        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
> > > >
> > > > -        apply, reason = should_apply(parm, d)
> > > > -        if not apply:
> > > > -            if reason:
> > > > -                bb.note("Patch %s %s" % (patchname, reason))
> > > > +        if not locals:
> > > > +            if all:
> > > > +                sources.append(local)
> > > >               continue
> > > > -
> > > > -        patchparm = {'patchname': patchname}
> > > > -        if "striplevel" in parm:
> > > > -            striplevel = parm["striplevel"]
> > > > -        elif "pnum" in parm:
> > > > -            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use
> > > > 'striplevel'" % url)
> > > > -            striplevel = parm["pnum"]
> > > >           else:
> > > > -            striplevel = '1'
> > > > -        patchparm['striplevel'] = striplevel
> > > > +            for patch in locals:
> > > > +                parm = urldata.parm
> > > > +                patchname = parm.get('pname') or os.path.basename(patch)
> > > > +
> > > > +                apply, reason = should_apply(parm, d)
> > > > +                if not apply:
> > > > +                    if reason:
> > > > +                        bb.note("Patch %s %s" % (patchname, reason))
> > > > +                    continue
> > > > +
> > > > +                patchparm = {'patchname': patchname}
> > > > +                if "striplevel" in parm:
> > > > +                    striplevel = parm["striplevel"]
> > > > +                elif "pnum" in parm:
> > > > +                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use
> > > > 'striplevel'" % url)
> > > > +                    striplevel = parm["pnum"]
> > > > +                else:
> > > > +                    striplevel = '1'
> > > > +                patchparm['striplevel'] = striplevel
> > > >
> > > > -        patchdir = parm.get('patchdir')
> > > > -        if patchdir:
> > > > -            patchparm['patchdir'] = patchdir
> > > > +                patchdir = parm.get('patchdir')
> > > > +                if patchdir:
> > > > +                    patchparm['patchdir'] = patchdir
> > > >
> > > > -        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
> > > > -        patches.append(localurl)
> > > > +                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
> > > > +                patches.append(localurl)
> > > >
> > > >       if all:
> > > >           return sources
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > > -=-=-=-=-=-=-=-=-=-=-=-
> > > Links: You receive all messages sent to this group.
> > > View/Reply Online (#159521): https://lists.openembedded.org/g/openembedded-core/message/159521
> > > Mute This Topic: https://lists.openembedded.org/mt/87635233/1050810
> > > Group Owner: openembedded-core+owner@lists.openembedded.org
> > > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
> > > -=-=-=-=-=-=-=-=-=-=-=-
> > >
> >
> > --
> > - Thou shalt not follow the NULL pointer, for chaos and madness await
> > thee at its end
> > - "Use the force Harry" - Gandalf, Star Trek II
>
Quentin Schulz Dec. 10, 2021, 2:16 p.m. UTC | #5 
On Fri, Dec 10, 2021 at 09:01:18AM -0500, Bruce Ashfield wrote:
> On Fri, Dec 10, 2021 at 8:29 AM Konrad Weihmann <kweihmann@outlook.com> wrote:
> >
> >
> >
> > On 10.12.21 14:04, Max Krummenacher wrote:
> > > The current developer manual specifies that patches that are
> > > part of a directory which is given in SRC_URI are applied by
> > > the do_patch task. However that is not implemented in the
> > > current code.
> > >
> > > Implement part of it with two differences:
> > > - The implementation requires the parameter "apply=yes" and
> > >    adds all files as patches, not only files ending in .patch
> > >    and .diff.
> > >    This keeps recipes which depend on the current implementation
> > >    working.
> > > - The possibility to exclude a file in that directory from being
> > >    applied by a follow up entry with "apply=no" is dropped.
> > >
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > ---
> > >   meta/lib/oe/patch.py | 98 ++++++++++++++++++++++++++------------------
> > >   1 file changed, 59 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> > > index 950fe723dc..0d2afab2f9 100644
> > > --- a/meta/lib/oe/patch.py
> > > +++ b/meta/lib/oe/patch.py
> > > @@ -794,68 +794,88 @@ class UserResolver(Resolver):
> > >               raise
> > >           os.chdir(olddir)
> > >
> > > -
> > > -def patch_path(url, fetch, workdir, expand=True):
> > > -    """Return the local path of a patch, or return nothing if this isn't a patch"""
> > > -
> > > -    local = fetch.localpath(url)
> > > -    if os.path.isdir(local):
> > > -        return
> > > +def is_patch(local, workdir, apply_all, expand):
> > >       base, ext = os.path.splitext(os.path.basename(local))
> > >       if ext in ('.gz', '.bz2', '.xz', '.Z'):
> > >           if expand:
> > >               local = os.path.join(workdir, base)
> > >           ext = os.path.splitext(base)[1]
> > >
> > > -    urldata = fetch.ud[url]
> > > +    if ext in (".diff", ".patch") or apply_all:
> > > +        return True
> > > +    return False
> > > +
> > > +def patch_path(local, urldata, fetch, workdir, expand=True):
> > > +    """Return a list of local paths of patches or return an empty list if there are no patches"""
> > > +    patches = []
> > > +
> > > +    apply_all = False
> > >       if "apply" in urldata.parm:
> > >           apply = oe.types.boolean(urldata.parm["apply"])
> > >           if not apply:
> > > -            return
> > > -    elif ext not in (".diff", ".patch"):
> > > -        return
> > > +            return patches
> > > +        else:
> > > +            apply_all = True
> > > +
> > > +    if os.path.isdir(local) and apply_all:
> > > +        for f in sorted(os.listdir(local)):
> >
> > This assumes that patches can be ordered (in here alphabetically) -
> > which isn't the case for most of the projects I worked with - I think it
> > was also a reason why one has to specify the order and inclusion in SRC_URI.
> 
> This was immediately my concern as well when I read the 0/N for the series.
> 
> I'd be more inclined to remove the documentation, rather than trying
> to deal with sorting and sequencing issues. Since it hasn't been
> working until now, there wouldn't be any users to annoy with dropped
> functionality.
> 
> (That being said, we could make the argument that anyone attempting to
> just point at a directory full of patches knows enough that they need
> to have them named in a way to keep them sorted and applied in order).
> 

First, I'd very much welcome a patch for the documentation since I
assume if this gets merged it does not get backported, so currently
maintained but not master branches would have the old behavior,
warranting a fix in the documentation.

I would assume that this can break kernel-yocto and the KERNEL_FEATURES
with optional patching if .scc and .patch files are in the same
directory which is added to SRC_URI with apply=True.

On a slightly different topic, I personally think that including directories
in SRC_URI should be discouraged because overriding a directory from a
bbappend is completely replacing it and not doing a mergeof both
directories.

i.e.

some-dir/a
some-dir/b
some-dir/c

bbappend/some-dir/b

The result is the only file fetched by the recipe is b from the
bbappend. So it makes it much harder to override only one file from
within a directory which is included in SRC_URI.

Cheers,
Quentin

> Cheers,
> 
> Bruce
> 
> 
> >
> > > +            sublocal = local + '/' + f
> >
> > Wouldn't be a sorted glob be more suitable here?
> >
> > > +            # Also search in subdirs
> > > +            if os.path.isdir(sublocal):
> > > +                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
> > > +            else:
> > > +                if is_patch(sublocal, workdir, apply_all, expand):
> > > +                    patches.append(sublocal)
> > > +    else:
> > > +        if is_patch(local, workdir, apply_all, expand):
> > > +            patches.append(local)
> > >
> > > -    return local
> > > +    return patches
> > >
> > >   def src_patches(d, all=False, expand=True):
> > > +    """Return a list of local paths from SRC_URI. With all=False all patches targeting do_patch, with all=True all other local paths"""
> > >       workdir = d.getVar('WORKDIR')
> > >       fetch = bb.fetch2.Fetch([], d)
> > >       patches = []
> > >       sources = []
> > > -    for url in fetch.urls:
> > > -        local = patch_path(url, fetch, workdir, expand)
> > > -        if not local:
> > > -            if all:
> > > -                local = fetch.localpath(url)
> > > -                sources.append(local)
> > > -            continue
> > >
> > > +    for url in fetch.urls:
> > > +        local = fetch.localpath(url)
> > >           urldata = fetch.ud[url]
> > > -        parm = urldata.parm
> > > -        patchname = parm.get('pname') or os.path.basename(local)
> > > +        locals = []
> > > +        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
> > >
> > > -        apply, reason = should_apply(parm, d)
> > > -        if not apply:
> > > -            if reason:
> > > -                bb.note("Patch %s %s" % (patchname, reason))
> > > +        if not locals:
> > > +            if all:
> > > +                sources.append(local)
> > >               continue
> > > -
> > > -        patchparm = {'patchname': patchname}
> > > -        if "striplevel" in parm:
> > > -            striplevel = parm["striplevel"]
> > > -        elif "pnum" in parm:
> > > -            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> > > -            striplevel = parm["pnum"]
> > >           else:
> > > -            striplevel = '1'
> > > -        patchparm['striplevel'] = striplevel
> > > +            for patch in locals:
> > > +                parm = urldata.parm
> > > +                patchname = parm.get('pname') or os.path.basename(patch)
> > > +
> > > +                apply, reason = should_apply(parm, d)
> > > +                if not apply:
> > > +                    if reason:
> > > +                        bb.note("Patch %s %s" % (patchname, reason))
> > > +                    continue
> > > +
> > > +                patchparm = {'patchname': patchname}
> > > +                if "striplevel" in parm:
> > > +                    striplevel = parm["striplevel"]
> > > +                elif "pnum" in parm:
> > > +                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
> > > +                    striplevel = parm["pnum"]
> > > +                else:
> > > +                    striplevel = '1'
> > > +                patchparm['striplevel'] = striplevel
> > >
> > > -        patchdir = parm.get('patchdir')
> > > -        if patchdir:
> > > -            patchparm['patchdir'] = patchdir
> > > +                patchdir = parm.get('patchdir')
> > > +                if patchdir:
> > > +                    patchparm['patchdir'] = patchdir
> > >
> > > -        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
> > > -        patches.append(localurl)
> > > +                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
> > > +                patches.append(localurl)
> > >
> > >       if all:
> > >           return sources
> > >
> > >
> > >
> > >
> > >
> >
> > 
> >
> 
> 
> --
> - Thou shalt not follow the NULL pointer, for chaos and madness await
> thee at its end
> - "Use the force Harry" - Gandalf, Star Trek II

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#159522): https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.openembedded.org_g_openembedded-2Dcore_message_159522&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=X60O6Qnokj0DyoS8ZB3VcYfT4vD3HbyQCdfUDE8LsXY7Bct-woOv9I4WNMtNTrdK&s=22Rj9T2H_RxxHXYOcLmONzO9ywnmrSq0Z2_ED40wL4k&e= 
> Mute This Topic: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.openembedded.org_mt_87635233_6293953&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=X60O6Qnokj0DyoS8ZB3VcYfT4vD3HbyQCdfUDE8LsXY7Bct-woOv9I4WNMtNTrdK&s=VjlEmd1KMBU7WBGP0UoE1Pq3EEEUMHTd0Tm349XATcI&e= 
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.openembedded.org_g_openembedded-2Dcore_unsub&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=X60O6Qnokj0DyoS8ZB3VcYfT4vD3HbyQCdfUDE8LsXY7Bct-woOv9I4WNMtNTrdK&s=vL7bnRc7bvHFyVCAj_sB4Gafs1RczwSyyizYEfAcX18&e=  [quentin.schulz@theobroma-systems.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
diff mbox series

Patch

diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 950fe723dc..0d2afab2f9 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -794,68 +794,88 @@  class UserResolver(Resolver):
             raise
         os.chdir(olddir)
 
-
-def patch_path(url, fetch, workdir, expand=True):
-    """Return the local path of a patch, or return nothing if this isn't a patch"""
-
-    local = fetch.localpath(url)
-    if os.path.isdir(local):
-        return
+def is_patch(local, workdir, apply_all, expand):
     base, ext = os.path.splitext(os.path.basename(local))
     if ext in ('.gz', '.bz2', '.xz', '.Z'):
         if expand:
             local = os.path.join(workdir, base)
         ext = os.path.splitext(base)[1]
 
-    urldata = fetch.ud[url]
+    if ext in (".diff", ".patch") or apply_all:
+        return True
+    return False
+
+def patch_path(local, urldata, fetch, workdir, expand=True):
+    """Return a list of local paths of patches or return an empty list if there are no patches"""
+    patches = []
+
+    apply_all = False
     if "apply" in urldata.parm:
         apply = oe.types.boolean(urldata.parm["apply"])
         if not apply:
-            return
-    elif ext not in (".diff", ".patch"):
-        return
+            return patches
+        else:
+            apply_all = True
+
+    if os.path.isdir(local) and apply_all:
+        for f in sorted(os.listdir(local)):
+            sublocal = local + '/' + f
+            # Also search in subdirs
+            if os.path.isdir(sublocal):
+                patches = patches + patch_path(sublocal, urldata, fetch, workdir, expand)
+            else:
+                if is_patch(sublocal, workdir, apply_all, expand):
+                    patches.append(sublocal)
+    else:
+        if is_patch(local, workdir, apply_all, expand):
+            patches.append(local)
 
-    return local
+    return patches
 
 def src_patches(d, all=False, expand=True):
+    """Return a list of local paths from SRC_URI. With all=False all patches targeting do_patch, with all=True all other local paths"""
     workdir = d.getVar('WORKDIR')
     fetch = bb.fetch2.Fetch([], d)
     patches = []
     sources = []
-    for url in fetch.urls:
-        local = patch_path(url, fetch, workdir, expand)
-        if not local:
-            if all:
-                local = fetch.localpath(url)
-                sources.append(local)
-            continue
 
+    for url in fetch.urls:
+        local = fetch.localpath(url)
         urldata = fetch.ud[url]
-        parm = urldata.parm
-        patchname = parm.get('pname') or os.path.basename(local)
+        locals = []
+        locals = locals + patch_path(local, urldata, fetch, workdir, expand)
 
-        apply, reason = should_apply(parm, d)
-        if not apply:
-            if reason:
-                bb.note("Patch %s %s" % (patchname, reason))
+        if not locals:
+            if all:
+                sources.append(local)
             continue
-
-        patchparm = {'patchname': patchname}
-        if "striplevel" in parm:
-            striplevel = parm["striplevel"]
-        elif "pnum" in parm:
-            #bb.msg.warn(None, "Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
-            striplevel = parm["pnum"]
         else:
-            striplevel = '1'
-        patchparm['striplevel'] = striplevel
+            for patch in locals:
+                parm = urldata.parm
+                patchname = parm.get('pname') or os.path.basename(patch)
+
+                apply, reason = should_apply(parm, d)
+                if not apply:
+                    if reason:
+                        bb.note("Patch %s %s" % (patchname, reason))
+                    continue
+
+                patchparm = {'patchname': patchname}
+                if "striplevel" in parm:
+                    striplevel = parm["striplevel"]
+                elif "pnum" in parm:
+                    #bb.warn("Deprecated usage of 'pnum' url parameter in '%s', please use 'striplevel'" % url)
+                    striplevel = parm["pnum"]
+                else:
+                    striplevel = '1'
+                patchparm['striplevel'] = striplevel
 
-        patchdir = parm.get('patchdir')
-        if patchdir:
-            patchparm['patchdir'] = patchdir
+                patchdir = parm.get('patchdir')
+                if patchdir:
+                    patchparm['patchdir'] = patchdir
 
-        localurl = bb.fetch.encodeurl(('file', '', local, '', '', patchparm))
-        patches.append(localurl)
+                localurl = bb.fetch.encodeurl(('file', '', patch, '', '', patchparm))
+                patches.append(localurl)
 
     if all:
         return sources