[Buildroot] [RFCv1 0/4] Per-package SDK and target directories

[Buildroot] [RFCv1 0/4] Per-package SDK and target directories

[Thomas Petazzoni]

Hello,

Here is a first iteration of the per-package SDK and target directory
implementation. This is definitely not ready for merging, but it
already works enough to give it some exposure. I'm sending it early so
that:

 - Interested people can take the patches, try them on their
   configurations, and report me the problems that they face.

 - Interested people start reviewing the patch series will it is still
   small and simple, because I'm sure it will grow quite a bit before
   it gets to a point where it can be merged.

It works well enough to build a complete toolchain and a system with
Busybox, Dropbear, a Python interpreter, strace, with the result
booting under Qemu.

If you're interested in testing it, you can find the patch series at:

  http://git.free-electrons.com/users/thomas-petazzoni/buildroot/log/?h=ppsh

Note: in the following discussion, we use the terminology "SDK" to
refer to the combination of HOST_DIR and STAGING_DIR, i.e the
cross-compiler and its sysroot with target libraries/headers, and a
bunch of other native programs. Since STAGING_DIR is included inside
HOST_DIR, the SDK directory is in essence just HOST_DIR.

What is per-package SDK and target?
===================================

Currently, output/host (SDK) and output/target are global directories:
all packages use files installed by other packages in those global
directories, and in turn into those global directories.

The idea of per-package SDK and target is that instead of having such
global directories, we would have one SDK directory and one target
directory per-package, containing just what this package needs
(dependencies), and in which the package installs its files.

Why do we want per-package SDK and target?
==========================================

There are two main motivations for per-package SDK and target
directories, which are related:

 1. Since the SDK directory is global, a package can currently see
    libraries/headers that it has not explicitly expressed a
    dependency on. So package A may not have a dependency on package
    B, but if package B happens to have been installed before, and
    package A "configure" script automaticatically detects a library
    installed by package B, it will use it. We have added tons of
    conditional dependencies in Buildroot to make such situations less
    problematic, but it is an endless fight, as upstream developers
    constantly add more optional dependencies to their software.

    Using per-package SDK, a given package uses as its SDK a directory
    that only contains the libraries/headers from packages explicitly
    listed in its dependencies. So it cannot mistakenly see a
    library/header installed by another package.

 2. Top-level parallel build (building multiple packages in parallel)
    is currently not enabled because we have global SDK and target
    directories.

    Let's imagine package A configure script auto-detects a library
    provided by package B, but Buildroot does not encode this
    dependency in package A's .mk file. Package A and package B are
    therefore totally independent from a build dependency point of
    view.

    Without top-level parallel build (current situation), you have a
    guarantee on the build order: either A is always built before B,
    or B is always built before A. So at least, every build gives the
    same result: A is built with B's functionality or without it, but
    it is consistent accross rebuilds.

    If you enable top-level parallel build, because A and B are
    totally independent, you can get A built before B or B built
    before A depending on the build scheduling. This can change at
    every build! This means that for a given configuration, A will
    sometimes be built with B's functionality, sometimes not. Totally
    unacceptable.

    And of course, this extends to case where package B gets installed
    while package A is being configured. So package A configure script
    will see some parts of B being installed, other parts not
    installed, causing all sort of weird and difficult to debug race
    conditions.

    By having per-package SDK directories, we ensure that package A
    will only see the dependencies its has explicitly expressed, and
    that no other package gets installed in parallel in the same SDK
    directory.

How is it implemented?
======================

The basic idea is pretty simple:

 - For each package, we have:

   output/per-package/<package>/host, which is the specific SDK
   directory for <package>.

   output/per-package/<package>/target, which is the specific target
   directory for <package>.

 - Before <package> is configured, we copy into its specific SDK and
   target directories the SDK and target directories from its direct
   dependencies. This basically populates its SDK with the
   cross-compiler and all libraries that <package> needs. This copy is
   done using rsync with hard links, so it is very fast and cheap from
   a storage consumption point of view.

 - While <package> is being built, HOST_DIR, STAGING_DIR and
   TARGET_DIR are tweaked to point to the current <package> specific
   SDK and target directories. I.e, HOST_DIR no longer points to
   output/host, but to output/per-package/<package>/host/.

And this is basically enough to get things working!

There are of course a few additional things to do:

 - At the end of the build, we still want a global target directory
   (to generate the filesystem images) and a global SDK. Therefore, in
   target-finalize, we assemble such global directories by copying the
   per-package SDK and target directories from all packages listed in
   the $(PACKAGES) variable.

 - We need to fix our pkg-config wrapper script so that it uses
   relative path instead of absolute paths. This allows the wrapper
   script to work regardless of which per-package SDK directory it is
   installed in.

 - We need to move away from using absolute RPATH for host binaries,
   and use LD_LIBRARY_PATH pointing to the current per-package SDK
   directory.

What remains to be done?
========================

 - Completely get rid of the global HOST_DIR, TARGET_DIR and
   STAGING_DIR definitions, so that nothing uses them, and they really
   become per-package variables.

 - While the toolchain sysroot, pkg-config paths and host RPATHs are
   properly handled, there are quite certainly a lot of other places
   that have absolute paths that won't work well with per-package SDK,
   and that need to be adjusted.

 - Perhaps use a temporary per-package SDK and target directory when
   building a package, and rename it to the final name once the
   package has finished building. This would allow to detect
   situations where the per-package SDK directory of package A
   contains absolute paths referring to the per-package SDK directory
   of package B. Such absolute paths are wrong, but we currently don't
   see them because the per-package SDK directory for package B still
   exists.

 - Many other issues that I'm sure people will report.

Comparison with Gustavo's patch series
======================================

Gustavo Zacarias did a previous implementation of this, available in
http://repo.or.cz/buildroot-gz.git/shortlog/refs/heads/pps3-next. Compared
to Gustavo's patch series, this patch series:

 - Implement per-package SDK and target directories, while Gustavo was
   only doing per-package staging.

 - Gustavo had several patches to pass BR_TOOLCHAIN_SYSROOT to the
   toolchain wrapper to pass the location of the current sysroot.

   This is no longer needed, because 1/ we're doing a full per-package
   SDK directory rather than per-package staging, which means the
   sysroot is always at the same relative location compared to the
   cross-compiler binary and 2/ the toolchain wrapper derives the
   sysroot location relatively to the wrapper location. Thanks to
   this, the following patches from Gustavo are (I believe) not
   needed:

   http://repo.or.cz/buildroot-gz.git/commitdiff/1ef6e798064649726d396bcb581ddbe4573c2460
   http://repo.or.cz/buildroot-gz.git/commitdiff/d15f6c6917b555bbbbbf97c292fad4db9d4007d4
   http://repo.or.cz/buildroot-gz.git/commitdiff/95900b65c29a010d85a769c9c6374cf3835f2169
   http://repo.or.cz/buildroot-gz.git/commitdiff/7ed53a5437ab09b85bf6d1953f6ff6049dfcc114

 - Gustavo had a patch to make our pkg-config wrapper script look at
   BR_TOOLCHAIN_SYSROOT, in order to find the
   /usr/{lib,share}/pkgconfig folders of the current sysroot. Just
   like for the toolchain-wrapper, such a change is no longer needed,
   and a simple change to our pkg-config script to use relative paths
   is sufficient.

 - I haven't integrated Gustavo patches that move from $(shell ...) to
   using backticks to delay the evaluation of
   STAGING_DIR/HOST_DIR/TARGET_DIR:

   http://repo.or.cz/buildroot-gz.git/commitdiff/0c11c60865f1445830643bb404f92c20d563260c
   http://repo.or.cz/buildroot-gz.git/commitdiff/207d2248ec8542293b6201b5c86f971021a3a591
   http://repo.or.cz/buildroot-gz.git/commitdiff/7f6a69819fbb176e29a1c3a53b4a76efe87dad15
   http://repo.or.cz/buildroot-gz.git/commitdiff/3328a9745eee555f5e5ef7df835afc26612ecd7b
   http://repo.or.cz/buildroot-gz.git/commitdiff/45a9d8c2aa6f3c0d2d8ed989d843890025faa3e8

 - I haven't looked at Qt specific issues, such as the ones fixed by
   Gustavo in:

   http://repo.or.cz/buildroot-gz.git/commitdiff/643e982e635f4386ccefcda9da4b84536af84d04

 - I am not doing all the .la files, *-config, *.prl, etc. tweaks that
   Gustavo is doing in:

   http://repo.or.cz/buildroot-gz.git/commitdiff/3f7b227b4c8e4514df6e5d54f88fcfb3a3a4bae0

   It is part of the "remaining absolute paths that need to be fixed"
   item that I mentionned above.

Best regards,

Thomas


Thomas Petazzoni (4):
  pkgconf: use relative path to STAGING_DIR instead of absolute path
  core: change host RPATH handling
  toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN
  core: implement per-package SDK and target

 Makefile                                           | 19 +++++++--
 package/Makefile.in                                |  2 +-
 package/pkg-generic.mk                             | 45 ++++++++++++++++++----
 package/pkgconf/pkgconf.mk                         |  4 +-
 support/scripts/fix-rpath                          | 15 +++++---
 .../toolchain-external/pkg-toolchain-external.mk   |  2 +-
 6 files changed, 67 insertions(+), 20 deletions(-)

-- 
2.13.6

[Buildroot] [RFCv1 1/4] pkgconf: use relative path to STAGING_DIR instead of absolute path

[Thomas Petazzoni]

The pkg-config wrapper script is currently generated with absolute
paths to $(STAGING_DIR). However, this will not work properly with
per-package SDK, and each package will be built with a different
STAGING_DIR value.

In order to fix this, we adjust how the pkg-config wrapper script is
generated, so that it uses a relative path to itself: the sysroot (i.e
STAGING_DIR) is always located in $(path of
pkg-config)/../$(STAGING_SUBDIR).

This change is independent from the per-package SDK work, and could be
applied independently from it.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/pkgconf/pkgconf.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/pkgconf/pkgconf.mk b/package/pkgconf/pkgconf.mk
index cc190d26da..d11ce485f7 100644
--- a/package/pkgconf/pkgconf.mk
+++ b/package/pkgconf/pkgconf.mk
@@ -19,8 +19,8 @@ endef
 define HOST_PKGCONF_INSTALL_WRAPPER
 	$(INSTALL) -m 0755 -D package/pkgconf/pkg-config.in \
 		$(HOST_DIR)/bin/pkg-config
-	$(SED) 's, at PKG_CONFIG_LIBDIR@,$(STAGING_DIR)/usr/lib/pkgconfig:$(STAGING_DIR)/usr/share/pkgconfig,' \
-		-e 's, at STAGING_DIR@,$(STAGING_DIR),' \
+	$(SED) 's, at PKG_CONFIG_LIBDIR@,$$(dirname $$0)/../$(STAGING_SUBDIR)/usr/lib/pkgconfig:$$(dirname $$0)/../$(STAGING_SUBDIR)/usr/share/pkgconfig,' \
+		-e 's, at STAGING_DIR@,$$(dirname $$0)/../$(STAGING_SUBDIR),' \
 		$(HOST_DIR)/bin/pkg-config
 endef
 
-- 
2.13.6

[Buildroot] [RFCv1 1/4] pkgconf: use relative path to STAGING_DIR instead of absolute path

[Arnout Vandecappelle]

On 03-11-17 17:06, Thomas Petazzoni wrote:
> The pkg-config wrapper script is currently generated with absolute
> paths to $(STAGING_DIR). However, this will not work properly with
> per-package SDK, and each package will be built with a different
> STAGING_DIR value.
> 
> In order to fix this, we adjust how the pkg-config wrapper script is
> generated, so that it uses a relative path to itself: the sysroot (i.e
> STAGING_DIR) is always located in $(path of
> pkg-config)/../$(STAGING_SUBDIR).
> 
> This change is independent from the per-package SDK work, and could be
> applied independently from it.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

 It's OK as it is

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 However...

> ---
>  package/pkgconf/pkgconf.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/pkgconf/pkgconf.mk b/package/pkgconf/pkgconf.mk
> index cc190d26da..d11ce485f7 100644
> --- a/package/pkgconf/pkgconf.mk
> +++ b/package/pkgconf/pkgconf.mk
> @@ -19,8 +19,8 @@ endef
>  define HOST_PKGCONF_INSTALL_WRAPPER
>  	$(INSTALL) -m 0755 -D package/pkgconf/pkg-config.in \
>  		$(HOST_DIR)/bin/pkg-config
> -	$(SED) 's, at PKG_CONFIG_LIBDIR@,$(STAGING_DIR)/usr/lib/pkgconfig:$(STAGING_DIR)/usr/share/pkgconfig,' \
> -		-e 's, at STAGING_DIR@,$(STAGING_DIR),' \
> +	$(SED) 's, at PKG_CONFIG_LIBDIR@,$$(dirname $$0)/../$(STAGING_SUBDIR)/usr/lib/pkgconfig:$$(dirname $$0)/../$(STAGING_SUBDIR)/usr/share/pkgconfig,' \
> +		-e 's, at STAGING_DIR@,$$(dirname $$0)/../$(STAGING_SUBDIR),' \

 Instead of all this complexity, why not do it directly in pkg-config.in, and
just substitute @STAGING_SUBDIR@ ? Note that the file will have 4 instances of
$(dirname $0) so an intermediary variable is called for.

 If you do that, you can maybe also do some line splitting there, and add the
missing exec.

 Regards,
 Arnout

>  		$(HOST_DIR)/bin/pkg-config
>  endef
>  
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Currently, our strategy for the RPATH of host binaries is as follows:

 - During the build, we force an absolute RPATH to be encoded into
   binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
   host binaries will find their libraries.

 - In the "make sdk" target, when preparing the SDK to be relocatable,
   we use patchelf to replace those absolute RPATHs by relative RPATHs
   that use $ORIGIN.

Unfortunately, the use of absolute RPATH during the build is not
compatible with the move to per-package SDK, where a different host
directory is used for the build of each package. Therefore, we need to
move to a different strategy for RPATH handling.

The new strategy is as follows:

 - During the build, we do not encode any RPATH into the host
   binaries. Instead, we specify LD_LIBRARY_PATH environment to point
   to the current HOST_DIR/lib directory (for the package being
   currently built).

 - At the end of the build, in order to make sure that binaries are
   usable without LD_LIBRARY_PATH, we fixup the host binaries to use
   $ORIGIN/../lib. This is more-or-less what was done previously in
   the "make sdk" target, except that instead of turning absolute
   paths into relative paths in the RPATH, we are simply setting the
   RPATH to $ORIGIN/../lib.

   In order to implement this, the fix-rpath script logic is adjusted
   for the "host" case.

An alternative strategy would have been to keep the
-Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
host binaries, and fix such RPATH at the end of the build of every
package. However, that would require calling fix-rpath after the
installation of every package, which is a bit expensive.

This change is independent from the per-package SDK functionality, and
could be applied separately.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile                  |  4 +++-
 package/Makefile.in       |  2 +-
 package/pkg-generic.mk    |  8 --------
 support/scripts/fix-rpath | 15 ++++++++++-----
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index 79db7fe48a..5496273329 100644
--- a/Makefile
+++ b/Makefile
@@ -231,6 +231,8 @@ LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
 LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
 LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
 
+export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
+
 ################################################################################
 #
 # staging and target directories do NOT list these as
@@ -558,7 +560,6 @@ world: target-post-image
 .PHONY: sdk
 sdk: world
 	@$(call MESSAGE,"Rendering the SDK relocatable")
-	$(TOPDIR)/support/scripts/fix-rpath host
 	$(TOPDIR)/support/scripts/fix-rpath staging
 	$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
 	echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
@@ -679,6 +680,7 @@ $(TARGETS_ROOTFS): target-finalize
 .PHONY: target-finalize
 target-finalize: $(PACKAGES)
 	@$(call MESSAGE,"Finalizing target directory")
+	$(TOPDIR)/support/scripts/fix-rpath host
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
 	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
 		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
diff --git a/package/Makefile.in b/package/Makefile.in
index a1a5316051..e94a75c230 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -220,7 +220,7 @@ HOST_CPPFLAGS  = -I$(HOST_DIR)/include
 HOST_CFLAGS   ?= -O2
 HOST_CFLAGS   += $(HOST_CPPFLAGS)
 HOST_CXXFLAGS += $(HOST_CFLAGS)
-HOST_LDFLAGS  += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib
+HOST_LDFLAGS  += -L$(HOST_DIR)/lib
 
 # The macros below are taken from linux 4.11 and adapted slightly.
 # Copy more when needed.
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index cca94ba338..82f8c06821 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -105,14 +105,6 @@ endef
 
 GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
 
-# This hook checks that host packages that need libraries that we build
-# have a proper DT_RPATH or DT_RUNPATH tag
-define check_host_rpath
-	$(if $(filter install-host,$(2)),\
-		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR)))
-endef
-GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
-
 define step_check_build_dir_one
 	if [ -d $(2) ]; then \
 		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
index 15705a3b0d..eed751f5da 100755
--- a/support/scripts/fix-rpath
+++ b/support/scripts/fix-rpath
@@ -61,7 +61,7 @@ main() {
     local rootdir
     local tree="${1}"
     local find_args=( )
-    local sanitize_extra_args=( )
+    local sanitize_args=( )
 
     if ! "${PATCHELF}" --version > /dev/null 2>&1; then
 	echo "Error: can't execute patchelf utility '${PATCHELF}'"
@@ -89,7 +89,8 @@ main() {
             cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
 
             # we always want $ORIGIN-based rpaths to make it relocatable.
-            sanitize_extra_args+=( "--relative-to-file" )
+            sanitize_args+=( "--set-rpath" )
+            sanitize_args+=( "\$ORIGIN/../lib" )
             ;;
 
         staging)
@@ -101,14 +102,18 @@ main() {
             done
 
             # should be like for the target tree below
-            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--make-rpath-relative" )
+            sanitize_args+=( "${rootdir}" )
             ;;
 
         target)
             rootdir="${TARGET_DIR}"
             # we don't want $ORIGIN-based rpaths but absolute paths without rootdir.
             # we also want to remove rpaths pointing to /lib or /usr/lib.
-            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--make-rpath-relative" )
+            sanitize_args+=( "${rootdir}" )
             ;;
 
         *)
@@ -125,7 +130,7 @@ main() {
             # make files writable if necessary
             changed=$(chmod -c u+w "${file}")
             # call patchelf to sanitize the rpath
-            ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
+            ${PATCHELF} ${sanitize_args[@]} "${file}"
             # restore the original permission
             test "${changed}" != "" && chmod u-w "${file}"
         fi
-- 
2.13.6

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Yann E. MORIN]

Thomas, All,

Here is a summary of our discussion on IRC...

On 2017-11-03 17:06 +0100, Thomas Petazzoni spake thusly:
> Currently, our strategy for the RPATH of host binaries is as follows:
> 
>  - During the build, we force an absolute RPATH to be encoded into
>    binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
>    host binaries will find their libraries.
> 
>  - In the "make sdk" target, when preparing the SDK to be relocatable,
>    we use patchelf to replace those absolute RPATHs by relative RPATHs
>    that use $ORIGIN.
> 
> Unfortunately, the use of absolute RPATH during the build is not
> compatible with the move to per-package SDK, where a different host
> directory is used for the build of each package. Therefore, we need to
> move to a different strategy for RPATH handling.
> 
> The new strategy is as follows:
> 
>  - During the build, we do not encode any RPATH into the host
>    binaries. Instead, we specify LD_LIBRARY_PATH environment to point
>    to the current HOST_DIR/lib directory (for the package being
>    currently built).

In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
from the environment).

Briefly, it does not work when we build a library that is also present
on the distro, which is used by a tool from the distro, and we build it
in an incompatibl way.

So, we'll have to come up with another solution, like your suggested
alternative.

>  - At the end of the build, in order to make sure that binaries are
>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>    $ORIGIN/../lib. This is more-or-less what was done previously in
>    the "make sdk" target, except that instead of turning absolute
>    paths into relative paths in the RPATH, we are simply setting the
>    RPATH to $ORIGIN/../lib.
> 
>    In order to implement this, the fix-rpath script logic is adjusted
>    for the "host" case.
> 
> An alternative strategy would have been to keep the
> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> host binaries, and fix such RPATH at the end of the build of every
> package. However, that would require calling fix-rpath after the
> installation of every package, which is a bit expensive.

I wonder how expensive that would be. It would be nice to time this.

Also note that, even if the overhead is noticeable, this would be
compensated by the mere fact that we are now doing a parallel build, so
we would end up winning.

Regards,
Yann E. MORIN.

> This change is independent from the per-package SDK functionality, and
> could be applied separately.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  Makefile                  |  4 +++-
>  package/Makefile.in       |  2 +-
>  package/pkg-generic.mk    |  8 --------
>  support/scripts/fix-rpath | 15 ++++++++++-----
>  4 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 79db7fe48a..5496273329 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -231,6 +231,8 @@ LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
>  LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
>  LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
>  
> +export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
> +
>  ################################################################################
>  #
>  # staging and target directories do NOT list these as
> @@ -558,7 +560,6 @@ world: target-post-image
>  .PHONY: sdk
>  sdk: world
>  	@$(call MESSAGE,"Rendering the SDK relocatable")
> -	$(TOPDIR)/support/scripts/fix-rpath host
>  	$(TOPDIR)/support/scripts/fix-rpath staging
>  	$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
>  	echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
> @@ -679,6 +680,7 @@ $(TARGETS_ROOTFS): target-finalize
>  .PHONY: target-finalize
>  target-finalize: $(PACKAGES)
>  	@$(call MESSAGE,"Finalizing target directory")
> +	$(TOPDIR)/support/scripts/fix-rpath host
>  	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
>  	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
>  		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
> diff --git a/package/Makefile.in b/package/Makefile.in
> index a1a5316051..e94a75c230 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -220,7 +220,7 @@ HOST_CPPFLAGS  = -I$(HOST_DIR)/include
>  HOST_CFLAGS   ?= -O2
>  HOST_CFLAGS   += $(HOST_CPPFLAGS)
>  HOST_CXXFLAGS += $(HOST_CFLAGS)
> -HOST_LDFLAGS  += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib
> +HOST_LDFLAGS  += -L$(HOST_DIR)/lib
>  
>  # The macros below are taken from linux 4.11 and adapted slightly.
>  # Copy more when needed.
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index cca94ba338..82f8c06821 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -105,14 +105,6 @@ endef
>  
>  GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
>  
> -# This hook checks that host packages that need libraries that we build
> -# have a proper DT_RPATH or DT_RUNPATH tag
> -define check_host_rpath
> -	$(if $(filter install-host,$(2)),\
> -		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR)))
> -endef
> -GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
> -
>  define step_check_build_dir_one
>  	if [ -d $(2) ]; then \
>  		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
> index 15705a3b0d..eed751f5da 100755
> --- a/support/scripts/fix-rpath
> +++ b/support/scripts/fix-rpath
> @@ -61,7 +61,7 @@ main() {
>      local rootdir
>      local tree="${1}"
>      local find_args=( )
> -    local sanitize_extra_args=( )
> +    local sanitize_args=( )
>  
>      if ! "${PATCHELF}" --version > /dev/null 2>&1; then
>  	echo "Error: can't execute patchelf utility '${PATCHELF}'"
> @@ -89,7 +89,8 @@ main() {
>              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
>  
>              # we always want $ORIGIN-based rpaths to make it relocatable.
> -            sanitize_extra_args+=( "--relative-to-file" )
> +            sanitize_args+=( "--set-rpath" )
> +            sanitize_args+=( "\$ORIGIN/../lib" )
>              ;;
>  
>          staging)
> @@ -101,14 +102,18 @@ main() {
>              done
>  
>              # should be like for the target tree below
> -            sanitize_extra_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--make-rpath-relative" )
> +            sanitize_args+=( "${rootdir}" )
>              ;;
>  
>          target)
>              rootdir="${TARGET_DIR}"
>              # we don't want $ORIGIN-based rpaths but absolute paths without rootdir.
>              # we also want to remove rpaths pointing to /lib or /usr/lib.
> -            sanitize_extra_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--make-rpath-relative" )
> +            sanitize_args+=( "${rootdir}" )
>              ;;
>  
>          *)
> @@ -125,7 +130,7 @@ main() {
>              # make files writable if necessary
>              changed=$(chmod -c u+w "${file}")
>              # call patchelf to sanitize the rpath
> -            ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
> +            ${PATCHELF} ${sanitize_args[@]} "${file}"
>              # restore the original permission
>              test "${changed}" != "" && chmod u-w "${file}"
>          fi
> -- 
> 2.13.6
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Hello,

On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:

> In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
> not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
> from the environment).
> 
> Briefly, it does not work when we build a library that is also present
> on the distro, which is used by a tool from the distro, and we build it
> in an incompatibl way.
> 
> So, we'll have to come up with another solution, like your suggested
> alternative.
> 
> >  - At the end of the build, in order to make sure that binaries are
> >    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
> >    $ORIGIN/../lib. This is more-or-less what was done previously in
> >    the "make sdk" target, except that instead of turning absolute
> >    paths into relative paths in the RPATH, we are simply setting the
> >    RPATH to $ORIGIN/../lib.
> > 
> >    In order to implement this, the fix-rpath script logic is adjusted
> >    for the "host" case.
> > 
> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
> I wonder how expensive that would be. It would be nice to time this.
> 
> Also note that, even if the overhead is noticeable, this would be
> compensated by the mere fact that we are now doing a parallel build, so
> we would end up winning.

Thanks for the feedback. We already discussed this on IRC, and I agree.
I'll have a look at implementing the solution of doing the patchelf
adding $ORIGIN/../lib as an RPATH directly after the installation of
each package.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 05-11-17 15:44, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:
> 
>> In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
>> not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
>> from the environment).
>>
>> Briefly, it does not work when we build a library that is also present
>> on the distro, which is used by a tool from the distro, and we build it
>> in an incompatibl way.
>>
>> So, we'll have to come up with another solution, like your suggested
>> alternative.
>>
>>>  - At the end of the build, in order to make sure that binaries are
>>>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>>>    $ORIGIN/../lib. This is more-or-less what was done previously in
>>>    the "make sdk" target, except that instead of turning absolute
>>>    paths into relative paths in the RPATH, we are simply setting the
>>>    RPATH to $ORIGIN/../lib.
>>>
>>>    In order to implement this, the fix-rpath script logic is adjusted
>>>    for the "host" case.
>>>
>>> An alternative strategy would have been to keep the
>>> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
>>> host binaries, and fix such RPATH at the end of the build of every
>>> package. However, that would require calling fix-rpath after the
>>> installation of every package, which is a bit expensive.  
>>
>> I wonder how expensive that would be. It would be nice to time this.
>>
>> Also note that, even if the overhead is noticeable, this would be
>> compensated by the mere fact that we are now doing a parallel build, so
>> we would end up winning.
> 
> Thanks for the feedback. We already discussed this on IRC, and I agree.
> I'll have a look at implementing the solution of doing the patchelf
> adding $ORIGIN/../lib as an RPATH directly after the installation of
> each package.

 Note that according to Wolfgang's measurements (if I remember correctly), doing
fix-rpath on the host dir didn't take too long, but doing it in staging does.
There aren't that many host files.

 Regards,
 Arnout

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Hello,

On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:

> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
> I wonder how expensive that would be. It would be nice to time this.

OK, to time this, I did a quick hacky shell script
(http://code.bulix.org/uboevd-223829). What it does is:

 - Generate a mix of ELF files and random data files (half of the files
   are ELF files, half are random data files). This is the "generate"
   target of the script.

 - Fixup rpath to $ORIGIN/../lib if not already fixed. We don't want to
   fixup files repeatedly, as we don't want to duplicate files over and
   over by breaking hard links. So we really want to set the RPATH only
   once per binary.

Results are as follows:

$ ./rpath-evaluation.sh generate 0 500
... generate 250 random data files, 250 ELF programs with a crappy
rpath ...

$ time ./rpath-evaluation.sh fixup

real	0m0.976s
user	0m0.546s
sys	0m0.457s
$ time ./rpath-evaluation.sh fixup

real	0m0.610s
user	0m0.393s
sys	0m0.234s

So the first fixup pass, which actually fixes the RPATH of 250 binaries
takes about one second. The next fixup pass doesn't do anything, except
checking that all binaries already have a fixed RPATH.

So we see that it will take ~600ms to scan 500 files, provided half
of them are ELF files, verify that they already have the correct RPATH.

To me, this sounds very reasonable. What do you think?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 07-11-17 23:41, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:
> 
>>> An alternative strategy would have been to keep the
>>> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
>>> host binaries, and fix such RPATH at the end of the build of every
>>> package. However, that would require calling fix-rpath after the
>>> installation of every package, which is a bit expensive.  
>>
>> I wonder how expensive that would be. It would be nice to time this.
> 
> OK, to time this, I did a quick hacky shell script
> (http://code.bulix.org/uboevd-223829). What it does is:
> 
>  - Generate a mix of ELF files and random data files (half of the files
>    are ELF files, half are random data files). This is the "generate"
>    target of the script.
> 
>  - Fixup rpath to $ORIGIN/../lib if not already fixed. We don't want to
>    fixup files repeatedly, as we don't want to duplicate files over and
>    over by breaking hard links. So we really want to set the RPATH only
>    once per binary.
> 
> Results are as follows:
> 
> $ ./rpath-evaluation.sh generate 0 500
> ... generate 250 random data files, 250 ELF programs with a crappy
> rpath ...
> 
> $ time ./rpath-evaluation.sh fixup
> 
> real	0m0.976s
> user	0m0.546s
> sys	0m0.457s
> $ time ./rpath-evaluation.sh fixup
> 
> real	0m0.610s
> user	0m0.393s
> sys	0m0.234s
> 
> So the first fixup pass, which actually fixes the RPATH of 250 binaries
> takes about one second. The next fixup pass doesn't do anything, except
> checking that all binaries already have a fixed RPATH.
> 
> So we see that it will take ~600ms to scan 500 files, provided half
> of them are ELF files, verify that they already have the correct RPATH.
> 
> To me, this sounds very reasonable. What do you think?

 500 files is not a lot if you include staging as well, so I've repeated the
experiment with 50000 files and on a HDD. First pass takes 3m34, second pass
3m43. Hm, perhaps I should have left my computer alone for a while during this
experiment :-).

 Anyway, I think the bottom line is that the fixup script should avoid going
over staging. But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

 Regards,
 Arnout

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Hello,

On Wed, 8 Nov 2017 00:50:15 +0100, Arnout Vandecappelle wrote:

> > To me, this sounds very reasonable. What do you think?  
> 
>  500 files is not a lot if you include staging as well

Why would I include staging? I don't see at all why fixing up RPATH in
staging after each package build would be necessary.

> so I've repeated the experiment with 50000 files and on a HDD. First
> pass takes 3m34, second pass 3m43. Hm, perhaps I should have left my
> computer alone for a while during this experiment :-).

It's not very logical that the second pass is longer than the first
pass, since the first pass does do patchelf invocations per file, while
the second pass does only one patchelf invocation per file.

>  Anyway, I think the bottom line is that the fixup script should
> avoid going over staging.

Why would it need to go over staging?

> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

Yes, that was my intent. If needed, we could optimize it by only doing
the fixup at the end of a host package installation. But since we have
a few target packages installing host stuff (toolchain, qt, etc.), it
would require them to explicitly state that they need host rpath
fixups. Perhaps it's easiest for now to just do the rpath fixup after
every package installation.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 08-11-17 09:55, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 8 Nov 2017 00:50:15 +0100, Arnout Vandecappelle wrote:
> 
>>> To me, this sounds very reasonable. What do you think?  
>>
>>  500 files is not a lot if you include staging as well
> 
> Why would I include staging? I don't see at all why fixing up RPATH in
> staging after each package build would be necessary.

 The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.

> 
>> so I've repeated the experiment with 50000 files and on a HDD. First
>> pass takes 3m34, second pass 3m43. Hm, perhaps I should have left my
>> computer alone for a while during this experiment :-).
> 
> It's not very logical that the second pass is longer than the first
> pass, since the first pass does do patchelf invocations per file, while
> the second pass does only one patchelf invocation per file.
> 
>>  Anyway, I think the bottom line is that the fixup script should
>> avoid going over staging.
> 
> Why would it need to go over staging?
> 
>> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

 So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
So then it's perhaps easier to just -prune staging.

> Yes, that was my intent. If needed, we could optimize it by only doing
> the fixup at the end of a host package installation. But since we have
> a few target packages installing host stuff (toolchain, qt, etc.), it
> would require them to explicitly state that they need host rpath
> fixups. Perhaps it's easiest for now to just do the rpath fixup after
> every package installation.

 After every package installation would be better, yes.

 Regards,
 Arnout


> 
> Best regards,
> 
> Thomas
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Hello,

On Wed, 8 Nov 2017 11:55:19 +0100, Arnout Vandecappelle wrote:

> >>> To me, this sounds very reasonable. What do you think?    
> >>
> >>  500 files is not a lot if you include staging as well  
> > 
> > Why would I include staging? I don't see at all why fixing up RPATH in
> > staging after each package build would be necessary.  
> 
>  The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.

Hum, you didn't reply to my question, which was: "Why would we need to
fixup RPATHs in STAGING_DIR" ?

> >> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.  
> 
>  So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
> So then it's perhaps easier to just -prune staging.

Absolutely.

> > Yes, that was my intent. If needed, we could optimize it by only doing
> > the fixup at the end of a host package installation. But since we have
> > a few target packages installing host stuff (toolchain, qt, etc.), it
> > would require them to explicitly state that they need host rpath
> > fixups. Perhaps it's easiest for now to just do the rpath fixup after
> > every package installation.  
> 
>  After every package installation would be better, yes.

ACK.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 08-11-17 13:30, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 8 Nov 2017 11:55:19 +0100, Arnout Vandecappelle wrote:
> 
>>>>> To me, this sounds very reasonable. What do you think?    
>>>>
>>>>  500 files is not a lot if you include staging as well  
>>>
>>> Why would I include staging? I don't see at all why fixing up RPATH in
>>> staging after each package build would be necessary.  
>>
>>  The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.
> 
> Hum, you didn't reply to my question, which was: "Why would we need to
> fixup RPATHs in STAGING_DIR" ?

 We don't, obviously. However, if we naively go through HOST_DIR we will include
STAGING_DIR. I thought that was obvious :-)

 Regards,
 Arnout

> 
>>>> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.  
>>
>>  So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
>> So then it's perhaps easier to just -prune staging.
> 
> Absolutely.
> 
>>> Yes, that was my intent. If needed, we could optimize it by only doing
>>> the fixup at the end of a host package installation. But since we have
>>> a few target packages installing host stuff (toolchain, qt, etc.), it
>>> would require them to explicitly state that they need host rpath
>>> fixups. Perhaps it's easiest for now to just do the rpath fixup after
>>> every package installation.  
>>
>>  After every package installation would be better, yes.
> 
> ACK.
> 
> Thomas
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 03-11-17 17:06, Thomas Petazzoni wrote:
> Currently, our strategy for the RPATH of host binaries is as follows:
> 
>  - During the build, we force an absolute RPATH to be encoded into
>    binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
>    host binaries will find their libraries.
> 
>  - In the "make sdk" target, when preparing the SDK to be relocatable,
>    we use patchelf to replace those absolute RPATHs by relative RPATHs
>    that use $ORIGIN.
> 
> Unfortunately, the use of absolute RPATH during the build is not
> compatible with the move to per-package SDK, where a different host
> directory is used for the build of each package. Therefore, we need to
> move to a different strategy for RPATH handling.
> 
> The new strategy is as follows:
> 
>  - During the build, we do not encode any RPATH into the host
>    binaries. Instead, we specify LD_LIBRARY_PATH environment to point
>    to the current HOST_DIR/lib directory (for the package being
>    currently built).
> 
>  - At the end of the build, in order to make sure that binaries are
>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>    $ORIGIN/../lib. This is more-or-less what was done previously in
>    the "make sdk" target, except that instead of turning absolute
>    paths into relative paths in the RPATH, we are simply setting the
>    RPATH to $ORIGIN/../lib.
> 
>    In order to implement this, the fix-rpath script logic is adjusted
>    for the "host" case.
> 
> An alternative strategy would have been to keep the
> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> host binaries, and fix such RPATH at the end of the build of every
> package. However, that would require calling fix-rpath after the
> installation of every package, which is a bit expensive.

 As discussed, this would be the preferred approach after all.

> 
> This change is independent from the per-package SDK functionality, and
> could be applied separately.

 Shouldn't this type of comment be below the --- ?

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
[snip]
> @@ -89,7 +89,8 @@ main() {
>              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
>  
>              # we always want $ORIGIN-based rpaths to make it relocatable.
> -            sanitize_extra_args+=( "--relative-to-file" )
> +            sanitize_args+=( "--set-rpath" )
> +            sanitize_args+=( "\$ORIGIN/../lib" )

 One advantage of --make-rpath-relative over --set-rpath is that the former will
also remove rpaths that are not needed (which I guess is often the case for host
tools). OTOH for host tools this is not really important. And it doesn't even
save size, patchelf doesn't shift sections to the left (IIRC). But it does save
a tiny amount of startup time - for gcc, this could have an impact on overall
build time.


 Regards,
 Arnout

[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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Thomas Petazzoni]

Hello,

On Tue, 7 Nov 2017 22:15:38 +0100, Arnout Vandecappelle wrote:

> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
>  As discussed, this would be the preferred approach after all.

Yes, agreed.

> > This change is independent from the per-package SDK functionality, and
> > could be applied separately.  
> 
>  Shouldn't this type of comment be below the --- ?

Well, it's not really that I expected the v1 of the patches to be
merged :-)

> > @@ -89,7 +89,8 @@ main() {
> >              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
> >  
> >              # we always want $ORIGIN-based rpaths to make it relocatable.
> > -            sanitize_extra_args+=( "--relative-to-file" )
> > +            sanitize_args+=( "--set-rpath" )
> > +            sanitize_args+=( "\$ORIGIN/../lib" )  
> 
>  One advantage of --make-rpath-relative over --set-rpath is that the former will
> also remove rpaths that are not needed (which I guess is often the case for host
> tools).

Well, --set-rpath sets the RPATH, so I believe it entirely removes any
existing RPATH and replaces them all by just $ORIGIN/../lib. So I don't
see how it could be more "efficient", since we're down to a single
RPATH, which is the only rpath that is needed.

Perhaps the only problem that I can think of is if some host binaries
do have libraries installed in non-standard locations, and really need
a custom absolute RPATH such
as /home/foo/buildroot/output/per-package/host/lib/baz/ to be turned
into $ORIGIN/../lib/baz/. However, I don't think we have such
non-standard library locations for host packages, so I simply ignored
this possible problem for now. It is a problem that can be fixed later
on if needed by adjusting the host rpath mangling logic.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 2/4] core: change host RPATH handling

[Arnout Vandecappelle]

On 07-11-17 22:22, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 7 Nov 2017 22:15:38 +0100, Arnout Vandecappelle wrote:
[snip]
>>  One advantage of --make-rpath-relative over --set-rpath is that the former will
>> also remove rpaths that are not needed (which I guess is often the case for host
>> tools).
> 
> Well, --set-rpath sets the RPATH, so I believe it entirely removes any
> existing RPATH and replaces them all by just $ORIGIN/../lib. So I don't
> see how it could be more "efficient", since we're down to a single
> RPATH, which is the only rpath that is needed.

 In many cases, no rpath is needed because it doesn't link with any
Buildroot-built libraries. In that case, no rpath is slightly more efficient for
application startup.

> 
> Perhaps the only problem that I can think of is if some host binaries
> do have libraries installed in non-standard locations, and really need
> a custom absolute RPATH such
> as /home/foo/buildroot/output/per-package/host/lib/baz/ to be turned
> into $ORIGIN/../lib/baz/. However, I don't think we have such
> non-standard library locations for host packages, so I simply ignored
> this possible problem for now. It is a problem that can be fixed later
> on if needed by adjusting the host rpath mangling logic.

 I've taken a quick look at what I have in my build dirs, and there is no .so
file in another location except for dlopen()'d stuff.

 Regards,
 Arnout

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 3/4] toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN

[Thomas Petazzoni]

The upcoming per-package SDK functionality is heavily based on the
fact that HOST_DIR, STAGING_DIR and TARGET_DIR are evaluated during
the configure/build/install steps of the packages. Therefore, any
evaluation-during-assignment using := is going to cause problems, and
need to be turned into evaluation-during-use using =.

This patch fix up one such instance in the external toolchain code.

This change is independent from the per-package SDK functionality, and
could be applied separately.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
index dc0588c536..b9ad1720a1 100644
--- a/toolchain/toolchain-external/pkg-toolchain-external.mk
+++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
@@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
 TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
 endif
 else
-TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
+TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
 endif
 
 # If this is a buildroot toolchain, it already has a wrapper which we want to
-- 
2.13.6

[Buildroot] [RFCv1 3/4] toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN

[Arnout Vandecappelle]

On 03-11-17 17:06, Thomas Petazzoni wrote:
> The upcoming per-package SDK functionality is heavily based on the
> fact that HOST_DIR, STAGING_DIR and TARGET_DIR are evaluated during
> the configure/build/install steps of the packages. Therefore, any
> evaluation-during-assignment using := is going to cause problems, and
> need to be turned into evaluation-during-use using =.
> 
> This patch fix up one such instance in the external toolchain code.
             fixes

> 
> This change is independent from the per-package SDK functionality, and
> could be applied separately.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> index dc0588c536..b9ad1720a1 100644
> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
>  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
>  endif
>  else
> -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
> +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin

 It gives me the shivers to see the same variable sometimes defined recursive
and sometimes defined immediate... But I can't find an elegant solution (other
than introducing an artificial extra variable, which is also ugly), so

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout


>  endif
>  
>  # If this is a buildroot toolchain, it already has a wrapper which we want to
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 3/4] toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN

[Thomas Petazzoni]

Hello,

On Tue, 7 Nov 2017 22:23:24 +0100, Arnout Vandecappelle wrote:

> > diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > index dc0588c536..b9ad1720a1 100644
> > --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> > +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
> >  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
> >  endif
> >  else
> > -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
> > +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin  
> 
>  It gives me the shivers to see the same variable sometimes defined recursive
> and sometimes defined immediate... But I can't find an elegant solution (other
> than introducing an artificial extra variable, which is also ugly), so
> 
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

I also saw the other TOOLCHAIN_EXTERNAL_BIN assignment using := above,
and didn't change it for now because that was not the case I was
interested in testing/fixing. But my plan was to get back to this :=
assignment at some point. However, it seems like you have concluded
that we have to keep := here. Could you explain why?

Note: I think keeping the := is OK, because TOOLCHAIN_EXTERNAL_PREFIX
doesn't reference anything like HOST_DIR/STAGING_DIR/TARGET_DIR, so
it's fine to have evaluation at the time of assignment for this case.

Perhaps a comment above would help clarify that it's intentional?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 3/4] toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN

[Arnout Vandecappelle]

On 07-11-17 22:33, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 7 Nov 2017 22:23:24 +0100, Arnout Vandecappelle wrote:
> 
>>> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> index dc0588c536..b9ad1720a1 100644
>>> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
>>>  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
>>>  endif
>>>  else
>>> -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
>>> +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin  
>>
>>  It gives me the shivers to see the same variable sometimes defined recursive
>> and sometimes defined immediate... But I can't find an elegant solution (other
>> than introducing an artificial extra variable, which is also ugly), so
>>
>> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> I also saw the other TOOLCHAIN_EXTERNAL_BIN assignment using := above,
> and didn't change it for now because that was not the case I was
> interested in testing/fixing. But my plan was to get back to this :=
> assignment at some point. However, it seems like you have concluded
> that we have to keep := here. Could you explain why?

 It's using $(shell which ...). That variable ends up in $(TARGET_CC), so it is
re-evaluated hundreds of times. I don't know if it really makes much of a
difference, but the rule of thumb is: think twice about using
recursively-expanded if it's used in many places and it contains $(shell ...)
constructs.

> 
> Note: I think keeping the := is OK, because TOOLCHAIN_EXTERNAL_PREFIX
> doesn't reference anything like HOST_DIR/STAGING_DIR/TARGET_DIR, so
> it's fine to have evaluation at the time of assignment for this case.
> 
> Perhaps a comment above would help clarify that it's intentional?

 Not really needed, the code speaks for itself. The $(shell) makes it quite easy
to see why immediate expansion is chosen there.

 Regards,
 Arnout

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni]

This commit implemnts the core of the move to per-package SDK and
target directories. The main idea is that instead of having a global
output/host and output/target in which all packages install files, we
switch to per-package host and target folders, that only contain their
explicit dependencies.

There are two main benefits:

 - Packages will no longer discover dependencies that they do not
   explicitly indicate in their <pkg>_DEPENDENCIES variable.

 - We can support top-level parallel build properly, because a package
   only "sees" its own host directory and target directory, isolated
   from the build of other packages that can happen in parallel.

It works as follows:

 - A new output/per-package/ folder is created, which will contain one
   sub-folder per package, and inside it, a "host" folder and a
   "target" folder:

   output/per-package/busybox/target
   output/per-package/busybox/host
   output/per-package/host-fakeroot/target
   output/per-package/host-fakeroot/host

   This output/per-package/ folder is PER_PACKAGE_DIR, and each
   package defines <pkg>_TARGET_DIR, <pkg>_HOST_DIR and
   <pkg>_STAGING_DIR pointing to its own target, host and staging
   directories.

 - The <pkg>_TARGET_DIR, <pkg>_HOST_DIR and <pkg>_STAGING_DIR variable
   are passed as TARGET_DIR, HOST_DIR and STAGING_DIR to the various
   make targets used for the different build steps of the
   packages. Effectively this means that when a package references
   $(HOST_DIR), the value is in fact $(<pkg>_HOST_DIR).

 - Of course, packages have dependencies, so those dependencies must
   be installed in the per-package host and target folders before
   configuring the package. To do so, we simply rsync (using hard
   links to save space and time) the host and target folders of the
   direct dependencies of the package to the current package host and
   target folders. We only need to take care of direct dependencies
   (and not recursively all dependencies), because we accumulate into
   those per-package host and target folders the files installed by
   the dependencies.

 - We fix LD_LIBRARY_PATH to make it point to the current package host
   directory.

This is basically enough to make per-package SDK and target work. The
only gotcha is that at the end of the build, output/target and
output/host are empty, which means that:

 - The filesystem image creation code cannot work.

 - We don't have a SDK to build code outside of Buildroot.

In order to fix this, this commit extends the target-finalize step so
that it starts by populating output/target and output/host by
rsync-ing into them the target and host directories of all packages
listed in the $(PACKAGES) variable.

[Note: in fact, output/host is not completely empty, even though they
should be. It contains the following files:
output/host/
output/host/lib64
output/host/usr
output/host/share
output/host/share/buildroot
output/host/share/buildroot/Platform
output/host/share/buildroot/Platform/Buildroot.cmake
output/host/share/buildroot/toolchainfile.cmake
output/host/lib
Perhaps those files should be installed by some package instead,
perhaps the 'toolchain' package.]

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile               | 17 ++++++++++++++---
 package/pkg-generic.mk | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 5496273329..4e1ba62569 100644
--- a/Makefile
+++ b/Makefile
@@ -216,6 +216,8 @@ BR_GRAPH_OUT := $(or $(BR2_GRAPH_OUT),pdf)
 BUILD_DIR := $(BASE_DIR)/build
 BINARIES_DIR := $(BASE_DIR)/images
 TARGET_DIR := $(BASE_DIR)/target
+PER_PACKAGE_DIR := $(BASE_DIR)/per-package
+
 # initial definition so that 'make clean' works for most users, even without
 # .config. HOST_DIR will be overwritten later when .config is included.
 HOST_DIR := $(BASE_DIR)/host
@@ -231,7 +233,7 @@ LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
 LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
 LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
 
-export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
+export LD_LIBRARY_PATH = $(if $(PKG),$($(PKG)_HOST_DIR)/lib)
 
 ################################################################################
 #
@@ -239,7 +241,7 @@ export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
 # dependencies anywhere else
 #
 ################################################################################
-$(BUILD_DIR) $(TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
+$(BUILD_DIR) $(TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):
 	@mkdir -p $@
 
 BR2_CONFIG = $(CONFIG_DIR)/.config
@@ -675,10 +677,19 @@ endef
 TARGET_FINALIZE_HOOKS += PURGE_LOCALES
 endif
 
+ALL_PACKAGES_TARGET_DIRS = $(foreach pkg,$(PACKAGES),$($(call UPPERCASE,$(pkg))_TARGET_DIR))
+ALL_PACKAGES_HOST_DIRS   = $(foreach pkg,$(PACKAGES),$($(call UPPERCASE,$(pkg))_HOST_DIR))
+
 $(TARGETS_ROOTFS): target-finalize
 
 .PHONY: target-finalize
 target-finalize: $(PACKAGES)
+	@$(call MESSAGE,"Creating global target directory")
+	$(foreach dir,$(ALL_PACKAGES_TARGET_DIRS),\
+		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))
+	@$(call MESSAGE,"Creating global host directory")
+	$(foreach dir,$(ALL_PACKAGES_HOST_DIRS),\
+		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))
 	@$(call MESSAGE,"Finalizing target directory")
 	$(TOPDIR)/support/scripts/fix-rpath host
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
@@ -972,7 +983,7 @@ printvars:
 clean:
 	rm -rf $(TARGET_DIR) $(BINARIES_DIR) $(HOST_DIR) \
 		$(BUILD_DIR) $(BASE_DIR)/staging \
-		$(LEGAL_INFO_DIR) $(GRAPHS_DIR)
+		$(LEGAL_INFO_DIR) $(GRAPHS_DIR) $(PER_PACKAGE_DIR)
 
 .PHONY: distclean
 distclean: clean
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 82f8c06821..aee4011f63 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
 $(BUILD_DIR)/%/.stamp_extracted:
 	@$(call step_start,extract)
 	@$(call MESSAGE,"Extracting")
+	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)
 	$(foreach hook,$($(PKG)_PRE_EXTRACT_HOOKS),$(call $(hook))$(sep))
 	$(Q)mkdir -p $(@D)
 	$($(PKG)_EXTRACT_CMDS)
@@ -215,6 +216,10 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
 $(BUILD_DIR)/%/.stamp_configured:
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
+	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_HOST_DIRS),\
+		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))
+	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_TARGET_DIRS),\
+		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
@@ -409,6 +414,10 @@ $(2)_NAME			=  $(1)
 $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
 $(2)_PKGDIR			=  $(pkgdir)
 
+$(2)_TARGET_DIR		= $$(PER_PACKAGE_DIR)/$(1)/target/
+$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
+$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)
+
 # Keep the package version that may contain forward slashes in the _DL_VERSION
 # variable, then replace all forward slashes ('/') by underscores ('_') to
 # sanitize the package version that is used in paths, directory and file names.
@@ -561,6 +570,13 @@ $(2)_FINAL_DEPENDENCIES = $$(sort $$($(2)_DEPENDENCIES))
 $(2)_FINAL_PATCH_DEPENDENCIES = $$(sort $$($(2)_PATCH_DEPENDENCIES))
 $(2)_FINAL_ALL_DEPENDENCIES = $$(sort $$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES))
 
+$(2)_FINAL_DEPENDENCIES_HOST_DIRS = \
+	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
+		$$($$(call UPPERCASE,$$(dep))_HOST_DIR))
+$(2)_FINAL_DEPENDENCIES_TARGET_DIRS = \
+	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
+		$$($$(call UPPERCASE,$$(dep))_TARGET_DIR))
+
 $(2)_INSTALL_STAGING		?= NO
 $(2)_INSTALL_IMAGES		?= NO
 $(2)_INSTALL_TARGET		?= YES
@@ -789,17 +805,38 @@ $(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
 # define the PKG variable for all targets, containing the
 # uppercase package variable prefix
 $$($(2)_TARGET_INSTALL_TARGET):		PKG=$(2)
+$$($(2)_TARGET_INSTALL_TARGET):		HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_INSTALL_TARGET):		STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_INSTALL_TARGET):		TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_INSTALL_STAGING):	PKG=$(2)
+$$($(2)_TARGET_INSTALL_STAGING):	HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_INSTALL_STAGING):	STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_INSTALL_STAGING):	TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
+$$($(2)_TARGET_INSTALL_IMAGES):		HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_INSTALL_IMAGES):		STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_INSTALL_IMAGES):		TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_INSTALL_HOST):		PKG=$(2)
+$$($(2)_TARGET_INSTALL_HOST):		HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_INSTALL_HOST):		STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_INSTALL_HOST):		TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
+$$($(2)_TARGET_BUILD):			HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_BUILD):			STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_BUILD):			TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_CONFIGURE):		STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_CONFIGURE):		TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_RSYNC):			SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			RAWNAME=$$(patsubst host-%,%,$(1))
 $$($(2)_TARGET_PATCH):			PKGDIR=$(pkgdir)
 $$($(2)_TARGET_EXTRACT):		PKG=$(2)
+$$($(2)_TARGET_EXTRACT):		HOST_DIR=$$($(2)_HOST_DIR)
+$$($(2)_TARGET_EXTRACT):		STAGING_DIR=$$($(2)_STAGING_DIR)
+$$($(2)_TARGET_EXTRACT):		TARGET_DIR=$$($(2)_TARGET_DIR)
 $$($(2)_TARGET_SOURCE):			PKG=$(2)
 $$($(2)_TARGET_SOURCE):			PKGDIR=$(pkgdir)
 $$($(2)_TARGET_ACTUAL_SOURCE):		PKG=$(2)
-- 
2.13.6

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Arnout Vandecappelle]

On 03-11-17 17:06, Thomas Petazzoni wrote:
> This commit implemnts the core of the move to per-package SDK and
> target directories. The main idea is that instead of having a global
> output/host and output/target in which all packages install files, we
> switch to per-package host and target folders, that only contain their
> explicit dependencies.
> 
> There are two main benefits:
> 
>  - Packages will no longer discover dependencies that they do not
>    explicitly indicate in their <pkg>_DEPENDENCIES variable.
> 
>  - We can support top-level parallel build properly, because a package
>    only "sees" its own host directory and target directory, isolated
>    from the build of other packages that can happen in parallel.
> 
> It works as follows:
> 
>  - A new output/per-package/ folder is created, which will contain one
>    sub-folder per package, and inside it, a "host" folder and a
>    "target" folder:
> 
>    output/per-package/busybox/target
>    output/per-package/busybox/host
>    output/per-package/host-fakeroot/target
>    output/per-package/host-fakeroot/host
> 
>    This output/per-package/ folder is PER_PACKAGE_DIR, and each
>    package defines <pkg>_TARGET_DIR, <pkg>_HOST_DIR and
>    <pkg>_STAGING_DIR pointing to its own target, host and staging
>    directories.
> 
>  - The <pkg>_TARGET_DIR, <pkg>_HOST_DIR and <pkg>_STAGING_DIR variable
>    are passed as TARGET_DIR, HOST_DIR and STAGING_DIR to the various
>    make targets used for the different build steps of the
>    packages. Effectively this means that when a package references
>    $(HOST_DIR), the value is in fact $(<pkg>_HOST_DIR).
> 
>  - Of course, packages have dependencies, so those dependencies must
>    be installed in the per-package host and target folders before
>    configuring the package. To do so, we simply rsync (using hard
>    links to save space and time) the host and target folders of the
>    direct dependencies of the package to the current package host and
>    target folders. We only need to take care of direct dependencies
>    (and not recursively all dependencies), because we accumulate into
>    those per-package host and target folders the files installed by
>    the dependencies.
> 
>  - We fix LD_LIBRARY_PATH to make it point to the current package host
>    directory.
> 
> This is basically enough to make per-package SDK and target work. The
> only gotcha is that at the end of the build, output/target and
> output/host are empty, which means that:
> 
>  - The filesystem image creation code cannot work.
> 
>  - We don't have a SDK to build code outside of Buildroot.
> 
> In order to fix this, this commit extends the target-finalize step so
> that it starts by populating output/target and output/host by
> rsync-ing into them the target and host directories of all packages
> listed in the $(PACKAGES) variable.

 It would have been good to mention somewhere in the commit log the problem of 2
packages writing the same file...

 Also, is there a good reason not to do the rsync at the end of package
installation? I don't know how robust rsync is against race conditions in
directory or hardlink creation... OK, so I did a test and it races pretty badly
:-) So, could you add this to the commit log? Like:

It is necessary to do this sequentially in the target-finalize step and not in
each package. Doing it in package installation means that it can be done in
parallel. In that case, there is a chance that two rsyncs are creating the same
hardlink or directory at the same time, which makes one of them fail.

> 
> [Note: in fact, output/host is not completely empty, even though they
> should be. It contains the following files:
> output/host/
> output/host/lib64
> output/host/usr
> output/host/share
> output/host/share/buildroot
> output/host/share/buildroot/Platform
> output/host/share/buildroot/Platform/Buildroot.cmake
> output/host/share/buildroot/toolchainfile.cmake
> output/host/lib
> Perhaps those files should be installed by some package instead,
> perhaps the 'toolchain' package.]

 If we go that way: perhaps the .cmake files should be installed by some package
like 'host-cmake-infra' that is added to the dependencies from cmake-package.

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  Makefile               | 17 ++++++++++++++---
>  package/pkg-generic.mk | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5496273329..4e1ba62569 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -216,6 +216,8 @@ BR_GRAPH_OUT := $(or $(BR2_GRAPH_OUT),pdf)
>  BUILD_DIR := $(BASE_DIR)/build
>  BINARIES_DIR := $(BASE_DIR)/images
>  TARGET_DIR := $(BASE_DIR)/target
> +PER_PACKAGE_DIR := $(BASE_DIR)/per-package
> +
>  # initial definition so that 'make clean' works for most users, even without
>  # .config. HOST_DIR will be overwritten later when .config is included.
>  HOST_DIR := $(BASE_DIR)/host
> @@ -231,7 +233,7 @@ LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
>  LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
>  LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
>  
> -export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
> +export LD_LIBRARY_PATH = $(if $(PKG),$($(PKG)_HOST_DIR)/lib)
>  
>  ################################################################################
>  #
> @@ -239,7 +241,7 @@ export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
>  # dependencies anywhere else
>  #
>  ################################################################################
> -$(BUILD_DIR) $(TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
> +$(BUILD_DIR) $(TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):
>  	@mkdir -p $@
>  
>  BR2_CONFIG = $(CONFIG_DIR)/.config
> @@ -675,10 +677,19 @@ endef
>  TARGET_FINALIZE_HOOKS += PURGE_LOCALES
>  endif
>  
> +ALL_PACKAGES_TARGET_DIRS = $(foreach pkg,$(PACKAGES),$($(call UPPERCASE,$(pkg))_TARGET_DIR))
> +ALL_PACKAGES_HOST_DIRS   = $(foreach pkg,$(PACKAGES),$($(call UPPERCASE,$(pkg))_HOST_DIR))
> +
>  $(TARGETS_ROOTFS): target-finalize
>  
>  .PHONY: target-finalize
>  target-finalize: $(PACKAGES)
> +	@$(call MESSAGE,"Creating global target directory")
> +	$(foreach dir,$(ALL_PACKAGES_TARGET_DIRS),\
> +		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))

 Why -u? Since we're not actually copying anything, there is no reason to not
overwrite files that already exist, right?

 Also, shouldn't there be a / behind $(dir)? Otherwise, if TARGET_DIR exists
already, rsync will create $(dir) as a subdirectory of $(TARGET_DIR), no?


> +	@$(call MESSAGE,"Creating global host directory")
> +	$(foreach dir,$(ALL_PACKAGES_HOST_DIRS),\
> +		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))
>  	@$(call MESSAGE,"Finalizing target directory")
>  	$(TOPDIR)/support/scripts/fix-rpath host
>  	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
> @@ -972,7 +983,7 @@ printvars:
>  clean:
>  	rm -rf $(TARGET_DIR) $(BINARIES_DIR) $(HOST_DIR) \
>  		$(BUILD_DIR) $(BASE_DIR)/staging \
> -		$(LEGAL_INFO_DIR) $(GRAPHS_DIR)
> +		$(LEGAL_INFO_DIR) $(GRAPHS_DIR) $(PER_PACKAGE_DIR)
>  
>  .PHONY: distclean
>  distclean: clean
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 82f8c06821..aee4011f63 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
>  $(BUILD_DIR)/%/.stamp_extracted:
>  	@$(call step_start,extract)
>  	@$(call MESSAGE,"Extracting")
> +	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)

 Why do you create them here? It would make more sense to do it just before the
rsync, no?

 Given the possible host-tar and host-lzip dependencies, the rsync should be
done here already, not in the configure step. Ouch, that's not good, because the
dependencies are only evaluated for the configure step... There are
PATCH_DEPENDENCIES, but those are only before patch. Well, I guess that's a
reason to keep host-tar and host-lzip as DEPENDENCIES_HOST_PREREQ :-)

>  	$(foreach hook,$($(PKG)_PRE_EXTRACT_HOOKS),$(call $(hook))$(sep))
>  	$(Q)mkdir -p $(@D)
>  	$($(PKG)_EXTRACT_CMDS)
> @@ -215,6 +216,10 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>  $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call step_start,configure)
>  	@$(call MESSAGE,"Configuring")
> +	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_HOST_DIRS),\
> +		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))

 Shouldn't there be a $Q somewhere?

> +	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_TARGET_DIRS),\
> +		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
>  	$($(PKG)_CONFIGURE_CMDS)
>  	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
> @@ -409,6 +414,10 @@ $(2)_NAME			=  $(1)
>  $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
>  $(2)_PKGDIR			=  $(pkgdir)
>  
> +$(2)_TARGET_DIR		= $$(PER_PACKAGE_DIR)/$(1)/target/

 Ah, here is the trailing slash, that explains why it worked. I don't really
like it when you have to rely on a trailing slash in the variable definition...

> +$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
> +$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)

 I hate adding more per-package variables. Can't you use the expanded values,
substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
definition of TARGET_DIR etc:

TARGET_DIR = $(if
$(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)

 That way you also don't need to pass those variables to each individual stamp rule.

> +
>  # Keep the package version that may contain forward slashes in the _DL_VERSION
>  # variable, then replace all forward slashes ('/') by underscores ('_') to
>  # sanitize the package version that is used in paths, directory and file names.
> @@ -561,6 +570,13 @@ $(2)_FINAL_DEPENDENCIES = $$(sort $$($(2)_DEPENDENCIES))
>  $(2)_FINAL_PATCH_DEPENDENCIES = $$(sort $$($(2)_PATCH_DEPENDENCIES))
>  $(2)_FINAL_ALL_DEPENDENCIES = $$(sort $$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES))
>  
> +$(2)_FINAL_DEPENDENCIES_HOST_DIRS = \
> +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> +		$$($$(call UPPERCASE,$$(dep))_HOST_DIR))
> +$(2)_FINAL_DEPENDENCIES_TARGET_DIRS = \
> +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> +		$$($$(call UPPERCASE,$$(dep))_TARGET_DIR))

 I may be overengineering things, but perhaps things can be simplified as
follows (with the definition of TARGET_DIR etc as above):

define RSYNC_HOST_DIRS
	$(foreach pkg,$(1),\
		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/host;\
		rsync -au --link-dest=$$dir $$dir/ $(HOST_DIR)$(sep))
endef

define RSYNC_TARGET_DIRS
	$(foreach pkg,$(1),\
		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/target;\
		rsync -au --link-dest=$$dir $$dir/ $(TARGET_DIR)$(sep))
endef

(as always, completely untested). These macros can be used both in the config
stamp rule and in the finalize rule. I even considered leaving out the argument
and instead defaulting $(1) to
$(if $(PKG),$($(PKG)_FINAL_DEPENDENCIES),$(PACKAGES))
but that probably just makes things harder to understand.


 Otherwise looks good to me :-P

 Regards,
 Arnout

> +
>  $(2)_INSTALL_STAGING		?= NO
>  $(2)_INSTALL_IMAGES		?= NO
>  $(2)_INSTALL_TARGET		?= YES
> @@ -789,17 +805,38 @@ $(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
>  # define the PKG variable for all targets, containing the
>  # uppercase package variable prefix
>  $$($(2)_TARGET_INSTALL_TARGET):		PKG=$(2)
> +$$($(2)_TARGET_INSTALL_TARGET):		HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_INSTALL_TARGET):		STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_INSTALL_TARGET):		TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_INSTALL_STAGING):	PKG=$(2)
> +$$($(2)_TARGET_INSTALL_STAGING):	HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_INSTALL_STAGING):	STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_INSTALL_STAGING):	TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
> +$$($(2)_TARGET_INSTALL_IMAGES):		HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_INSTALL_IMAGES):		STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_INSTALL_IMAGES):		TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_INSTALL_HOST):		PKG=$(2)
> +$$($(2)_TARGET_INSTALL_HOST):		HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_INSTALL_HOST):		STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_INSTALL_HOST):		TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_BUILD):			PKG=$(2)
> +$$($(2)_TARGET_BUILD):			HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_BUILD):			STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_BUILD):			TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
> +$$($(2)_TARGET_CONFIGURE):		HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_CONFIGURE):		STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_CONFIGURE):		TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_RSYNC):			SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
>  $$($(2)_TARGET_RSYNC):			PKG=$(2)
>  $$($(2)_TARGET_PATCH):			PKG=$(2)
>  $$($(2)_TARGET_PATCH):			RAWNAME=$$(patsubst host-%,%,$(1))
>  $$($(2)_TARGET_PATCH):			PKGDIR=$(pkgdir)
>  $$($(2)_TARGET_EXTRACT):		PKG=$(2)
> +$$($(2)_TARGET_EXTRACT):		HOST_DIR=$$($(2)_HOST_DIR)
> +$$($(2)_TARGET_EXTRACT):		STAGING_DIR=$$($(2)_STAGING_DIR)
> +$$($(2)_TARGET_EXTRACT):		TARGET_DIR=$$($(2)_TARGET_DIR)
>  $$($(2)_TARGET_SOURCE):			PKG=$(2)
>  $$($(2)_TARGET_SOURCE):			PKGDIR=$(pkgdir)
>  $$($(2)_TARGET_ACTUAL_SOURCE):		PKG=$(2)
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni]

Hello,

Thanks a lot for the suggestions, I'll take them into account for the
next version. Just a few comments/questions below.

On Tue, 7 Nov 2017 23:50:52 +0100, Arnout Vandecappelle wrote:

> > In order to fix this, this commit extends the target-finalize step so
> > that it starts by populating output/target and output/host by
> > rsync-ing into them the target and host directories of all packages
> > listed in the $(PACKAGES) variable.  
> 
>  It would have been good to mention somewhere in the commit log the problem of 2
> packages writing the same file...

True. I believe the work Yann has done to detect packages overwriting
files installed by other packages is definitely a pre-requisite for
this work, so I was kind of assuming that the "packages don't overwrite
each other" would already be a rule by the time my per-package SDK
patches get merged. But I should make this explicit in the cover letter
at least.

>  Also, is there a good reason not to do the rsync at the end of package
> installation? I don't know how robust rsync is against race conditions in
> directory or hardlink creation... OK, so I did a test and it races pretty badly
> :-) So, could you add this to the commit log? Like:
> 
> It is necessary to do this sequentially in the target-finalize step and not in
> each package. Doing it in package installation means that it can be done in
> parallel. In that case, there is a chance that two rsyncs are creating the same
> hardlink or directory at the same time, which makes one of them fail.

I didn't think at all about doing this in parallel. To me, the creation
of the final/common HOST_DIR/TARGET_DIR/STAGING_DIR is not at
all something that needs to be done while the build is on-going. And I
could go even further: unless you call "make sdk", we technically don't
really need to create the final/common HOST_DIR/STAGING_DIR. Only a
TARGET_DIR is needed, and perhaps even we could avoid a global one, and
instead have a per-filesystem image one, in order to avoid the parallel
filesystem image creation problem.

> > [Note: in fact, output/host is not completely empty, even though they
> > should be. It contains the following files:
> > output/host/
> > output/host/lib64
> > output/host/usr
> > output/host/share
> > output/host/share/buildroot
> > output/host/share/buildroot/Platform
> > output/host/share/buildroot/Platform/Buildroot.cmake
> > output/host/share/buildroot/toolchainfile.cmake
> > output/host/lib
> > Perhaps those files should be installed by some package instead,
> > perhaps the 'toolchain' package.]  
> 
>  If we go that way: perhaps the .cmake files should be installed by some package
> like 'host-cmake-infra' that is added to the dependencies from cmake-package.

That was my thinking: perhaps those files should all be installed by
packages.

> >  .PHONY: target-finalize
> >  target-finalize: $(PACKAGES)
> > +	@$(call MESSAGE,"Creating global target directory")
> > +	$(foreach dir,$(ALL_PACKAGES_TARGET_DIRS),\
> > +		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))  
> 
>  Why -u? Since we're not actually copying anything, there is no reason to not
> overwrite files that already exist, right?

I just borrowed parts of Gustavo patches, including those rsync
invocations. Indeed -u is not needed.

>  Also, shouldn't there be a / behind $(dir)? Otherwise, if TARGET_DIR exists
> already, rsync will create $(dir) as a subdirectory of $(TARGET_DIR), no?

I'll fix.

> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 82f8c06821..aee4011f63 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
> >  $(BUILD_DIR)/%/.stamp_extracted:
> >  	@$(call step_start,extract)
> >  	@$(call MESSAGE,"Extracting")
> > +	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)  
> 
>  Why do you create them here? It would make more sense to do it just before the
> rsync, no?

I don't remember, I'll have to check again. I don't immediately see a
reason why they would be needed before the rsync.

>  Given the possible host-tar and host-lzip dependencies, the rsync should be
> done here already, not in the configure step. Ouch, that's not good, because the
> dependencies are only evaluated for the configure step... There are
> PATCH_DEPENDENCIES, but those are only before patch. Well, I guess that's a
> reason to keep host-tar and host-lzip as DEPENDENCIES_HOST_PREREQ :-)

These are all topics that I haven't looked at yet. So thanks for
pointing them out. I should probably collect a TODO list on per-package
SDK to not forget about all the things people have mentioned as things
to look at.

> > +	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_HOST_DIRS),\
> > +		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))  
> 
>  Shouldn't there be a $Q somewhere?

Yes. Seeing the rsync commands was useful for debugging, as you can
imagine :)


> > +$(2)_TARGET_DIR		= $$(PER_PACKAGE_DIR)/$(1)/target/  
> 
>  Ah, here is the trailing slash, that explains why it worked. I don't really
> like it when you have to rely on a trailing slash in the variable definition...

OK.

> 
> > +$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
> > +$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)  
> 
>  I hate adding more per-package variables. Can't you use the expanded values,
> substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
> definition of TARGET_DIR etc:
> 
> TARGET_DIR = $(if
> $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)
> 
>  That way you also don't need to pass those variables to each individual stamp rule.

I didn't think about this, sounds like a good idea, I'll try that.

However, I'm actually planning to drop entirely having TARGET_DIR
defined outside of package rules. What I want to have ultimately is:

 - TARGET_DIR points to the current package per-package target
   directory. If we're outside of a package rule, TARGET_DIR has no
   value (or possibly a bogus value such as /this/is/a/bogus/thing/).
   Indeed, we really don't want things to directly install/mess with
   the global/common target directory.

 - Have something like COMMON_TARGET_DIR that points to
   $(BASE_DIR)/target, and we carefully change the places that should
   access the COMMON_TARGET_DIR (i.e mainly target-finalize and some
   filesystem generation stuff).

> >  # Keep the package version that may contain forward slashes in the _DL_VERSION
> >  # variable, then replace all forward slashes ('/') by underscores ('_') to
> >  # sanitize the package version that is used in paths, directory and file names.
> > @@ -561,6 +570,13 @@ $(2)_FINAL_DEPENDENCIES = $$(sort $$($(2)_DEPENDENCIES))
> >  $(2)_FINAL_PATCH_DEPENDENCIES = $$(sort $$($(2)_PATCH_DEPENDENCIES))
> >  $(2)_FINAL_ALL_DEPENDENCIES = $$(sort $$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES))
> >  
> > +$(2)_FINAL_DEPENDENCIES_HOST_DIRS = \
> > +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> > +		$$($$(call UPPERCASE,$$(dep))_HOST_DIR))
> > +$(2)_FINAL_DEPENDENCIES_TARGET_DIRS = \
> > +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> > +		$$($$(call UPPERCASE,$$(dep))_TARGET_DIR))  
> 
>  I may be overengineering things, but perhaps things can be simplified as
> follows (with the definition of TARGET_DIR etc as above):
> 
> define RSYNC_HOST_DIRS
> 	$(foreach pkg,$(1),\
> 		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/host;\
> 		rsync -au --link-dest=$$dir $$dir/ $(HOST_DIR)$(sep))
> endef
> 
> define RSYNC_TARGET_DIRS
> 	$(foreach pkg,$(1),\
> 		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/target;\
> 		rsync -au --link-dest=$$dir $$dir/ $(TARGET_DIR)$(sep))
> endef
> 
> (as always, completely untested). These macros can be used both in the config
> stamp rule and in the finalize rule. I even considered leaving out the argument
> and instead defaulting $(1) to
> $(if $(PKG),$($(PKG)_FINAL_DEPENDENCIES),$(PACKAGES))
> but that probably just makes things harder to understand.

I'll think about this suggestion.

Thanks again for the detailed review.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Arnout Vandecappelle]

On 08-11-17 00:11, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks a lot for the suggestions, I'll take them into account for the
> next version. Just a few comments/questions below.
> 
> On Tue, 7 Nov 2017 23:50:52 +0100, Arnout Vandecappelle wrote:

[snip]
>>  Also, is there a good reason not to do the rsync at the end of package
>> installation? I don't know how robust rsync is against race conditions in
>> directory or hardlink creation... OK, so I did a test and it races pretty badly
>> :-) So, could you add this to the commit log? Like:
>>
>> It is necessary to do this sequentially in the target-finalize step and not in
>> each package. Doing it in package installation means that it can be done in
>> parallel. In that case, there is a chance that two rsyncs are creating the same
>> hardlink or directory at the same time, which makes one of them fail.
> 
> I didn't think at all about doing this in parallel. 

 The point is not that you may want to do it in parallel. The point is that it
is natural to do something that needs to be done for each package at the time
you build that package. Doing stuff in finalize is always a bit weird.

> To me, the creation
> of the final/common HOST_DIR/TARGET_DIR/STAGING_DIR is not at
> all something that needs to be done while the build is on-going. And I
> could go even further: unless you call "make sdk", we technically don't
> really need to create the final/common HOST_DIR/STAGING_DIR.

 Absolutely true, it would make sense to do that in 'make sdk'.

> Only a
> TARGET_DIR is needed, and perhaps even we could avoid a global one, and
> instead have a per-filesystem image one, in order to avoid the parallel
> filesystem image creation problem.

 That's a great idea!

[snip]
>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>> index 82f8c06821..aee4011f63 100644
>>> --- a/package/pkg-generic.mk
>>> +++ b/package/pkg-generic.mk
>>> @@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
>>>  $(BUILD_DIR)/%/.stamp_extracted:
>>>  	@$(call step_start,extract)
>>>  	@$(call MESSAGE,"Extracting")
>>> +	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)  
>>
>>  Why do you create them here? It would make more sense to do it just before the
>> rsync, no?
> 
> I don't remember, I'll have to check again. I don't immediately see a
> reason why they would be needed before the rsync.

 A package without any dependencies will not do the rsync, and I do think you
want the directories to exist. Or maybe not, I'm not sure... Actually, shouldn't
a skeleton be installed there as well?


>>  Given the possible host-tar and host-lzip dependencies, the rsync should be
>> done here already, not in the configure step. Ouch, that's not good, because the
>> dependencies are only evaluated for the configure step... There are
>> PATCH_DEPENDENCIES, but those are only before patch. Well, I guess that's a
>> reason to keep host-tar and host-lzip as DEPENDENCIES_HOST_PREREQ :-)
> 
> These are all topics that I haven't looked at yet. So thanks for
> pointing them out. I should probably collect a TODO list on per-package
> SDK to not forget about all the things people have mentioned as things
> to look at.

 Yeah, a skeleton HOST_DIR that every package depends on would simplify that,
perhaps.

[snip]
>>  I hate adding more per-package variables. Can't you use the expanded values,
>> substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
>> definition of TARGET_DIR etc:
>>
>> TARGET_DIR = $(if
>> $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)
>>
>>  That way you also don't need to pass those variables to each individual stamp rule.
> 
> I didn't think about this, sounds like a good idea, I'll try that.
> 
> However, I'm actually planning to drop entirely having TARGET_DIR
> defined outside of package rules. What I want to have ultimately is:
> 
>  - TARGET_DIR points to the current package per-package target
>    directory. If we're outside of a package rule, TARGET_DIR has no
>    value (or possibly a bogus value such as /this/is/a/bogus/thing/).
>    Indeed, we really don't want things to directly install/mess with
>    the global/common target directory.

 bogus thing, good plan!

> 
>  - Have something like COMMON_TARGET_DIR that points to
>    $(BASE_DIR)/target, and we carefully change the places that should
>    access the COMMON_TARGET_DIR (i.e mainly target-finalize and some
>    filesystem generation stuff).

 Ack yes, sounds good.


 Regards,
 Arnout


[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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni]

Hello,

On Wed, 8 Nov 2017 00:57:40 +0100, Arnout Vandecappelle wrote:

> > To me, the creation
> > of the final/common HOST_DIR/TARGET_DIR/STAGING_DIR is not at
> > all something that needs to be done while the build is on-going. And I
> > could go even further: unless you call "make sdk", we technically don't
> > really need to create the final/common HOST_DIR/STAGING_DIR.  
> 
>  Absolutely true, it would make sense to do that in 'make sdk'.
> 
> > Only a
> > TARGET_DIR is needed, and perhaps even we could avoid a global one, and
> > instead have a per-filesystem image one, in order to avoid the parallel
> > filesystem image creation problem.  
> 
>  That's a great idea!

For those two things, I believe there is one reason to keep creating
the global HOST_DIR and TARGET_DIR in target-finalize: backward
compatibility with user habits. It is going to be really weird for a
lot of our users if output/host and output/target are empty at the end
of the build.

There is already something really weird going on with my changes: if
you run "make foobar" but BR2_PACKAGE_FOOBAR is not enabled, then
foobar is built, installed in its per-package directory, but because it
is not enabled at the Kconfig level, this package is not in PACKAGES,
so it is not copied to the global TARGET_DIR. So you don't have foobar
installed in output/target, nor in your final root filesystem image!

> >>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> >>> index 82f8c06821..aee4011f63 100644
> >>> --- a/package/pkg-generic.mk
> >>> +++ b/package/pkg-generic.mk
> >>> @@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
> >>>  $(BUILD_DIR)/%/.stamp_extracted:
> >>>  	@$(call step_start,extract)
> >>>  	@$(call MESSAGE,"Extracting")
> >>> +	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)    
> >>
> >>  Why do you create them here? It would make more sense to do it just before the
> >> rsync, no?  
> > 
> > I don't remember, I'll have to check again. I don't immediately see a
> > reason why they would be needed before the rsync.  

Following my proposal in my previous e-mail to have
EXTRACT_DEPENDENCIES rsync'ed into the per-package host/target at
extract time, it makes sense to create those folders at the extract
step :)

> 
>  A package without any dependencies will not do the rsync, and I do think you
> want the directories to exist. Or maybe not, I'm not sure... Actually, shouldn't
> a skeleton be installed there as well?

For the target directory, there is no problem: all target packages
depend on the skeleton package, so there is always a skeleton in the
per-package target directory.

> >>  Given the possible host-tar and host-lzip dependencies, the rsync should be
> >> done here already, not in the configure step. Ouch, that's not good, because the
> >> dependencies are only evaluated for the configure step... There are
> >> PATCH_DEPENDENCIES, but those are only before patch. Well, I guess that's a
> >> reason to keep host-tar and host-lzip as DEPENDENCIES_HOST_PREREQ :-)  
> > 
> > These are all topics that I haven't looked at yet. So thanks for
> > pointing them out. I should probably collect a TODO list on per-package
> > SDK to not forget about all the things people have mentioned as things
> > to look at.  
> 
>  Yeah, a skeleton HOST_DIR that every package depends on would simplify that,
> perhaps.

The skeleton HOST_DIR is just:

$(HOST_DIR)/usr: $(HOST_DIR)
        @ln -snf . $@

$(HOST_DIR)/lib: $(HOST_DIR)
        @mkdir -p $@
        @case $(HOSTARCH) in \
                (*64) ln -snf lib $(@D)/lib64;; \
                (*)   ln -snf lib $(@D)/lib32;; \
        esac

But yes, perhaps that could be clarified as a host-skeleton package ?

I'll have to double check how this works with my current proposed
implementation.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni]

Hello,

A few more answers/topics I wanted to discuss.

On Tue, 7 Nov 2017 23:50:52 +0100, Arnout Vandecappelle wrote:

>  Given the possible host-tar and host-lzip dependencies, the rsync
> should be done here already, not in the configure step. Ouch, that's
> not good, because the dependencies are only evaluated for the
> configure step... There are PATCH_DEPENDENCIES, but those are only
> before patch. Well, I guess that's a reason to keep host-tar and
> host-lzip as DEPENDENCIES_HOST_PREREQ :-)

Here is how I'm thinking of solving the problem:

 - Next to <pkg>_PATCH_DEPENDENCIES, introduce the concept of
   <pkg>_EXTRACT_DEPENDENCIES.

 - If there is no suitable tar in the system, then host-tar would be
   added to <pkg>_EXTRACT_DEPENDENCIES of all packages, except host-tar
   itself.

 - If the package needs lzip and there is no lzip available on the
   system, host-lzip is added to <pkg>_EXTRACT_DEPENDENCIES.

 - If ccache support is enabled, host-ccache is added to
   <pkg>_DEPENDENCIES of all packages, except host-tar, which is built
   with HOSTCC_NOCCACHE.

 - At the beginning of the extract step of a package, we rsync to its
   per-package target/host directories the per-package target/host dirs
   of the packages listed in its <pkg>_EXTRACT_DEPENDENCIES variable.

 - At the beginning of the patch step of a package, we rsync to its
   per-package target/host directories the per-package target/host dirs
   of the packages listed in its <pkg>_PATCH_DEPENDENCIES variable.

Thoughts?

> > +$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
> > +$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)  
> 
>  I hate adding more per-package variables. Can't you use the expanded values,
> substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
> definition of TARGET_DIR etc:
> 
> TARGET_DIR = $(if
> $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)
> 
>  That way you also don't need to pass those variables to each individual stamp rule.

I've changed this in my new iteration. The only drawback is that since
we no longer have those shortcut variables, the foreach loops building
the per-package directories, and building the final global directories
are a bit less readable:

Per-package host/target creation:

	$(foreach pkg,$($(PKG)_FINAL_DEPENDENCIES),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
		$(HOST_DIR)$(sep))
	$(foreach pkg,$($(PKG)_FINAL_DEPENDENCIES),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
		$(TARGET_DIR)$(sep))

Global host/target creation in target-finalize:

	$(foreach pkg,$(PACKAGES),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
		$(TARGET_DIR)$(sep))
	@$(call MESSAGE,"Creating global host directory")
	$(foreach pkg,$(PACKAGES),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
		$(HOST_DIR)$(sep))

But I guess that's OK. If we find that too weird, I can always
introduce some some make helpers:

per-package-target-dir = $(PER_PACKAGE_DIR)/$(1)/target
per-package-host-dir = $(PER_PACKAGE_DIR)/$(1)/host

But:

  $(call per-package-target-dir,$(pkg))

is not really shorter than

  $(PER_PACKAGE_DIR)/$(1)/target

It is actually longer :-)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Arnout Vandecappelle]

On 24-11-17 15:43, Thomas Petazzoni wrote:
> Hello,
> 
> A few more answers/topics I wanted to discuss.
> 
> On Tue, 7 Nov 2017 23:50:52 +0100, Arnout Vandecappelle wrote:
> 
>>  Given the possible host-tar and host-lzip dependencies, the rsync
>> should be done here already, not in the configure step. Ouch, that's
>> not good, because the dependencies are only evaluated for the
>> configure step... There are PATCH_DEPENDENCIES, but those are only
>> before patch. Well, I guess that's a reason to keep host-tar and
>> host-lzip as DEPENDENCIES_HOST_PREREQ :-)
> 
> Here is how I'm thinking of solving the problem:
> 
>  - Next to <pkg>_PATCH_DEPENDENCIES, introduce the concept of
>    <pkg>_EXTRACT_DEPENDENCIES.

 _PATCH_DEPENDENCIES is different: it introduces a dependency between foo-patch
and bar-patch. Here you want a dependency between foo-extract and tar.

> 
>  - If there is no suitable tar in the system, then host-tar would be
>    added to <pkg>_EXTRACT_DEPENDENCIES of all packages, except host-tar
>    itself.

 So you need an additional NO_EXTRACT_DEPENDENCIES or something to exclude it
for tar...


>  - If the package needs lzip and there is no lzip available on the
>    system, host-lzip is added to <pkg>_EXTRACT_DEPENDENCIES.
> 
>  - If ccache support is enabled, host-ccache is added to
>    <pkg>_DEPENDENCIES of all packages, except host-tar, which is built
>    with HOSTCC_NOCCACHE.
> 
>  - At the beginning of the extract step of a package, we rsync to its
>    per-package target/host directories the per-package target/host dirs
>    of the packages listed in its <pkg>_EXTRACT_DEPENDENCIES variable.
> 
>  - At the beginning of the patch step of a package, we rsync to its
>    per-package target/host directories the per-package target/host dirs
>    of the packages listed in its <pkg>_PATCH_DEPENDENCIES variable.

 So this bit doesn't make sense: _PATCH_DEPENDENCIES are not (necessarily) built
before foo-patch. So to be reproducible, it should *not* be rsynced yet.

> 
> Thoughts?

 I'm not at all happy with this approach. It adds generic-package stuff that is
used for only one package, and it spreads the logic out over different places.

 I'd rather make the logic explicit in dependencies.mk. Something like

ifneq ($(filter host-tar,$(DEPENDENCIES_HOST_PREREQ)),)
$(filter-out host-tar,$(DEPENDENCIES_HOST_PREREQ)): host-tar
endif

ifneq ($(filter host-ccache,$(DEPENDENCIES_HOST_PREREQ)),)
$(filter-out host-tar host-ccache,$(DEPENDENCIES_HOST_PREREQ)): host-ccache
endif

etc. You'll probably want to introduce a variable to collect the filter-out
things, though.


 Regards,
 Arnout


> 
>>> +$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
>>> +$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)  
>>
>>  I hate adding more per-package variables. Can't you use the expanded values,
>> substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
>> definition of TARGET_DIR etc:
>>
>> TARGET_DIR = $(if
>> $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)
>>
>>  That way you also don't need to pass those variables to each individual stamp rule.
> 
> I've changed this in my new iteration. The only drawback is that since
> we no longer have those shortcut variables, the foreach loops building
> the per-package directories, and building the final global directories
> are a bit less readable:
> 
> Per-package host/target creation:
> 
> 	$(foreach pkg,$($(PKG)_FINAL_DEPENDENCIES),\
> 		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> 		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> 		$(HOST_DIR)$(sep))
> 	$(foreach pkg,$($(PKG)_FINAL_DEPENDENCIES),\
> 		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> 		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> 		$(TARGET_DIR)$(sep))
> 
> Global host/target creation in target-finalize:
> 
> 	$(foreach pkg,$(PACKAGES),\
> 		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> 		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> 		$(TARGET_DIR)$(sep))
> 	@$(call MESSAGE,"Creating global host directory")
> 	$(foreach pkg,$(PACKAGES),\
> 		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> 		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> 		$(HOST_DIR)$(sep))
> 
> But I guess that's OK. If we find that too weird, I can always
> introduce some some make helpers:
> 
> per-package-target-dir = $(PER_PACKAGE_DIR)/$(1)/target
> per-package-host-dir = $(PER_PACKAGE_DIR)/$(1)/host
> 
> But:
> 
>   $(call per-package-target-dir,$(pkg))
> 
> is not really shorter than
> 
>   $(PER_PACKAGE_DIR)/$(1)/target
> 
> It is actually longer :-)
> 
> Thomas
> 

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni]

Hello,

On Sat, 25 Nov 2017 18:30:26 +0100, Arnout Vandecappelle wrote:

> > Here is how I'm thinking of solving the problem:
> > 
> >  - Next to <pkg>_PATCH_DEPENDENCIES, introduce the concept of
> >    <pkg>_EXTRACT_DEPENDENCIES.  
> 
>  _PATCH_DEPENDENCIES is different: it introduces a dependency between foo-patch
> and bar-patch. Here you want a dependency between foo-extract and tar.

True.

>  So this bit doesn't make sense: _PATCH_DEPENDENCIES are not (necessarily) built
> before foo-patch. So to be reproducible, it should *not* be rsynced yet.
> 
> > 
> > Thoughts?  
> 
>  I'm not at all happy with this approach. It adds generic-package stuff that is
> used for only one package, and it spreads the logic out over different places.
> 
>  I'd rather make the logic explicit in dependencies.mk. Something like
> 
> ifneq ($(filter host-tar,$(DEPENDENCIES_HOST_PREREQ)),)
> $(filter-out host-tar,$(DEPENDENCIES_HOST_PREREQ)): host-tar
> endif
> 
> ifneq ($(filter host-ccache,$(DEPENDENCIES_HOST_PREREQ)),)
> $(filter-out host-tar host-ccache,$(DEPENDENCIES_HOST_PREREQ)): host-ccache
> endif

I don't understand how this can work.

The big thing to remember is that when I'm building a package, it only
sees in its per-package host/staging directory the dependencies
explicitly listed in this package <pkg>_DEPENDENCIES variable.

With your approach, neither host-tar nor host-ccache are listed in any
package <pkg>_DEPENDENCIES variable, so the per-package host directory
of host-ccache and host-tar will never be rsync'ed into the per-package
host directories of other packages.

Due to this, the package infrastructure _will_ have to know about the
fact that all packages depend on host-tar/host-ccache, for the simple
reason that we need to know that we have to rsync host-tar/host-ccache
when populating the per-package host directory in the configure step.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Yann E. MORIN]

Thomas, Arnout, All,

On 2017-11-25 21:01 +0100, Thomas Petazzoni spake thusly:
> On Sat, 25 Nov 2017 18:30:26 +0100, Arnout Vandecappelle wrote:
> 
> > > Here is how I'm thinking of solving the problem:
> > > 
> > >  - Next to <pkg>_PATCH_DEPENDENCIES, introduce the concept of
> > >    <pkg>_EXTRACT_DEPENDENCIES.  
> > 
> >  _PATCH_DEPENDENCIES is different: it introduces a dependency between foo-patch
> > and bar-patch. Here you want a dependency between foo-extract and tar.

And I think this is a sane approach. We already have such requirements
for tar, lzip and xz.

So we already have three cases where we might want to add an extract
dependency, which is imho sufficient to justify support in the infra.

Besides, having the dependencies managed from the infra will guarantee
the build ordering and the fact that they are built only when required.

> >  So this bit doesn't make sense: _PATCH_DEPENDENCIES are not (necessarily) built
> > before foo-patch. So to be reproducible, it should *not* be rsynced yet.
> > 
> > > 
> > > Thoughts?  
> > 
> >  I'm not at all happy with this approach. It adds generic-package stuff that is
> > used for only one package, and it spreads the logic out over different places.

Quite the opposite: it is used for at least three packages, and it
gathers all the dependencies under the aegis of the package infra.

> >  I'd rather make the logic explicit in dependencies.mk. Something like
> > 
> > ifneq ($(filter host-tar,$(DEPENDENCIES_HOST_PREREQ)),)
> > $(filter-out host-tar,$(DEPENDENCIES_HOST_PREREQ)): host-tar
> > endif
> > 
> > ifneq ($(filter host-ccache,$(DEPENDENCIES_HOST_PREREQ)),)
> > $(filter-out host-tar host-ccache,$(DEPENDENCIES_HOST_PREREQ)): host-ccache
> > endif
> 
> I don't understand how this can work.

TBH, I have a bit of an issue following it as well... :-/

> The big thing to remember is that when I'm building a package, it only
> sees in its per-package host/staging directory the dependencies
> explicitly listed in this package <pkg>_DEPENDENCIES variable.
> 
> With your approach, neither host-tar nor host-ccache are listed in any
> package <pkg>_DEPENDENCIES variable, so the per-package host directory
> of host-ccache and host-tar will never be rsync'ed into the per-package
> host directories of other packages.
> 
> Due to this, the package infrastructure _will_ have to know about the
> fact that all packages depend on host-tar/host-ccache, for the simple
> reason that we need to know that we have to rsync host-tar/host-ccache
> when populating the per-package host directory in the configure step.

I think we should strive at moving as much as possible to packages and
the package infra.

Regards,
Yann E. MORIN.

> 
> Best regards,
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [RFCv1 0/4] Per-package SDK and target directories

[Arnout Vandecappelle]

On 03-11-17 17:06, Thomas Petazzoni wrote:
[snip]
> What remains to be done?
> ========================
> 
>  - Completely get rid of the global HOST_DIR, TARGET_DIR and
>    STAGING_DIR definitions, so that nothing uses them, and they really
>    become per-package variables.

 I don't think that that's needed.


>  - While the toolchain sysroot, pkg-config paths and host RPATHs are
>    properly handled, there are quite certainly a lot of other places
>    that have absolute paths that won't work well with per-package SDK,
>    and that need to be adjusted.
> 
>  - Perhaps use a temporary per-package SDK and target directory when
>    building a package, and rename it to the final name once the
>    package has finished building. This would allow to detect
>    situations where the per-package SDK directory of package A
>    contains absolute paths referring to the per-package SDK directory
>    of package B. Such absolute paths are wrong, but we currently don't
>    see them because the per-package SDK directory for package B still
>    exists.


 Add an instrumentation hook:

	! grep -r $(PER_PACKAGE_DIR) $(HOST_DIR) $(TARGET_DIR)

That will immediately point to the culprit package. Maybe you want to avoid
grepping through gigabytes of data over and over again so perhaps use the same
approach as check-bin-arch.


 But actually,

> 
>  - Many other issues that I'm sure people will report.
> 
> Comparison with Gustavo's patch series
> ======================================
> 
> Gustavo Zacarias did a previous implementation of this, available in
> http://repo.or.cz/buildroot-gz.git/shortlog/refs/heads/pps3-next. Compared
> to Gustavo's patch series, this patch series:
> 

[snip]
>  - I haven't integrated Gustavo patches that move from $(shell ...) to
>    using backticks to delay the evaluation of
>    STAGING_DIR/HOST_DIR/TARGET_DIR:
> 
>    http://repo.or.cz/buildroot-gz.git/commitdiff/0c11c60865f1445830643bb404f92c20d563260c
>    http://repo.or.cz/buildroot-gz.git/commitdiff/207d2248ec8542293b6201b5c86f971021a3a591
>    http://repo.or.cz/buildroot-gz.git/commitdiff/7f6a69819fbb176e29a1c3a53b4a76efe87dad15
>    http://repo.or.cz/buildroot-gz.git/commitdiff/3328a9745eee555f5e5ef7df835afc26612ecd7b
>    http://repo.or.cz/buildroot-gz.git/commitdiff/45a9d8c2aa6f3c0d2d8ed989d843890025faa3e8

 Independently of the PPS feature, these are useful patches I think. Can you
submit them?


>  - I haven't looked at Qt specific issues, such as the ones fixed by
>    Gustavo in:
> 
>    http://repo.or.cz/buildroot-gz.git/commitdiff/643e982e635f4386ccefcda9da4b84536af84d04

 Yeah, and something similar for CMake as well...

 I'm a bit worried about python and friends. They may embed paths in all kinds
of weird places. Like sysconfigdata has paths to compilers and stuff.

 But now I think about it a little more: is there really a problem if e.g. a .la
file refers to a library in one of the other per-package dirs? The library does
exist there after all... The only thing we need to avoid is that a package
installs something in a different package's per-package dir. You may of course
end up with references to indirect dependencies, but they are still dependencies
that are tracked by Buildroot so they're OK.

 So how about this: at the end of the package build, we make the per-package dir
readonly. An evil package can of course still make it readwrite again before
installing something, but it's not very likely.

 Cleaning up all references to other per-package dirs is only really needed if
we want to make a tarball of the directory to save as a seed for a later build.
But we're not there yet by a long shot, so let's just ignore that use case.


 Regards,
 Arnout

> 
>  - I am not doing all the .la files, *-config, *.prl, etc. tweaks that
>    Gustavo is doing in:
> 
>    http://repo.or.cz/buildroot-gz.git/commitdiff/3f7b227b4c8e4514df6e5d54f88fcfb3a3a4bae0
> 
>    It is part of the "remaining absolute paths that need to be fixed"
>    item that I mentionned above.

-- 
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:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF