[Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target

Fri Nov 23 13:14:09 UTC 2018

Hello Yann,

Thanks a lot for your extensive review!

On Tue, 20 Nov 2018 22:36:12 +0100, Yann E. MORIN wrote:

> I'm not fond of the naming here...
> 
> In the end, can we expect to have a layout that would look like:
> 
>     output/build/busybox/host/
>     output/build/busybox/target/
>     output/build/busybox/busybox-1.2.3/     # Source tree, and
>                                             # currently, build dir
> And then we can add for OOT:
>     output/build/busybox/build-dir/

It would be nice indeed, but it's quite a radical change, as it moves
the location of the build directory of each package. I am not sure I
want to do that as part of this series, which is already complicated by
itself.

Also for OOT, your scheme doesn't yet make sense for package that have
both a target and a host variant: the goal is to share the source
directory between the host and the target variant. Starting from your
example, how would it work for libglib2 and host-libglib2 ?

	output/build/libglib2/libglib2-1.2.3/

for the libglib2 source code ? Where would be the build directories for
the target and host variants ?

I think a scheme that would maybe make more sense is:

	output/src/libglib2-1.2.3/
	output/build/libglib2/
	output/build/host-libglib2/
	output/per-package/libglib2/{target,host}/
	output/per-package/host-libglib2/{target,host}/

or something like that.

Since for now there is no consensus on what the scheme would be, I'm
keeping my proposal as-is, but I can adapt it to whatever we decide.
However, what I would like is that we find a path where this series can
remain limited to per-package directory support. It's already
complicated enough that I don't want to be drowned into reworking other
aspects at the same time.

> >+PER_PACKAGE_DIR := $(BASE_DIR)/per-package  
> 
> As I said above, I'm not too fond of that naming.... I have nothing
> better to suggest for now, except that we can change that later (enough
> bike-shedding).

You don't like the PER_PACKAGE_DIR variable name, or the "per-package"
directory name ? Or both ?

The variable name is easy to change any time. The directory name can be
changed, but it's a bit more annoying: once users are going to become
used to such a name/organization, changing it is going to make things
harder for users.

However, keep in mind that the BR2_PER_PACKAGE_DIRECTORIES option
(formerly BR2_PER_PACKAGE_FOLDERS) is still experimental, so whatever
it does can still be changed until we remove the "experimental" mark.

> > +ifeq ($(BR2_PER_PACKAGE_FOLDERS),)
> >  # Parallel execution of this Makefile is disabled because it changes
> >  # the packages building order, that can be a problem for two reasons:
> >  # - If a package has an unspecified optional dependency and that
> > @@ -245,6 +247,13 @@ endif
> >  # use the -j<jobs> option when building, e.g:
> >  #      make -j$((`getconf _NPROCESSORS_ONLN`+1))
> >  .NOTPARALLEL:
> > +endif  
> 
> I would have postponed parallel build activation to a separate commit.

Indeed, I've split that into a separate commit.

> > @@ -455,7 +464,11 @@ LZCAT := $(call qstrip,$(BR2_LZCAT))
> >  TAR_OPTIONS = $(call qstrip,$(BR2_TAR_OPTIONS)) -xf
> >  
> >  # packages compiled for the host go here
> > +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> > +HOST_DIR = $(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/host,$(call qstrip,$(BR2_HOST_DIR)))
> > +else
> >  HOST_DIR := $(call qstrip,$(BR2_HOST_DIR))  
> 
> Why do we still have HOST_DIR as immediately-defined?
> 
> Since HOST_DIR is simply defined in the per-package case, I see no
> reason to have immediately-defined in the standard case either.

As Arnout explained: this definition was already like this. In the
!BR2_PER_PACKAGE_FOLDERS case, there is no need for it to be
recursively-expanded. However, in the BR2_PER_PACKAGE_FOLDERS case it
*must* be recursively expanded.

> Please, sort PACKAGES, so that:
> 
>   - the copy is always done in the same order (sort always sort in the
>     ascii order);
> 
>   - duplicates get removed.

Done.

> > +	@$(call MESSAGE,"Creating global target directory")
> > +	$(foreach pkg,$(PACKAGES),\  
> 
> Ditto, sort PACKAGES.

Done.

> >  # staging and target directories do NOT list these as
> >  # dependencies anywhere else
> > -$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
> > +$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):  
> 
> Can we now split this long line, please? ;-)

True, done.

> >  ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
> >  TARGETS_ROOTFS += rootfs-$(1)
> > -PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))
> > +PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES)) $(ROOTFS_COMMON_DEPENDENCIES)  
> 
> If I am not mistaken, this is now in master:
>     https://git.buildroot.org/buildroot/commit/?id=305e4487e5c18ed89bf2aa106b2068f9dce686fb
> 
> But that's missing from next, indeed.

Correct, I've dropped this change from my commit, and simply added the
existing commit to my branch in the mean time.

> > +# $1: deps
> > +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> > +define prepare-per-package-folder  
> 
> The location where you define this macro, in the instrumentation hooks,
> is not optimum I think. You even did not provide a help-comment for it.

I moved it to pkg-utils.mk, which is a better place since another
commit in my series uses this macro from pkg-kconfig.mk. I have also
added a help comment to it.

> 
> > +	mkdir -p $(HOST_DIR) $(TARGET_DIR)
> > +	$(foreach pkg,$(1),\
> > +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> > +		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> > +		$(HOST_DIR)$(sep))
> > +	$(foreach pkg,$(1),\
> > +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> > +		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> > +		$(TARGET_DIR)$(sep))  
> 
> Why do you need two 'foreach' calls? Can't the following work?
> 
>     $(foreach pkg,$(1),\
>         rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
>             $(PER_PACKAGE_DIR)/$(pkg)/host/ \
>             $(HOST_DIR)$(sep) \
>         rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
>             $(PER_PACKAGE_DIR)/$(pkg)/target/ \
>             $(TARGET_DIR)$(sep))

As Arnout explained, it looked pretty nice to see the host directory
being populated first, then the target one. Having a single loop really
doesn't change anything in terms of the operation cost: we still have
the same number of rsync's. So I can of side with Arnout here, I like
the two loops. But if you disagree strongly, I can change it, it's not
a big/strong feeling on my side.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com