Makefile: include BR2_EXTERNAL before other packages

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

BR2_EXTERNAL may contain packages that are "providers" of generic
functionalities, such as EGL/GLES/...

So, BR2_EXTERNAL must be included before our built-in packages,
otherwise the dependencies and rules for our built-in packages
are evaluated too early.

David has reported that doing so fixes his use-case, but we should
still consider this as a stop-gap until we can come up with a proper
fix.

Remove a superfluous extra empty line at the same time.

Reported-by: David Corvoysier <david.corvoysier@orange.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: David Corvoysier <david.corvoysier@orange.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > BR2_EXTERNAL may contain packages that are "providers" of generic
 > functionalities, such as EGL/GLES/...

Ehh, do we want to support that? With the way we have implemented the
virtual jpeg/egl/.. whatever packages, the virtual package needs to know
about all the packages providing that functionality, so it can add the
implementation to it's dependencies (and complain if nothing is
enabled) - So to do it from BR2_EXTERNAL those external packages have to
mess with the variables of the virtual package (E.G. JPEG_DEPENDENCIES),
which isn't really nice (and is an implementation detail).

Is this position going to be the right place to include BR2_EXTERNAL for
everyone? E.G. you cannot use it to implement custom filesystem types.

If we put the include here, then it becomes very close to the
BR2_PACKAGE_OVERRIDE_FILE functionality (which gets included just
before) - Perhaps we should not have two features doing basically the
same?

Comments anybody?

 > So, BR2_EXTERNAL must be included before our built-in packages,
 > otherwise the dependencies and rules for our built-in packages
 > are evaluated too early.

 > David has reported that doing so fixes his use-case, but we should
 > still consider this as a stop-gap until we can come up with a proper
 > fix.

 > Remove a superfluous extra empty line at the same time.

 > Reported-by: David Corvoysier <david.corvoysier@orange.com>
 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: David Corvoysier <david.corvoysier@orange.com>
 > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
 > ---
 >  Makefile | 7 ++++---
 >  1 file changed, 4 insertions(+), 3 deletions(-)

 > diff --git a/Makefile b/Makefile
 > index d52182e..22b7c99 100644
 > --- a/Makefile
 > +++ b/Makefile
 > @@ -121,7 +121,6 @@ else
 >    $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
 >  endif
 
 > -
 >  BUILD_DIR:=$(BASE_DIR)/build
 >  STAMP_DIR:=$(BASE_DIR)/stamps
 >  BINARIES_DIR:=$(BASE_DIR)/images
 > @@ -358,14 +357,16 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
 >  -include $(PACKAGE_OVERRIDE_FILE)
 >  endif
 
 > +# Include BR2_EXTERNAL before other packages, as BR2_EXTERNAL
 > +# may contain packages that are "providers" for eg. EGL/GLES/...
 > +include $(BR2_EXTERNAL)/external.mk
 > +
 >  include $(sort $(wildcard package/*/*.mk))
 
 >  include boot/common.mk
 >  include linux/linux.mk
 >  include system/system.mk
 
 > -include $(BR2_EXTERNAL)/external.mk
 > -
 >  TARGETS+=target-finalize
 
 >  ifeq ($(BR2_ENABLE_LOCALE_PURGE),y)
 > -- 
 > 1.8.1.2

 > _______________________________________________
 > buildroot mailing list
 > buildroot@busybox.net
 > http://lists.busybox.net/mailman/listinfo/buildroot

Peter, All,

On 2013-12-13 00:14 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>  > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>  > BR2_EXTERNAL may contain packages that are "providers" of generic
>  > functionalities, such as EGL/GLES/...
> 
> Ehh, do we want to support that? With the way we have implemented the
> virtual jpeg/egl/.. whatever packages, the virtual package needs to know
> about all the packages providing that functionality, so it can add the
> implementation to it's dependencies (and complain if nothing is
> enabled) - So to do it from BR2_EXTERNAL those external packages have to
> mess with the variables of the virtual package (E.G. JPEG_DEPENDENCIES),
> which isn't really nice (and is an implementation detail).

As we discussed (yesterday, or was it Sunday?) on IRC:
  - yes, this is an implementation detail
  - people using BR2_EXTERNAL are encouraged to push their packagings
    upstream to us
  - if they don't (eg. proprietary, or local packages), they are
    responsible for keeping up with our API changes, not unlike external
    kernel modules have to keep up with the kernel API changes
  - even as it is now, BR2_EXTERNAL can mess up with our built-in packages
  - yes, I think we do want to provide the ability for BR2_EXTERNAL to
    include packages that provide an implementation for a virtual
    package

After all, BR2_EXTERNAL is there for companies to put their dirty little
secrets they don't want to share with the community, or with the
recipients of their Buildroot copy.

If we prevent them from using BR2_EXTERNAL to have implementations for
virtual packages, then I think we are striking quite a blow to the
usefulness of BR2_EXTERNAL.

> Is this position going to be the right place to include BR2_EXTERNAL for
> everyone? E.G. you cannot use it to implement custom filesystem types.

That's a different reason, but this patch does not change the current
situation: BR2_EXTERNAL can't provide fs rules, since fs/common.mk is
included afterBR2_EXTERNAL, so BR2_EXTERNAL can't use the ROOTFS_TARGET
macro.

But since we haven't been advocating BR2_EXTERNAL for this purpose, and
it does not even work currently we hardly break any promise. ;-)

Note that my other patch that postpones the generation of packages rules
until after all packages have been parsed would solve that issue, if we
would extend that solution to filesystems, too.

> If we put the include here, then it becomes very close to the
> BR2_PACKAGE_OVERRIDE_FILE functionality (which gets included just
> before) - Perhaps we should not have two features doing basically the
> same?

In fact, I think we should get rid of BR2_PACKAGE_OVERRIDE_FILE
altogether, since BR2_EXTERNAL allows for the same functionality, plus
it is more versatile (again, with the rules-generation-postponing-patch).
My 2 cents... :-)

Regards,
Yann E. MORIN.