[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Nicolas S. Dade]

Signed-off-by: Nicolas S. Dade <nic.dade@mistsys.com>
---
 package/pkg-generic.mk | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f77aab2..09f2405 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -555,15 +555,18 @@ else
 # In the package override case, the sequence of steps
 #  source, by rsyncing
 #  depends
+#  patch
 #  configure
 
 # Use an order-only dependency so the "<pkg>-clean-for-rebuild" rule
 # can remove the stamp file without triggering the configure step.
-$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_RSYNC)
+$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_PATCH)
 
 $(1)-depends:		$$($(2)_FINAL_DEPENDENCIES)
 
-$(1)-patch:		$(1)-rsync
+$(1)-patch:		$$($(2)_TARGET_PATCH)
+$$($(2)_TARGET_PATCH):	$$($(2)_TARGET_RSYNC)
+
 $(1)-extract:		$(1)-rsync
 
 $(1)-rsync:		$$($(2)_TARGET_RSYNC)
-- 
1.9.1

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Yann E. MORIN]

Nicolas, All,

On 2015-04-08 17:28 -0700, Nicolas S. Dade spake thusly:
> Signed-off-by: Nicolas S. Dade <nic.dade@mistsys.com>

Thanks for your patch!

However, we're explicitly not allowing patching local sources, on
purpose.

The use-case for using local sources is to allow people to use a
development version of the package, one they are hacking on. Thus, we
believe that in most cases, the patches bundled with Buildroot would
most certainly fail to apply on local sources.

So, we think that, if those patches are still needed, the developper
will have to carry them in its local sources.

Also, your commit log is not explaining the issue you are facing, so it
is hard to udnerstand why you would need bundled patches to be applied,
and the use-case that would cover.

I'm leaving this open for now, for others to speak up, but I'd be
tempted to say "no" for the aforementioned reasons.

Regards,
Yann E. MORIN.

> ---
>  package/pkg-generic.mk | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f77aab2..09f2405 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -555,15 +555,18 @@ else
>  # In the package override case, the sequence of steps
>  #  source, by rsyncing
>  #  depends
> +#  patch
>  #  configure
>  
>  # Use an order-only dependency so the "<pkg>-clean-for-rebuild" rule
>  # can remove the stamp file without triggering the configure step.
> -$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_RSYNC)
> +$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_PATCH)
>  
>  $(1)-depends:		$$($(2)_FINAL_DEPENDENCIES)
>  
> -$(1)-patch:		$(1)-rsync
> +$(1)-patch:		$$($(2)_TARGET_PATCH)
> +$$($(2)_TARGET_PATCH):	$$($(2)_TARGET_RSYNC)
> +
>  $(1)-extract:		$(1)-rsync
>  
>  $(1)-rsync:		$$($(2)_TARGET_RSYNC)
> -- 
> 1.9.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Nicolas Dade]

Let me explain what led me to allow patches to local source. My local
source isn't mine. It's an SDK from the vendor of a part I'm using. The
vendor is much better at hardware than software, and their SDK doesn't
build as-is. So I need to patch it. I also need to maintain my patches
separated from the SDK since the vendor publishes updates to their SDK
every month or so.

So what I am doing is having the vendor's SDK source code unzip'ed in one
directory, and have buildroot use [subtrees of] that directory as the local
source, patch it with my fixes, and build it.

PS I also thought that not allowing patching only for local sources was a
strange asymmetry in buildroot. I expected local sources to work the same
as tarballs. It wasn't until I looked into the buildroot make system that I
realized why my patches weren't getting applied. So if you don't accept
this patch, at least you might document that local sources cannot be
patched in a clear way.



-- 
Nicolas Dade        nic.dade at mistsys.com        831-464-6843

On Thu, Apr 9, 2015 at 3:12 PM, Yann E. MORIN <yann.morin.1998@free.fr>
wrote:

> Nicolas, All,
>
> On 2015-04-08 17:28 -0700, Nicolas S. Dade spake thusly:
> > Signed-off-by: Nicolas S. Dade <nic.dade@mistsys.com>
>
> Thanks for your patch!
>
> However, we're explicitly not allowing patching local sources, on
> purpose.
>
> The use-case for using local sources is to allow people to use a
> development version of the package, one they are hacking on. Thus, we
> believe that in most cases, the patches bundled with Buildroot would
> most certainly fail to apply on local sources.
>
> So, we think that, if those patches are still needed, the developper
> will have to carry them in its local sources.
>
> Also, your commit log is not explaining the issue you are facing, so it
> is hard to udnerstand why you would need bundled patches to be applied,
> and the use-case that would cover.
>
> I'm leaving this open for now, for others to speak up, but I'd be
> tempted to say "no" for the aforementioned reasons.
>
> Regards,
> Yann E. MORIN.
>
> > ---
> >  package/pkg-generic.mk | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index f77aab2..09f2405 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -555,15 +555,18 @@ else
> >  # In the package override case, the sequence of steps
> >  #  source, by rsyncing
> >  #  depends
> > +#  patch
> >  #  configure
> >
> >  # Use an order-only dependency so the "<pkg>-clean-for-rebuild" rule
> >  # can remove the stamp file without triggering the configure step.
> > -$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_RSYNC)
> > +$$($(2)_TARGET_CONFIGURE): | $$($(2)_TARGET_PATCH)
> >
> >  $(1)-depends:                $$($(2)_FINAL_DEPENDENCIES)
> >
> > -$(1)-patch:          $(1)-rsync
> > +$(1)-patch:          $$($(2)_TARGET_PATCH)
> > +$$($(2)_TARGET_PATCH):       $$($(2)_TARGET_RSYNC)
> > +
> >  $(1)-extract:                $(1)-rsync
> >
> >  $(1)-rsync:          $$($(2)_TARGET_RSYNC)
> > --
> > 1.9.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
>
> --
>
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
> conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___
>      |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There
> is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v
>  conspiracy.  |
>
> '------------------------------^-------^------------------^--------------------'
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20150409/d6058003/attachment.html>

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Yann E. MORIN]

Nicolas, All,

On 2015-04-09 15:21 -0700, Nicolas Dade spake thusly:
> Let me explain what led me to allow patches to local source. My local
> source isn't mine. It's an SDK from the vendor of a part I'm using. The
> vendor is much better at hardware than software, and their SDK doesn't
> build as-is. So I need to patch it. I also need to maintain my patches
> separated from the SDK since the vendor publishes updates to their SDK
> every month or so.
> 
> So what I am doing is having the vendor's SDK source code unzip'ed in one
> directory, and have buildroot use [subtrees of] that directory as the local
> source, patch it with my fixes, and build it.

Ah, OK, I see. (To be honest, I was expecting something like that!)

Well, there's not much we can do with that...

On the one hand, we maintain the current behaviour, and your use-case is
broken. On the other hand, we solve a use-case like yours, but break
others where the sources are too different for our bundled patches to be
applied.

Needless to say we don't like breaking stuff...

Now, let us think about that...

Still, there's something I am wondering about. Why do you point those
packages to the vendor's SDK in the first place? Why don't you use the
ones referenced by Buildroot (e.g. from upstream)?

I can understand you'd want a few select packages from the SDK, mostly
those with HW dependencies, but the rest?

> PS I also thought that not allowing patching only for local sources was a
> strange asymmetry in buildroot. I expected local sources to work the same
> as tarballs. It wasn't until I looked into the buildroot make system that I
> realized why my patches weren't getting applied. So if you don't accept
> this patch, at least you might document that local sources cannot be
> patched in a clear way.

Well, I would think that's pretty well explained in the manual:
    http://buildroot.net/downloads/manual/manual.html#_using_buildroot_during_development

Quoting:

    When Buildroot finds that for a given package, an <pkg>_OVERRIDE_SRCDIR
    has been defined, it will no longer attempt to download, extract and
    patch the package. Instead, it will directly use the source code
    available in in the specified directory [...]

I don't know how to make that even more explicit... If you find a better
phrasing, do not hesitate! ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Yann E. MORIN]

Nicolas, All,

On 2015-04-09 17:35 -0700, Nicolas Dade spake thusly:
[--SNIP--]
> > why not used the [packages in buildroot]
> 
> I do for those which are in buildroot. What I'm building in the SDK are the
> vendor's utilities which talk to their hardware part. Those aren't packages
> in [regular] buildroot, but I've added packages to my buildroot to build
> these vendor utilities.

OK, I misread what you wrote. The way you do it makes sense, of course.

> > [documentation]
> 
> Ah, I see why I didn't find this in the docs. I'm not using the
> <pkg>_OVERRIDE_SRCDIR feature. I'm setting <pkg>_SITE to the subdir of the
> SDK I want, and <pkt>_SITE_METHOD to "local". That is described in section
> "17.5.2 generic-package reference", and doesn't mention patches or lack
> there-of.

Aha! But inder the hood, the 'local' site method hijacks the
_OVERRIDE_SRCDIR infra. Hence the same behaviour.

> This way buildroot rsyncs from the SDK subdir I want to build to
> output/<pkg>-<ver>/, applies my patches, and builds it there, and the SDK
> source tree is left untouched. My understanding of _OVERRIDE_SRCDIR is that
> it overrides what would normally be output/<pkg>-<ver>/, and if I set it to
> point to the SDK I'd end up patching the pristine SDK source.
> 
> Maybe the right thing is to patch if _SITE and SITE_METHOD=local is used,
> and not if _OVERRIDE_SRCDIR is used.

Well, doing so is surely not the solution, because we'd be trying to
revert a decision made previously. The best solution would be to teach
the download infra to handle the 'local' site method by itself and not
rely on OVERRIDE_SRCDIR to begin with.

However, as a workaround to your issue, I'd suggest you do somwething
like the following (using two packages 'foo' and 'bar' as example):


foo.mk:
    FOO_SOURCE = filename-of-SDK.tar.gz
    FOO_SITE = file:///path/to/SDK

    define FOO_BUILD_CMDS
        cd $(@D)/relative/path/to/package/foo; \
        make whatever
    endef

    # And similar for install and other _CMDS

bar.mk:
    BAR_SOURCE = filename-of-SDK.tar.gz
    BAR_SITE = file:///path/to/SDK

    define BAR_BUILD_CMDS
        cd $(@D)/relative/path/to/package/bar; \
        make whatever-else
    endef

    # And similar for install and other _CMDS

And then you can have whatever patch you want to apply to those packages
(but the patches should be relative to the top-dir of the SDK, not the
packages' dirs).

Yes, this would mean having many packages using the same source file,
but that's pretty much OK; it works even though we're not doing it for
any package in Buildroot.

Let's continue discussing a proper solution for that. In the meantime,
I'm marking your patch as "Rejected" in our Patchwork.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Arnout Vandecappelle]

 Hi Nicolas,

 [Please don't top-post]

On 10/04/15 00:21, Nicolas Dade wrote:
> Let me explain what led me to allow patches to local source. My local source
> isn't mine. It's an SDK from the vendor of a part I'm using. The vendor is much
> better at hardware than software, and their SDK doesn't build as-is. So I need
> to patch it. I also need to maintain my patches separated from the SDK since the
> vendor publishes updates to their SDK every month or so.
> 
> So what I am doing is having the vendor's SDK source code unzip'ed in one
> directory, and have buildroot use [subtrees of] that directory as the local
> source, patch it with my fixes, and build it.

 Instead of unzipping the source code, leave it as an archive (if it is really a
zipfile instead of a tarball, you'll need to supply custom EXTRACT_CMDS). Then
the patches still are applied. And it gives you much more flexibility to put the

> 
> PS I also thought that not allowing patching only for local sources was a
> strange asymmetry in buildroot. I expected local sources to work the same as
> tarballs.

 We probably should get rid of the 'local' site method, since it is indeed
confusing and serves no purpose as far as I'm concerned.


 Regards,
 Arnout

> It wasn't until I looked into the buildroot make system that I
> realized why my patches weren't getting applied. So if you don't accept this
> patch, at least you might document that local sources cannot be patched in a
> clear way.
> 
[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 1/1] pkg-generic: support patching local source code

[Nicolas Dade]

> leave it as an archive...zip...tarball

In truth it's delivered to me as a zip file inside a VM image. I run the VM
once to scp out the zip file. But that's no problem; I can repack it as a
tarball.

I considered doing what you two both propose: treat the SDK as a regular
tarball-based package and build just the pieces I wanted. However the
pristine zip file unpacks to 2.4GB. I have the disk space, but unpacking
that much several times still seems something to avoid.

I think I'll just continue to maintain my buildroot enhancement as a local
patch to buildroot itself.

> get rid of the 'local' site method

If you do that I'll be patching it back in too, locally of course.

Thanks for the feedback.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20150410/8b2f5555/attachment.html>