All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support
@ 2018-11-20 16:35 Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 1/9] Makefile: evaluate CCACHE and HOST{CC, CXX} at time of use Thomas Petazzoni
                   ` (8 more replies)
  0 siblings, 9 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

Hello,

Here is a fifth iteration of the per-package SDK and target directory
implementation.

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

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

However, if you want the full set of PPSH related patches, including
the package-specific fixes, and work on the Luarocks and Meson
infrastructure, you can pick up the following branch:

  http://git.bootlin.com/users/thomas-petazzoni/buildroot/log/?h=ppsh-v5-work

I.e the ppsh-v5 branch is more for reviewing, while ppsh-v5-work is
more for testing the whole PPSH functionality.

Changes since v5
================

 - Add patch "package/pkg-generic: adjust config scripts tweaks for
   per-package folders" that adjusts how pkg-generic.mk fixes *-config
   scripts, to use relative paths instead of absolute paths.

 - Add patch "package/pkg-generic: make libtool .la files compatible
   with per-package folders" that fixes libtool.la files for
   per-package folders.

 - Add patch "package/pkg-kconfig: handle KCONFIG_DEPENDENCIES with
   per-package folders" that properly handles KCONFIG_DEPENDENCIES in
   the pkg-kconfig infrastructure for per-package folders.

 - Added Acked-by from Yann E. Morin on the following patches:

   support/scripts/check-host-rpath: split condition on two statements
   Makefile: evaluate CCACHE and HOST{CC,CXX} at time of use

 - It is not included in this series, but since the v4 was sent, the
   case of the Luarocks and Meson based packages have been fixed, and
   I have sent separate series for these, because they can be merged
   separately. Many thanks to Fran?ois Perrad and Peter Seiderer for
   providing very useful insights on Luarocks and Meson.

Changes since v4
================

 - Dropped patches that have been merged upstream, mainly the "extract
   dependency" patches, but also a few other preparation patches.

 - Dropped the change of the RPATH handling. As discussed during
   previous Buildroot Developers meeting, it is not a big problem if
   during the build, the RPATH of a binary points to the library
   folder of another package per-package folder. Therefore, instead of
   fixing up RPATH at the end of each package installation, we keep
   what Buildroot does today: fix them at the very end of the build.

 - Made the support for per-package folders optional. Indeed, we
   realized there was no way to make sure it would be perfect from day
   1. Even if the principle works, there are lots of package-specific
   details to solve, and solving all of them before merging the base
   per-package folder support is not reasonable. So for now, an option
   BR2_PER_PACKAGE_FOLDERS is introduced, which defaults to off. When
   this option is enabled, the .NOTPARALLEL statement of the main
   Makefile goes away, which allows to do top-level parallel build.

 - Addition of a commit that reworks how the top-level directories are
   created.

 - Introduction of a prepare-per-package-folder function in
   pkg-generic.mk to encapsulate the logic to create the per-package
   folders from the dependencies of a package.

 - Rebased on top of the latest master.

Changes since v3
================

 - Dropped patches that have been merged upstream:
   pkgconf: use relative path to  STAGING_DIR instead of absolute path
   toolchain: post-pone evaluation of  TOOLCHAIN_EXTERNAL_BIN

 - For now, removed the most controversial/complicated patches
   (changing the rpath strategy, and switching to per-package
   host/target directories). The goal for now is to merge only the
   preparation patches.

Changes since v2
================

 - Solved the problem of all DEPENDENCIES_HOST_PREREQ targets:
   host-ccache, host-tar, host-lzip, host-xz, host-fakedate.

   To achieve this, introduced <pkg>_EXTRACT_DEPENDENCIES and used
   that to handle the dependencies on host-tar, host-lzip and host-xz.

   Used regular dependencies for host-ccache and host-fakedate.

   See below for more details.

Changes since v1
================

 - Rebased on the latest Buildroot next branch

 - The pkg-config changes are reworked according to Arnout's comments:
   only @STAGING_SUBDIR@ is replaced in pkg-config.in to generate
   pkg-config, the rest of the logic is internal to the script.

 - New patch to move the "host skeleton" logic into a proper
   host-skeleton package.

 - New patch to have the CMake related files generated as part of the
   toolchain package.

 - New patch to add a new "install" step that depends on the different
   install steps (target, staging, images, host). This is needed to
   later add some logic that is executed once all install steps of a
   package have been done.

 - Fix the approach to solve the RPATH logic: instead of using
   LD_LIBRARY_PATH, we actually fix with patchelf the RPATH of host
   binaries right after their installation.

 - Misc other improvements in the per-package SDK implementation.

What is this all about ?
========================

See the cover letter of v3 for details:
http://lists.busybox.net/pipermail/buildroot/2017-December/208384.html

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

Known issues:

 - The qt5 stack is entirely broken, because qmake hardcodes the
   installation path to be $(STAGING_DIR), so all qt5 packages install
   to the per-package $(STAGING_DIR) of qt5base.

   See
   http://lists.busybox.net/pipermail/buildroot/2018-November/235942.html

Best regards,

Thomas


Thomas Petazzoni (9):
  Makefile: evaluate CCACHE and HOST{CC,CXX} at time of use
  support/scripts/check-host-rpath: split condition on two statements
  Makefile: rework main directory creation logic
  Makefile: move .NOTPARALLEL statement after including .config file
  Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR
  package/pkg-generic: adjust config scripts tweaks for per-package
    folders
  core: implement per-package SDK and target
  package/pkg-generic: make libtool .la files compatible with
    per-package folders
  package/pkg-kconfig: handle KCONFIG_DEPENDENCIES with per-package
    folders

 Config.in                        | 18 ++++++
 Makefile                         | 94 ++++++++++++++++++++------------
 fs/common.mk                     |  3 +-
 package/pkg-generic.mk           | 41 ++++++++++++--
 package/pkg-kconfig.mk           |  1 +
 support/scripts/check-host-rpath |  8 ++-
 6 files changed, 122 insertions(+), 43 deletions(-)

-- 
2.19.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 1/9] Makefile: evaluate CCACHE and HOST{CC, CXX} at time of use
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 2/9] support/scripts/check-host-rpath: split condition on two statements Thomas Petazzoni
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

As we are going to move to per-package SDK, the location of CCACHE and
therefore the definitions of HOSTCC and HOSTCXX need to be evaluated
at the time of use and not at the time of assignment. Indeed, the
value of HOST_DIR changes from one package to the other.

Therefore, we need to change from := to =.

In addition, while doing A := $(something) $(A) is possible, doing A =
$(something) $(A) is not legal. So, instead of defining HOSTCC in
terms of the current HOSTCC variable, we re-use HOSTCC_NOCCACHE
instead.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 94adff3406..2819d44124 100644
--- a/Makefile
+++ b/Makefile
@@ -471,11 +471,11 @@ BR_PATH = "$(HOST_DIR)/bin:$(HOST_DIR)/sbin:$(PATH)"
 TARGET_DIR_WARNING_FILE = $(BASE_TARGET_DIR)/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
 
 ifeq ($(BR2_CCACHE),y)
-CCACHE := $(HOST_DIR)/bin/ccache
+CCACHE = $(HOST_DIR)/bin/ccache
 BR_CACHE_DIR ?= $(call qstrip,$(BR2_CCACHE_DIR))
 export BR_CACHE_DIR
-HOSTCC := $(CCACHE) $(HOSTCC)
-HOSTCXX := $(CCACHE) $(HOSTCXX)
+HOSTCC = $(CCACHE) $(HOSTCC_NOCCACHE)
+HOSTCXX = $(CCACHE) $(HOSTCXX_NOCCACHE)
 else
 export BR_NO_CCACHE
 endif
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 2/9] support/scripts/check-host-rpath: split condition on two statements
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 1/9] Makefile: evaluate CCACHE and HOST{CC, CXX} at time of use Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 3/9] Makefile: rework main directory creation logic Thomas Petazzoni
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

Inside the check_elf_has_rpath(), we check if the host binary has a
correct RPATH, which should be either an absolute path to
$(HOST_DIR)/lib, or a relative path using $ORIGIN. Those two
conditions are checked in a single statements, but as we are going to
add a third condition, let's split this up a bit:

 - If we have a RPATH to $(HOST_DIR)/lib -> we're good, return 0
 - If we have a RPATH to $ORIGIN/../lib -> we're good, return 0
 - Otherwise, we will exit the loop, and return 1

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/scripts/check-host-rpath | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
index 169628decb..6c5767da05 100755
--- a/support/scripts/check-host-rpath
+++ b/support/scripts/check-host-rpath
@@ -63,7 +63,8 @@ check_elf_has_rpath() {
         for dir in ${rpath//:/ }; do
             # Remove duplicate and trailing '/' for proper match
             dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
-            [ "${dir}" = "${hostdir}/lib" -o "${dir}" = "\$ORIGIN/../lib" ] && return 0
+            [ "${dir}" = "${hostdir}/lib" ] && return 0
+            [ "${dir}" = "\$ORIGIN/../lib" ] && return 0
         done
     done < <( readelf -d "${file}"                                              \
               |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 3/9] Makefile: rework main directory creation logic
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 1/9] Makefile: evaluate CCACHE and HOST{CC, CXX} at time of use Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 2/9] support/scripts/check-host-rpath: split condition on two statements Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file Thomas Petazzoni
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

In the current code, the creation of the main output folders
(BUILD_DIR, STAGING_DIR, HOST_DIR, TARGET_DIR, etc.) is done by a
global "dirs" target. While this works fine in the current situation,
it doesn't work well in a context where per-package host and target
directories are used.

For example, with the current code and per-package host directories,
the output/staging symbolic link ends up being created as a link to
the per-package package sysroot directory of the first package being
built, instead of the global sysroot.

This commit reworks the creation of those directories by having the
package/pkg-generic.mk code ensure that the build directory, target
directory, host directory, staging directory and binaries directory
exist before they are needed.

Two new targets, host-finalize and staging-finalize are added in the
main Makefile to create the compatibility symlinks for host and
staging directories. They will be extended later with additional logic
for per-package directories.

Thanks to those changes, the global "dirs" target is entirely removed.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Makefile               | 21 +++++++++------------
 fs/common.mk           |  1 +
 package/pkg-generic.mk |  7 ++++++-
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 2819d44124..e675ac26aa 100644
--- a/Makefile
+++ b/Makefile
@@ -572,10 +572,6 @@ $(foreach pkg,$(call UPPERCASE,$(PACKAGES)),\
 
 endif
 
-.PHONY: dirs
-dirs: $(BUILD_DIR) $(STAGING_DIR) $(BASE_TARGET_DIR) \
-	$(HOST_DIR) $(HOST_DIR_SYMLINK) $(BINARIES_DIR)
-
 $(BUILD_DIR)/buildroot-config/auto.conf: $(BR2_CONFIG)
 	$(MAKE1) $(EXTRAMAKEARGS) HOSTCC="$(HOSTCC_NOCCACHE)" HOSTCXX="$(HOSTCXX_NOCCACHE)" syncconfig
 
@@ -605,11 +601,6 @@ sdk: prepare-sdk $(BR2_TAR_HOST_DEPENDENCY)
 		--transform='s#^\.#$(BR2_SDK_PREFIX)#' \
 		-C $(HOST_DIR) "."
 
-# Populating the staging with the base directories is handled by the skeleton package
-$(STAGING_DIR):
-	@mkdir -p $(STAGING_DIR)
-	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
-
 RSYNC_VCS_EXCLUSIONS = \
 	--exclude .svn --exclude .git --exclude .hg --exclude .bzr \
 	--exclude CVS
@@ -710,8 +701,14 @@ $(TARGETS_ROOTFS): target-finalize
 # Avoid the rootfs name leaking down the dependency chain
 target-finalize: ROOTFS=
 
+host-finalize: $(HOST_DIR_SYMLINK)
+
+.PHONY: staging-finalize
+staging-finalize:
+	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
+
 .PHONY: target-finalize
-target-finalize: $(PACKAGES)
+target-finalize: $(PACKAGES) host-finalize
 	@$(call MESSAGE,"Finalizing target directory")
 	# Check files that are touched by more than one package
 	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt
@@ -782,7 +779,7 @@ endif
 	touch $(TARGET_DIR)/usr
 
 .PHONY: target-post-image
-target-post-image: $(TARGETS_ROOTFS) target-finalize
+target-post-image: $(TARGETS_ROOTFS) target-finalize staging-finalize
 	@rm -f $(ROOTFS_COMMON_TAR)
 	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
 		$(call MESSAGE,"Executing post-image script $(s)"); \
@@ -811,7 +808,7 @@ legal-info-prepare: $(LEGAL_INFO_DIR)
 	@cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config
 
 .PHONY: legal-info
-legal-info: dirs legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
+legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
 		$(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST)
 	@cat support/legal-info/README.header >>$(LEGAL_REPORT)
 	@if [ -r $(LEGAL_WARNINGS) ]; then \
diff --git a/fs/common.mk b/fs/common.mk
index 2a5a202a89..96658428ba 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -145,6 +145,7 @@ $$(BINARIES_DIR)/rootfs.$(1): ROOTFS=$(2)
 $$(BINARIES_DIR)/rootfs.$(1): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
 $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
+	mkdir -p $$(@D)
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f34f46afc8..309fd8cd48 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -173,6 +173,7 @@ $(BUILD_DIR)/%/.stamp_extracted:
 $(BUILD_DIR)/%/.stamp_rsynced:
 	@$(call step_start,rsync)
 	@$(call MESSAGE,"Syncing from source dir $(SRCDIR)")
+	@mkdir -p $(@D)
 	$(foreach hook,$($(PKG)_PRE_RSYNC_HOOKS),$(call $(hook))$(sep))
 	@test -d $(SRCDIR) || (echo "ERROR: $(SRCDIR) does not exist" ; exit 1)
 	rsync -au --chmod=u=rwX,go=rX $(RSYNC_VCS_EXCLUSIONS) $($(PKG)_OVERRIDE_SRCDIR_RSYNC_EXCLUSIONS) $(call qstrip,$(SRCDIR))/ $(@D)
@@ -238,6 +239,7 @@ $(BUILD_DIR)/%/.stamp_built::
 $(BUILD_DIR)/%/.stamp_host_installed:
 	@$(call step_start,install-host)
 	@$(call MESSAGE,"Installing to host directory")
+	@mkdir -p $(HOST_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
@@ -267,6 +269,7 @@ $(BUILD_DIR)/%/.stamp_host_installed:
 $(BUILD_DIR)/%/.stamp_staging_installed:
 	@$(call step_start,install-staging)
 	@$(call MESSAGE,"Installing to staging directory")
+	@mkdir -p $(STAGING_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
@@ -298,6 +301,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 # Install to images dir
 $(BUILD_DIR)/%/.stamp_images_installed:
 	@$(call step_start,install-image)
+	@mkdir -p $(BINARIES_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_IMAGES_HOOKS),$(call $(hook))$(sep))
 	@$(call MESSAGE,"Installing to images directory")
 	+$($(PKG)_INSTALL_IMAGES_CMDS)
@@ -309,6 +313,7 @@ $(BUILD_DIR)/%/.stamp_images_installed:
 $(BUILD_DIR)/%/.stamp_target_installed:
 	@$(call step_start,install-target)
 	@$(call MESSAGE,"Installing to target")
+	@mkdir -p $(TARGET_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_TARGET_CMDS)
 	$(if $(BR2_INIT_SYSTEMD),\
@@ -735,7 +740,7 @@ $$($(2)_TARGET_BUILD):	$$($(2)_TARGET_CONFIGURE)
 $(1)-configure:			$$($(2)_TARGET_CONFIGURE)
 $$($(2)_TARGET_CONFIGURE):	| $$($(2)_FINAL_DEPENDENCIES)
 
-$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare
+$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | prepare
 $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies
 
 ifeq ($$($(2)_OVERRIDE_SRCDIR),)
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (2 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 3/9] Makefile: rework main directory creation logic Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 20:18   ` Yann E. MORIN
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR Thomas Petazzoni
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

In a follow-up commit, we will make the .NOTPARALLEL statement
conditional on a Config.in option, so we need to move it further down.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Makefile | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile
index e675ac26aa..24c803872d 100644
--- a/Makefile
+++ b/Makefile
@@ -105,22 +105,6 @@ ifneq ($(firstword $(sort $(RUNNING_MAKE_VERSION) $(MIN_MAKE_VERSION))),$(MIN_MA
 $(error You have make '$(RUNNING_MAKE_VERSION)' installed. GNU make >= $(MIN_MAKE_VERSION) is required)
 endif
 
-# Parallel execution of this Makefile is disabled because it changes
-# the packages building order, that can be a problem for two reasons:
-# - If a package has an unspecified optional dependency and that
-#   dependency is present when the package is built, it is used,
-#   otherwise it isn't (but compilation happily proceeds) so the end
-#   result will differ if the order is swapped due to parallel
-#   building.
-# - Also changing the building order can be a problem if two packages
-#   manipulate the same file in the target directory.
-#
-# Taking into account the above considerations, if you still want to execute
-# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# use the -j<jobs> option when building, e.g:
-#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
-.NOTPARALLEL:
-
 # absolute path
 TOPDIR := $(CURDIR)
 CONFIG_CONFIG_IN = Config.in
@@ -246,6 +230,22 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
 -include $(BR2_CONFIG)
 endif
 
+# Parallel execution of this Makefile is disabled because it changes
+# the packages building order, that can be a problem for two reasons:
+# - If a package has an unspecified optional dependency and that
+#   dependency is present when the package is built, it is used,
+#   otherwise it isn't (but compilation happily proceeds) so the end
+#   result will differ if the order is swapped due to parallel
+#   building.
+# - Also changing the building order can be a problem if two packages
+#   manipulate the same file in the target directory.
+#
+# Taking into account the above considerations, if you still want to execute
+# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
+.NOTPARALLEL:
+
 # timezone and locale may affect build output
 ifeq ($(BR2_REPRODUCIBLE),y)
 export TZ = UTC
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (3 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 20:32   ` Yann E. MORIN
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders Thomas Petazzoni
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

In commit 7e9870ce32d6329d9e3d602247fbe1709a2275a4 ("core: introduce
intermediate BASE_TARGET_DIR variable"), the definition of
TARGET_DIR_WARNING_FILE was changed to use $(BASE_TARGET_DIR) instead
of $(TARGET_DIR).

However, this change is incompatible with per-package directories, and
is in fact not needed.

With per-package directories, using $(BASE_TARGET_DIR) means that
TARGET_DIR_WARNING_FILE is
output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM. Due to this, when
skeleton-init-common or skeleton-custom attempt to install it, it
fails, because it should be installed to their package per-package
target directory, and not the global output/target folder that doesn't
exist yet. The failure looks like this:

/usr/bin/install -m 0644 support/misc/target-dir-warning.txt /home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
/usr/bin/install: cannot create regular file '/home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM': No such file or directory
make[1]: *** [package/pkg-generic.mk:336: /home/thomas/projets/buildroot/output/build/skeleton-init-common/.stamp_target_installed] Error 1

TARGET_DIR_WARNING_FILE is used in three places:

 - In skeleton-custom.mk and skeleton-init-common.mk, where as
   explained above, using $(TARGET_DIR) fixes the use of
   $(TARGET_DIR_WARNING_FILE) in the context of per-package target
   directories.

 - In fs/common.mk, where it is used as argument to $(notdir ...) to
   retrieve just the name of the warning file. So in this case, we
   really don't care about the path of the file, just its name.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 24c803872d..23032988a5 100644
--- a/Makefile
+++ b/Makefile
@@ -468,7 +468,7 @@ BR_PATH = "$(HOST_DIR)/bin:$(HOST_DIR)/sbin:$(PATH)"
 
 # Location of a file giving a big fat warning that output/target
 # should not be used as the root filesystem.
-TARGET_DIR_WARNING_FILE = $(BASE_TARGET_DIR)/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
+TARGET_DIR_WARNING_FILE = $(TARGET_DIR)/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
 
 ifeq ($(BR2_CCACHE),y)
 CCACHE = $(HOST_DIR)/bin/ccache
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (4 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 20:53   ` Yann E. MORIN
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target Thomas Petazzoni
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

This commit adjusts the logic in pkg-generic.mk that tweaks the
*-config shell scripts installed by various libraries to make it
compatible with per-package folders.

This requires two fixes:

 - replacing $(STAGING_DIR) with a relative path from the config script
   to the staging directory, rather than using an absolute path of the
   staging directory.

   Without this, a *-config script provided by package A, but called
   from package B per-package folder will return paths from package A
   per-package folder:

   $ ./output/per-package/mcrypt/host/usr/<tuple>/sysroot/usr/bin/libmcrypt-config --libs
   -L..../output/per-package/libmcrypt/host/usr/<tuple>/sysroot/usr/lib/

   The libmcrypt-config script is installed by the libmcrypt package,
   and mcrypt is a package that depends on libmcrypt. When we call the
   libmcrypt-config script from the mcrypt per-package folder, it
   returns a -L flag that points to the libmcrypt per-package folder.

   One might say: but this is OK, since the sysroot of the libmcrypt
   per-package folder also contains the libmcrypt library. This is
   true, but we encounter a more subtle issue: because -L paths are
   considered before standard paths, ld ends up finding libc.so in the
   libmcrypt per-package folder. This libc.so file is a linker script
   that looks like this:

   GROUP ( /lib/libc.so.6 /usr/lib/libc_nonshared.a  AS_NEEDED ( /lib/ld-linux.so.3 ) )

   Normally, thanks to ld sysroot awareness, /lib/libc.so.6 in this
   script is re-interpreted according to the sysroot. But in this
   case, the library is *outside* the compiler sysroot. Remember: we
   are using the compiler/linker from the "mcrypt" per-package folder,
   but we found "libc.so.6" in the "libmcrypt" per-package folder.

   This causes the linker to really use the /lib/libc.so.6 from the
   host machine, obvisouly leading to a build failure such as:

   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /lib/libc.so.6
   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /usr/lib/libc_nonshared.a
   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /lib/ld-linux-nios2.so.1

 - Some *-config scripts, such as the apr-1-config script, contain
   references to host tools:

   CC=".../output/per-package/apr/hosr/bin/arm-linux-gcc"
   CCP=".../output/per-package/apr/hosr/bin/arm-linux-cpp"

   We also want to replace those with proper relative paths. To
   achieve this, we need to also replace $(HOST_DIR) with a relative
   path. Since $(STAGING_DIR) is inside $(HOST_DIR), the first
   replacement of $(STAGING_DIR) by @STAGING_DIR@ is no longer needed:
   replacing $(HOST_DIR) by @HOST_DIR@ is sufficient. We still need to
   replace @STAGING_DIR@ by the proper path though, as we introduce
   @STAGING_DIR@ references in exec_prefix and prefix variables, as
   well as -I and -L flags.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/pkg-generic.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 309fd8cd48..9b4db845b6 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -275,12 +275,13 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(call MESSAGE,"Fixing package configuration files") ;\
-			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \
-				-e "s,$(STAGING_DIR), at STAGING_DIR@,g" \
+			$(SED)  "s,$(HOST_DIR), at HOST_DIR@,g" \
+				-e "s,$(BASE_DIR), at BASE_DIR@,g" \
 				-e "s,^\(exec_\)\?prefix=.*,\1prefix=@STAGING_DIR@/usr,g" \
 				-e "s,-I/usr/,-I at STAGING_DIR@/usr/,g" \
 				-e "s,-L/usr/,-L at STAGING_DIR@/usr/,g" \
-				-e "s, at STAGING_DIR@,$(STAGING_DIR),g" \
+				-e 's, at STAGING_DIR@,$$(dirname $$0)/../..,g' \
+				-e 's, at HOST_DIR@,$$(dirname $$0)/../../../..,g' \
 				-e "s, at BASE_DIR@,$(BASE_DIR),g" \
 				$(addprefix $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ;\
 	fi
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (5 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 21:36   ` Yann E. MORIN
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 8/9] package/pkg-generic: make libtool .la files compatible with per-package folders Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 9/9] package/pkg-kconfig: handle KCONFIG_DEPENDENCIES " Thomas Petazzoni
  8 siblings, 1 reply; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

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.

 - The global TARGET_DIR and HOST_DIR variable now automatically point
   to the per-package directory when PKG is defined. So whenever a
   package references $(HOST_DIR) or $(TARGET_DIR) in its build
   process, it effectively references the per-package host/target
   directories. Note that STAGING_DIR is a sub-dir of HOST_DIR, so it
   is handled as well.

 - Of course, packages have dependencies, so those dependencies must
   be installed in the per-package host and target folders. 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. Note that this only works because we make the
   assumption that one package does *not* overwrite files installed by
   another package.

   This is done for "extract dependencies" at the beginning of the
   extract step, and for "normal dependencies" at the beginning of the
   configure step.

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 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.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Config.in                        | 18 ++++++++++++++++
 Makefile                         | 37 +++++++++++++++++++++++++++-----
 fs/common.mk                     |  2 +-
 package/pkg-generic.mk           | 20 ++++++++++++++++-
 support/scripts/check-host-rpath |  5 ++++-
 5 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/Config.in b/Config.in
index f965e9d6d8..70eaab900f 100644
--- a/Config.in
+++ b/Config.in
@@ -696,6 +696,24 @@ config BR2_REPRODUCIBLE
 	  This is labeled as an experimental feature, as not all
 	  packages behave properly to ensure reproducibility.
 
+config BR2_PER_PACKAGE_FOLDERS
+	bool "Use per-package folders (experimental)"
+	help
+	  This option will change the build process of Buildroot
+	  package to use per-package target and host directories.
+
+	  This is useful for two related purposes:
+
+	    - Cleanly isolate the build of each package, so that a
+	      given package only "sees" the dependencies it has
+	      explicitly expressed, and not other packages that may
+	      have by chance been built before.
+
+	    - Enable top-level parallel build.
+
+	  This is labeled as an experimental feature, as not all
+	  packages behave properly with per-package folders.
+
 endmenu
 
 comment "Security Hardening Options"
diff --git a/Makefile b/Makefile
index 23032988a5..f3c0e2326e 100644
--- a/Makefile
+++ b/Makefile
@@ -207,7 +207,8 @@ BINARIES_DIR := $(BASE_DIR)/images
 # The target directory is common to all packages,
 # but there is one that is specific to each filesystem.
 BASE_TARGET_DIR := $(BASE_DIR)/target
-TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+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
@@ -230,6 +231,7 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
 -include $(BR2_CONFIG)
 endif
 
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),)
 # Parallel execution of this Makefile is disabled because it changes
 # the packages building order, that can be a problem for two reasons:
 # - If a package has an unspecified optional dependency and that
@@ -245,6 +247,13 @@ endif
 # use the -j<jobs> option when building, e.g:
 #      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
+endif
+
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/target,$(BASE_TARGET_DIR)))
+else
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+endif
 
 # timezone and locale may affect build output
 ifeq ($(BR2_REPRODUCIBLE),y)
@@ -455,7 +464,11 @@ LZCAT := $(call qstrip,$(BR2_LZCAT))
 TAR_OPTIONS = $(call qstrip,$(BR2_TAR_OPTIONS)) -xf
 
 # packages compiled for the host go here
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+HOST_DIR = $(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/host,$(call qstrip,$(BR2_HOST_DIR)))
+else
 HOST_DIR := $(call qstrip,$(BR2_HOST_DIR))
+endif
 
 ifneq ($(HOST_DIR),$(BASE_DIR)/host)
 HOST_DIR_SYMLINK = $(BASE_DIR)/host
@@ -701,14 +714,28 @@ $(TARGETS_ROOTFS): target-finalize
 # Avoid the rootfs name leaking down the dependency chain
 target-finalize: ROOTFS=
 
-host-finalize: $(HOST_DIR_SYMLINK)
+host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+	@$(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))
+endif
 
 .PHONY: staging-finalize
 staging-finalize:
 	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
 
 .PHONY: target-finalize
-target-finalize: $(PACKAGES) host-finalize
+target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+	@$(call MESSAGE,"Creating global target directory")
+	$(foreach pkg,$(PACKAGES),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(TARGET_DIR)$(sep))
+endif
 	@$(call MESSAGE,"Finalizing target directory")
 	# Check files that are touched by more than one package
 	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt
@@ -972,7 +999,7 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf prepare-kconfig
 
 # staging and target directories do NOT list these as
 # dependencies anywhere else
-$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
+$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):
 	@mkdir -p $@
 
 # outputmakefile generates a Makefile in the output directory, if using a
@@ -1011,7 +1038,7 @@ printvars:
 clean:
 	rm -rf $(BASE_TARGET_DIR) $(BINARIES_DIR) $(HOST_DIR) $(HOST_DIR_SYMLINK) \
 		$(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/fs/common.mk b/fs/common.mk
index 96658428ba..fc4be3cc05 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -172,7 +172,7 @@ rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
 
 ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
 TARGETS_ROOTFS += rootfs-$(1)
-PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))
+PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES)) $(ROOTFS_COMMON_DEPENDENCIES)
 endif
 
 # Check for legacy POST_TARGETS rules
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 9b4db845b6..884b868a9a 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -98,7 +98,7 @@ GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
 # 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)))
+		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR) $(PER_PACKAGE_DIR)))
 endef
 GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
 
@@ -126,6 +126,21 @@ endef
 GLOBAL_INSTRUMENTATION_HOOKS += step_user
 endif
 
+# $1: deps
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+define prepare-per-package-folder
+	mkdir -p $(HOST_DIR) $(TARGET_DIR)
+	$(foreach pkg,$(1),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))
+	$(foreach pkg,$(1),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(TARGET_DIR)$(sep))
+endef
+endif
+
 ################################################################################
 # Implicit targets -- produce a stamp file for each step of a package build
 ################################################################################
@@ -133,6 +148,7 @@ endif
 # Retrieve the archive
 $(BUILD_DIR)/%/.stamp_downloaded:
 	@$(call step_start,download)
+	$(call prepare-per-package-folder,$($(PKG)_FINAL_DOWNLOAD_DEPENDENCIES))
 	$(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
 # Only show the download message if it isn't already downloaded
 	$(Q)for p in $($(PKG)_ALL_DOWNLOADS); do \
@@ -159,6 +175,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
 $(BUILD_DIR)/%/.stamp_extracted:
 	@$(call step_start,extract)
 	@$(call MESSAGE,"Extracting")
+	$(call prepare-per-package-folder,$($(PKG)_FINAL_EXTRACT_DEPENDENCIES))
 	$(foreach hook,$($(PKG)_PRE_EXTRACT_HOOKS),$(call $(hook))$(sep))
 	$(Q)mkdir -p $(@D)
 	$($(PKG)_EXTRACT_CMDS)
@@ -219,6 +236,7 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
 $(BUILD_DIR)/%/.stamp_configured:
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
+	$(call prepare-per-package-folder,$($(PKG)_FINAL_DEPENDENCIES))
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
index 6c5767da05..787a1763b0 100755
--- a/support/scripts/check-host-rpath
+++ b/support/scripts/check-host-rpath
@@ -11,6 +11,7 @@ export LC_ALL=C
 main() {
     local pkg="${1}"
     local hostdir="${2}"
+    local perpackagedir="${3}"
     local file ret
 
     # Remove duplicate and trailing '/' for proper match
@@ -20,7 +21,7 @@ main() {
     while read file; do
         is_elf "${file}" || continue
         elf_needs_rpath "${file}" "${hostdir}" || continue
-        check_elf_has_rpath "${file}" "${hostdir}" && continue
+        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
         if [ ${ret} -eq 0 ]; then
             ret=1
             printf "***\n"
@@ -57,6 +58,7 @@ elf_needs_rpath() {
 check_elf_has_rpath() {
     local file="${1}"
     local hostdir="${2}"
+    local perpackagedir="${3}"
     local rpath dir
 
     while read rpath; do
@@ -65,6 +67,7 @@ check_elf_has_rpath() {
             dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
             [ "${dir}" = "${hostdir}/lib" ] && return 0
             [ "${dir}" = "\$ORIGIN/../lib" ] && return 0
+            [[ ${dir} =~ ${perpackagedir}/[^/]*/host/lib ]] && return 0
         done
     done < <( readelf -d "${file}"                                              \
               |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 8/9] package/pkg-generic: make libtool .la files compatible with per-package folders
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (6 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 9/9] package/pkg-kconfig: handle KCONFIG_DEPENDENCIES " Thomas Petazzoni
  8 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

Libtool .la files unfortunately contain a number of absolute paths,
which now refer to per-package folders. Due to this, when building
package A, .la files may contain absolute paths referring to folders
in package B per-package sysroot. This causes some -L flags referring
to other sysroot from being added, which doesn't work as the linker no
longer realizes that such paths are within its sysroot.

To fix this, we introduce a replacement step of .la files in the
configure step, to make sure all paths refer to this package
per-package folder.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/pkg-generic.mk | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 884b868a9a..b2da97712b 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -237,6 +237,12 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
 	$(call prepare-per-package-folder,$($(PKG)_FINAL_DEPENDENCIES))
+ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
+	# Make sure .la files only reference the current per-package
+	# folder.
+	$(Q)find $(STAGING_DIR)/usr/lib* -name "*.la" | xargs --no-run-if-empty \
+		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]*/:$(PER_PACKAGE_DIR)/$(NAME)/:g"
+endif
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
@@ -890,6 +896,7 @@ $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
 $$($(2)_TARGET_INSTALL_HOST):		PKG=$(2)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		NAME=$(1)
 $$($(2)_TARGET_RSYNC):			SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 9/9] package/pkg-kconfig: handle KCONFIG_DEPENDENCIES with per-package folders
  2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
                   ` (7 preceding siblings ...)
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 8/9] package/pkg-generic: make libtool .la files compatible with per-package folders Thomas Petazzoni
@ 2018-11-20 16:35 ` Thomas Petazzoni
  8 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-20 16:35 UTC (permalink / raw)
  To: buildroot

The pkg-kconfig infrastructure hijacks the regular chain of build
steps to insert its own step to prepare the configuration of kconfig
packages. This additional step may have dependencies of its own, such
as host-flex, host-bison or toolchain.

In the context of per-package folder support, those dependencies must
be copied to the per-package folder of the current package prior to
doing the config preparation. This commit implements this logic by
adding a call to prepare-per-package-folder at the right spot.

Reported-by: Andreas Naumann <anaumann@ultratronik.de>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/pkg-kconfig.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index d6c95b897e..bf43632adc 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -113,6 +113,7 @@ endef
 # Since the file could be a defconfig file it needs to be expanded to a
 # full .config first.
 $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG): $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
+	$$(call prepare-per-package-folder,$$($(2)_KCONFIG_DEPENDENCIES))
 	$$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
 		$$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
 		$$(INSTALL) -m 0644 -D $$($(2)_KCONFIG_FILE) $$(@))
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file Thomas Petazzoni
@ 2018-11-20 20:18   ` Yann E. MORIN
  2018-11-21 13:44     ` Thomas Petazzoni
  0 siblings, 1 reply; 20+ messages in thread
From: Yann E. MORIN @ 2018-11-20 20:18 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
> In a follow-up commit, we will make the .NOTPARALLEL statement
> conditional on a Config.in option, so we need to move it further down.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

You forgot to carry my:

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

(Granted, it was hiding below a long rant/research about the need of a
config option ;-] )

Regards,
Yann E. MORIN.

> ---
>  Makefile | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e675ac26aa..24c803872d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,22 +105,6 @@ ifneq ($(firstword $(sort $(RUNNING_MAKE_VERSION) $(MIN_MAKE_VERSION))),$(MIN_MA
>  $(error You have make '$(RUNNING_MAKE_VERSION)' installed. GNU make >= $(MIN_MAKE_VERSION) is required)
>  endif
>  
> -# Parallel execution of this Makefile is disabled because it changes
> -# the packages building order, that can be a problem for two reasons:
> -# - If a package has an unspecified optional dependency and that
> -#   dependency is present when the package is built, it is used,
> -#   otherwise it isn't (but compilation happily proceeds) so the end
> -#   result will differ if the order is swapped due to parallel
> -#   building.
> -# - Also changing the building order can be a problem if two packages
> -#   manipulate the same file in the target directory.
> -#
> -# Taking into account the above considerations, if you still want to execute
> -# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
> -# use the -j<jobs> option when building, e.g:
> -#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
> -.NOTPARALLEL:
> -
>  # absolute path
>  TOPDIR := $(CURDIR)
>  CONFIG_CONFIG_IN = Config.in
> @@ -246,6 +230,22 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
>  -include $(BR2_CONFIG)
>  endif
>  
> +# Parallel execution of this Makefile is disabled because it changes
> +# the packages building order, that can be a problem for two reasons:
> +# - If a package has an unspecified optional dependency and that
> +#   dependency is present when the package is built, it is used,
> +#   otherwise it isn't (but compilation happily proceeds) so the end
> +#   result will differ if the order is swapped due to parallel
> +#   building.
> +# - Also changing the building order can be a problem if two packages
> +#   manipulate the same file in the target directory.
> +#
> +# Taking into account the above considerations, if you still want to execute
> +# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
> +# use the -j<jobs> option when building, e.g:
> +#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
> +.NOTPARALLEL:
> +
>  # timezone and locale may affect build output
>  ifeq ($(BR2_REPRODUCIBLE),y)
>  export TZ = UTC
> -- 
> 2.19.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR Thomas Petazzoni
@ 2018-11-20 20:32   ` Yann E. MORIN
  0 siblings, 0 replies; 20+ messages in thread
From: Yann E. MORIN @ 2018-11-20 20:32 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
> In commit 7e9870ce32d6329d9e3d602247fbe1709a2275a4 ("core: introduce
> intermediate BASE_TARGET_DIR variable"), the definition of
> TARGET_DIR_WARNING_FILE was changed to use $(BASE_TARGET_DIR) instead
> of $(TARGET_DIR).
> 
> However, this change is incompatible with per-package directories, and
> is in fact not needed.
> 
> With per-package directories, using $(BASE_TARGET_DIR) means that
> TARGET_DIR_WARNING_FILE is
> output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM. Due to this, when
> skeleton-init-common or skeleton-custom attempt to install it, it
> fails, because it should be installed to their package per-package
> target directory, and not the global output/target folder that doesn't
> exist yet. The failure looks like this:
> 
> /usr/bin/install -m 0644 support/misc/target-dir-warning.txt /home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
> /usr/bin/install: cannot create regular file '/home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM': No such file or directory
> make[1]: *** [package/pkg-generic.mk:336: /home/thomas/projets/buildroot/output/build/skeleton-init-common/.stamp_target_installed] Error 1
> 
> TARGET_DIR_WARNING_FILE is used in three places:
> 
>  - In skeleton-custom.mk and skeleton-init-common.mk, where as
>    explained above, using $(TARGET_DIR) fixes the use of
>    $(TARGET_DIR_WARNING_FILE) in the context of per-package target
>    directories.
> 
>  - In fs/common.mk, where it is used as argument to $(notdir ...) to
>    retrieve just the name of the warning file. So in this case, we
>    really don't care about the path of the file, just its name.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

At the beginning, I was a bit skeptical, and I was also wondering how it
would work with my on-going refactoring due to handling the capabilities,
but I so far can't see any problem with it. So:

Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 24c803872d..23032988a5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -468,7 +468,7 @@ BR_PATH = "$(HOST_DIR)/bin:$(HOST_DIR)/sbin:$(PATH)"
>  
>  # Location of a file giving a big fat warning that output/target
>  # should not be used as the root filesystem.
> -TARGET_DIR_WARNING_FILE = $(BASE_TARGET_DIR)/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
> +TARGET_DIR_WARNING_FILE = $(TARGET_DIR)/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM
>  
>  ifeq ($(BR2_CCACHE),y)
>  CCACHE = $(HOST_DIR)/bin/ccache
> -- 
> 2.19.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders Thomas Petazzoni
@ 2018-11-20 20:53   ` Yann E. MORIN
  2018-11-23 12:46     ` Thomas Petazzoni
  0 siblings, 1 reply; 20+ messages in thread
From: Yann E. MORIN @ 2018-11-20 20:53 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
> This commit adjusts the logic in pkg-generic.mk that tweaks the
> *-config shell scripts installed by various libraries to make it
> compatible with per-package folders.
> 
> This requires two fixes:
> 
>  - replacing $(STAGING_DIR) with a relative path from the config script
>    to the staging directory, rather than using an absolute path of the
>    staging directory.

Note [0], see below...

[--SNIP--]
>  - Some *-config scripts, such as the apr-1-config script, contain
>    references to host tools:
> 
>    CC=".../output/per-package/apr/hosr/bin/arm-linux-gcc"
>    CCP=".../output/per-package/apr/hosr/bin/arm-linux-cpp"
> 
>    We also want to replace those with proper relative paths. To

Note [1], see below...

>    achieve this, we need to also replace $(HOST_DIR) with a relative
>    path. Since $(STAGING_DIR) is inside $(HOST_DIR), the first
>    replacement of $(STAGING_DIR) by @STAGING_DIR@ is no longer needed:
>    replacing $(HOST_DIR) by @HOST_DIR@ is sufficient. We still need to
>    replace @STAGING_DIR@ by the proper path though, as we introduce
>    @STAGING_DIR@ references in exec_prefix and prefix variables, as
>    well as -I and -L flags.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/pkg-generic.mk | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 309fd8cd48..9b4db845b6 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -275,12 +275,13 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
>  	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
>  	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
>  		$(call MESSAGE,"Fixing package configuration files") ;\
> -			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \
> -				-e "s,$(STAGING_DIR), at STAGING_DIR@,g" \
> +			$(SED)  "s,$(HOST_DIR), at HOST_DIR@,g" \
> +				-e "s,$(BASE_DIR), at BASE_DIR@,g" \

Previously, BASE_DIR was the very first thing replaced, but now it no
longer is.

I'm not sure if I follow it all, but is the reason for that inversion
hidden behhinde notes [0] and [1], being the need to create relative
symlinks?

As far as I understand, if BASE_DIR is kept as the first replacement,
then we lose HOST_DIR as it (usually) is a subdir of BASE_DIR, so we end
up with just @BASE_DIR@/host and thus can't easily replace it with
relative paths anymore.

Right?

If so, then:

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

>  				-e "s,^\(exec_\)\?prefix=.*,\1prefix=@STAGING_DIR@/usr,g" \
>  				-e "s,-I/usr/,-I at STAGING_DIR@/usr/,g" \
>  				-e "s,-L/usr/,-L at STAGING_DIR@/usr/,g" \
> -				-e "s, at STAGING_DIR@,$(STAGING_DIR),g" \
> +				-e 's, at STAGING_DIR@,$$(dirname $$0)/../..,g' \
> +				-e 's, at HOST_DIR@,$$(dirname $$0)/../../../..,g' \
>  				-e "s, at BASE_DIR@,$(BASE_DIR),g" \
>  				$(addprefix $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ;\
>  	fi
> -- 
> 2.19.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-20 16:35 ` [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target Thomas Petazzoni
@ 2018-11-20 21:36   ` Yann E. MORIN
  2018-11-20 23:32     ` Arnout Vandecappelle
  2018-11-23 13:14     ` Thomas Petazzoni
  0 siblings, 2 replies; 20+ messages in thread
From: Yann E. MORIN @ 2018-11-20 21:36 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
> 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.
[--SNIP--]

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

I'm not fond of the naming here...

In the end, can we expect to have a layout that would look like:

    output/build/busybox/host/
    output/build/busybox/target/
    output/build/busybox/busybox-1.2.3/     # Source tree, and
                                            # currently, build dir
And then we can add for OOT:
    output/build/busybox/build-dir/

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
[--SNIP--]
> diff --git a/Makefile b/Makefile
> index 23032988a5..f3c0e2326e 100644
> --- a/Makefile
> +++ b/Makefile
>@@ -207,7 +207,8 @@ BINARIES_DIR := $(BASE_DIR)/images
> # The target directory is common to all packages,
> # but there is one that is specific to each filesystem.
> BASE_TARGET_DIR := $(BASE_DIR)/target
>-TARGET_DIR = $(if
>$(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
>+PER_PACKAGE_DIR := $(BASE_DIR)/per-package

As I said above, I'm not too fond of that naming.... I have nothing
better to suggest for now, except that we can change that later (enough
bike-shedding).

> # 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
> @@ -230,6 +231,7 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
>  -include $(BR2_CONFIG)
>  endif
>  
> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),)
>  # Parallel execution of this Makefile is disabled because it changes
>  # the packages building order, that can be a problem for two reasons:
>  # - If a package has an unspecified optional dependency and that
> @@ -245,6 +247,13 @@ endif
>  # use the -j<jobs> option when building, e.g:
>  #      make -j$((`getconf _NPROCESSORS_ONLN`+1))
>  .NOTPARALLEL:
> +endif

I would have postponed parallel build activation to a separate commit.

> @@ -455,7 +464,11 @@ LZCAT := $(call qstrip,$(BR2_LZCAT))
>  TAR_OPTIONS = $(call qstrip,$(BR2_TAR_OPTIONS)) -xf
>  
>  # packages compiled for the host go here
> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> +HOST_DIR = $(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/host,$(call qstrip,$(BR2_HOST_DIR)))
> +else
>  HOST_DIR := $(call qstrip,$(BR2_HOST_DIR))

Why do we still have HOST_DIR as immediately-defined?

Since HOST_DIR is simply defined in the per-package case, I see no
reason to have immediately-defined in the standard case either.

> +endif
>  
>  ifneq ($(HOST_DIR),$(BASE_DIR)/host)
>  HOST_DIR_SYMLINK = $(BASE_DIR)/host
> @@ -701,14 +714,28 @@ $(TARGETS_ROOTFS): target-finalize
>  # Avoid the rootfs name leaking down the dependency chain
>  target-finalize: ROOTFS=
>  
> -host-finalize: $(HOST_DIR_SYMLINK)
> +host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> +	@$(call MESSAGE,"Creating global host directory")
> +	$(foreach pkg,$(PACKAGES),\

Please, sort PACKAGES, so that:

  - the copy is always done in the same order (sort always sort in the
    ascii order);

  - duplicates get removed.

> +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> +		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> +		$(HOST_DIR)$(sep))
> +endif
>  
>  .PHONY: staging-finalize
>  staging-finalize:
>  	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
>  
>  .PHONY: target-finalize
> -target-finalize: $(PACKAGES) host-finalize
> +target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize
> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> +	@$(call MESSAGE,"Creating global target directory")
> +	$(foreach pkg,$(PACKAGES),\

Ditto, sort PACKAGES.

> +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> +		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> +		$(TARGET_DIR)$(sep))
> +endif
>  	@$(call MESSAGE,"Finalizing target directory")
>  	# Check files that are touched by more than one package
>  	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt
> @@ -972,7 +999,7 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf prepare-kconfig
>  
>  # staging and target directories do NOT list these as
>  # dependencies anywhere else
> -$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
> +$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):

Can we now split this long line, please? ;-)

> diff --git a/fs/common.mk b/fs/common.mk
> index 96658428ba..fc4be3cc05 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -172,7 +172,7 @@ rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
>  
>  ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
>  TARGETS_ROOTFS += rootfs-$(1)
> -PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))
> +PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES)) $(ROOTFS_COMMON_DEPENDENCIES)

If I am not mistaken, this is now in master:
    https://git.buildroot.org/buildroot/commit/?id=305e4487e5c18ed89bf2aa106b2068f9dce686fb

But that's missing from next, indeed.

>  endif
>  
>  # Check for legacy POST_TARGETS rules
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 9b4db845b6..884b868a9a 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -98,7 +98,7 @@ GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
>  # 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)))
> +		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR) $(PER_PACKAGE_DIR)))
>  endef
>  GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
>  
> @@ -126,6 +126,21 @@ endef
>  GLOBAL_INSTRUMENTATION_HOOKS += step_user
>  endif
>  
> +# $1: deps
> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> +define prepare-per-package-folder

The location where you define this macro, in the instrumentation hooks,
is not optimum I think. You even did not provide a help-comment for it.

> +	mkdir -p $(HOST_DIR) $(TARGET_DIR)
> +	$(foreach pkg,$(1),\
> +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> +		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> +		$(HOST_DIR)$(sep))
> +	$(foreach pkg,$(1),\
> +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> +		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> +		$(TARGET_DIR)$(sep))

Why do you need two 'foreach' calls? Can't the following work?

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

Regards,
Yann E. MORIN.

> +endef
> +endif
> +
>  ################################################################################
>  # Implicit targets -- produce a stamp file for each step of a package build
>  ################################################################################
> @@ -133,6 +148,7 @@ endif
>  # Retrieve the archive
>  $(BUILD_DIR)/%/.stamp_downloaded:
>  	@$(call step_start,download)
> +	$(call prepare-per-package-folder,$($(PKG)_FINAL_DOWNLOAD_DEPENDENCIES))
>  	$(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
>  # Only show the download message if it isn't already downloaded
>  	$(Q)for p in $($(PKG)_ALL_DOWNLOADS); do \
> @@ -159,6 +175,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
>  $(BUILD_DIR)/%/.stamp_extracted:
>  	@$(call step_start,extract)
>  	@$(call MESSAGE,"Extracting")
> +	$(call prepare-per-package-folder,$($(PKG)_FINAL_EXTRACT_DEPENDENCIES))
>  	$(foreach hook,$($(PKG)_PRE_EXTRACT_HOOKS),$(call $(hook))$(sep))
>  	$(Q)mkdir -p $(@D)
>  	$($(PKG)_EXTRACT_CMDS)
> @@ -219,6 +236,7 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>  $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call step_start,configure)
>  	@$(call MESSAGE,"Configuring")
> +	$(call prepare-per-package-folder,$($(PKG)_FINAL_DEPENDENCIES))
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
>  	$($(PKG)_CONFIGURE_CMDS)
>  	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
> diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
> index 6c5767da05..787a1763b0 100755
> --- a/support/scripts/check-host-rpath
> +++ b/support/scripts/check-host-rpath
> @@ -11,6 +11,7 @@ export LC_ALL=C
>  main() {
>      local pkg="${1}"
>      local hostdir="${2}"
> +    local perpackagedir="${3}"
>      local file ret
>  
>      # Remove duplicate and trailing '/' for proper match
> @@ -20,7 +21,7 @@ main() {
>      while read file; do
>          is_elf "${file}" || continue
>          elf_needs_rpath "${file}" "${hostdir}" || continue
> -        check_elf_has_rpath "${file}" "${hostdir}" && continue
> +        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
>          if [ ${ret} -eq 0 ]; then
>              ret=1
>              printf "***\n"
> @@ -57,6 +58,7 @@ elf_needs_rpath() {
>  check_elf_has_rpath() {
>      local file="${1}"
>      local hostdir="${2}"
> +    local perpackagedir="${3}"
>      local rpath dir
>  
>      while read rpath; do
> @@ -65,6 +67,7 @@ check_elf_has_rpath() {
>              dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
>              [ "${dir}" = "${hostdir}/lib" ] && return 0
>              [ "${dir}" = "\$ORIGIN/../lib" ] && return 0
> +            [[ ${dir} =~ ${perpackagedir}/[^/]*/host/lib ]] && return 0
>          done
>      done < <( readelf -d "${file}"                                              \
>                |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
> -- 
> 2.19.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-20 21:36   ` Yann E. MORIN
@ 2018-11-20 23:32     ` Arnout Vandecappelle
  2018-11-23 13:25       ` Thomas Petazzoni
  2018-11-23 13:14     ` Thomas Petazzoni
  1 sibling, 1 reply; 20+ messages in thread
From: Arnout Vandecappelle @ 2018-11-20 23:32 UTC (permalink / raw)
  To: buildroot



On 20/11/2018 22:36, Yann E. MORIN wrote:
> Thomas, All,
> 
> On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
>> 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.
> [--SNIP--]
> 
>>   output/per-package/busybox/target
>>   output/per-package/busybox/host
>>   output/per-package/host-fakeroot/target
>>   output/per-package/host-fakeroot/host
> 
> I'm not fond of the naming here...

 Yay bikeshedding!

> 
> In the end, can we expect to have a layout that would look like:
> 
>     output/build/busybox/host/
>     output/build/busybox/target/
>     output/build/busybox/busybox-1.2.3/     # Source tree, and
>                                             # currently, build dir

 If we're changing the world anyway, what about:

output/work/busybox-1.2.3/source
output/work/busybox-1.2.3/host
output/work/busybox-1.2.3/target
output/work/busybox-1.2.3/build

 Or maybe even:

output/work/busybox-1.2.3/source
output/work/busybox-1.2.3/dependencies/host
output/work/busybox-1.2.3/dependencies/staging -> host/tuple/sysroot
output/work/busybox-1.2.3/dependencies/target
output/work/busybox-1.2.3/build


 That said, I'm absolutely fine with keeping the current layout. The proposed
layout has the only advantage that you can kill the ppd together with the
source/build dir, but the big disadvantage that it breaks everybody's habits...

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

 Except that this patch doesn't even touch that line...

[snip]
>> +# $1: deps
>> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
>> +define prepare-per-package-folder

 While we're bikeshedding: we have about 1000 occurences of "directory" and less
than 30 of "folder".

> 
> The location where you define this macro, in the instrumentation hooks,
> is not optimum I think. You even did not provide a help-comment for it.

 At some point we started collecting this kind of thing in pkg-utils.mk. But I'm
not sure if we're still doing that...

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

 I kind of like how we first build up all of host, and then all of target. But
really, it doesn't make much of a difference.

 More importantly, however: should we also sync the target directory for host
packages? We *are* syncing the staging directory, of course...

[snip]
>> diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
>> index 6c5767da05..787a1763b0 100755
>> --- a/support/scripts/check-host-rpath
>> +++ b/support/scripts/check-host-rpath
>> @@ -11,6 +11,7 @@ export LC_ALL=C
>>  main() {
>>      local pkg="${1}"
>>      local hostdir="${2}"
>> +    local perpackagedir="${3}"
>>      local file ret
>>  
>>      # Remove duplicate and trailing '/' for proper match
>> @@ -20,7 +21,7 @@ main() {
>>      while read file; do
>>          is_elf "${file}" || continue
>>          elf_needs_rpath "${file}" "${hostdir}" || continue
>> -        check_elf_has_rpath "${file}" "${hostdir}" && continue
>> +        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
>>          if [ ${ret} -eq 0 ]; then
>>              ret=1
>>              printf "***\n"
>> @@ -57,6 +58,7 @@ elf_needs_rpath() {
>>  check_elf_has_rpath() {
>>      local file="${1}"
>>      local hostdir="${2}"
>> +    local perpackagedir="${3}"
>>      local rpath dir
>>  
>>      while read rpath; do
>> @@ -65,6 +67,7 @@ check_elf_has_rpath() {
>>              dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
>>              [ "${dir}" = "${hostdir}/lib" ] && return 0
>>              [ "${dir}" = "\$ORIGIN/../lib" ] && return 0
>> +            [[ ${dir} =~ ${perpackagedir}/[^/]*/host/lib ]] && return
 The commit message doesn't explain why this is needed, and it took me some time
to understand...

 Regards,
 Arnout

>>          done
>>      done < <( readelf -d "${file}"                                              \
>>                |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
>> -- 
>> 2.19.1
>>
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file
  2018-11-20 20:18   ` Yann E. MORIN
@ 2018-11-21 13:44     ` Thomas Petazzoni
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-21 13:44 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 20 Nov 2018 21:18:10 +0100, Yann E. MORIN wrote:
> Thomas, All,
> 
> On 2018-11-20 17:35 +0100, Thomas Petazzoni spake thusly:
> > In a follow-up commit, we will make the .NOTPARALLEL statement
> > conditional on a Config.in option, so we need to move it further down.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>  
> 
> You forgot to carry my:
> 
> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Ah right, I'll add it for v6.

> (Granted, it was hiding below a long rant/research about the need of a
> config option ;-] )

And I will update the commit log to include some explanation as to why
we need an explicit option and not enable per-package folders
automagically when make -jN is passed.

Thanks!

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders
  2018-11-20 20:53   ` Yann E. MORIN
@ 2018-11-23 12:46     ` Thomas Petazzoni
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-23 12:46 UTC (permalink / raw)
  To: buildroot

Hello Yann,

On Tue, 20 Nov 2018 21:53:57 +0100, Yann E. MORIN wrote:

> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 309fd8cd48..9b4db845b6 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -275,12 +275,13 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
> >  	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
> >  	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
> >  		$(call MESSAGE,"Fixing package configuration files") ;\
> > -			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \
> > -				-e "s,$(STAGING_DIR), at STAGING_DIR@,g" \
> > +			$(SED)  "s,$(HOST_DIR), at HOST_DIR@,g" \
> > +				-e "s,$(BASE_DIR), at BASE_DIR@,g" \  
> 
> Previously, BASE_DIR was the very first thing replaced, but now it no
> longer is.

Correct.

> I'm not sure if I follow it all, but is the reason for that inversion
> hidden behhinde notes [0] and [1], being the need to create relative
> symlinks?

We're not creating symlinks here, we replacing absolute paths with
relative paths using the $(dirname $0) trick. But I guess that's what
you meant.

> As far as I understand, if BASE_DIR is kept as the first replacement,
> then we lose HOST_DIR as it (usually) is a subdir of BASE_DIR, so we end
> up with just @BASE_DIR@/host and thus can't easily replace it with
> relative paths anymore.

Exactly: if we replace $(BASE_DIR) by @BASE_DIR@, we can't easily
replace $(HOST_DIR) by @HOST_DIR@ anymore and then do the necessary
fixup.

The reason why the $(BASE_DIR) -> @BASE_DIR@ and then @BASE_DIR@ ->
$(BASE_DIR) replacement was added in commit
7701fc53d19be7cab8bddba72046a39e8421fd9e was to support the case where
build takes place in /usr.

The problem was that:

	s,-L/usr,-L at STAGING_DIR@/usr,g

would continuously re-add @STAGING_DIR@ if the build happens in a
sub-dir of /usr, because -L/usr will always match, even if the path has
already been fixed up.

The replacement of $(HOST_DIR) to @HOST_DIR@ before the $(BASE_DIR) to
@BASE_DIR@ replacement doesn't break this logic. If the path is in
$(HOST_DIR), it will have been replaced by @HOST_DIR@ and will not
match -L/usr, and then if the path is somewhere else in $(BASE_DIR), it
will be replaced by @BASE_DIR@, and the existing logic will continue to
work.

> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

So, I'll take your Reviewed-by, but let me know if you're still
unconvinced for some reason.

Thanks!

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-20 21:36   ` Yann E. MORIN
  2018-11-20 23:32     ` Arnout Vandecappelle
@ 2018-11-23 13:14     ` Thomas Petazzoni
  1 sibling, 0 replies; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-23 13:14 UTC (permalink / raw)
  To: buildroot

Hello Yann,

Thanks a lot for your extensive review!

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

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

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

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

	output/build/libglib2/libglib2-1.2.3/

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

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

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

or something like that.

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

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

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

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

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

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

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

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

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

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

Done.

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

Done.

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

True, done.

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

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

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

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

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

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-20 23:32     ` Arnout Vandecappelle
@ 2018-11-23 13:25       ` Thomas Petazzoni
  2018-11-23 15:44         ` Arnout Vandecappelle
  0 siblings, 1 reply; 20+ messages in thread
From: Thomas Petazzoni @ 2018-11-23 13:25 UTC (permalink / raw)
  To: buildroot

Hello Arnout,

Thanks for the review!

On Wed, 21 Nov 2018 00:32:11 +0100, Arnout Vandecappelle wrote:

> >>   output/per-package/busybox/target
> >>   output/per-package/busybox/host
> >>   output/per-package/host-fakeroot/target
> >>   output/per-package/host-fakeroot/host  
> > 
> > I'm not fond of the naming here...  
> 
>  Yay bikeshedding!

 :-)

> > In the end, can we expect to have a layout that would look like:
> > 
> >     output/build/busybox/host/
> >     output/build/busybox/target/
> >     output/build/busybox/busybox-1.2.3/     # Source tree, and
> >                                             # currently, build dir  
> 
>  If we're changing the world anyway, what about:
> 
> output/work/busybox-1.2.3/source
> output/work/busybox-1.2.3/host
> output/work/busybox-1.2.3/target
> output/work/busybox-1.2.3/build

You need two build folders: one for the target variant and one for the
host variant.

>  Or maybe even:
> 
> output/work/busybox-1.2.3/source
> output/work/busybox-1.2.3/dependencies/host
> output/work/busybox-1.2.3/dependencies/staging -> host/tuple/sysroot
> output/work/busybox-1.2.3/dependencies/target
> output/work/busybox-1.2.3/build

Also, I don't really like the word "work", that's what is used in OE
and I find it very fuzzy.

> >> +# $1: deps
> >> +ifeq ($(BR2_PER_PACKAGE_FOLDERS),y)
> >> +define prepare-per-package-folder  
> 
>  While we're bikeshedding: we have about 1000 occurences of "directory" and less
> than 30 of "folder".

I renamed to use "directory", both the macro, but also
BR2_PER_PACKAGE_FOLDERS is now BR2_PER_PACKAGE_DIRECTORIES.

> > The location where you define this macro, in the instrumentation hooks,
> > is not optimum I think. You even did not provide a help-comment for it.  
> 
>  At some point we started collecting this kind of thing in pkg-utils.mk. But I'm
> not sure if we're still doing that...

I moved the macro to pkg-utils.mk. It makes sense, because the macro is
also used in pkg-kconfig.mk (in a follow-up commit).

> > Why do you need two 'foreach' calls? Can't the following work?
> > 
> >     $(foreach pkg,$(1),\
> >         rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
> >             $(PER_PACKAGE_DIR)/$(pkg)/host/ \
> >             $(HOST_DIR)$(sep) \
> >         rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
> >             $(PER_PACKAGE_DIR)/$(pkg)/target/ \
> >             $(TARGET_DIR)$(sep))  
> 
>  I kind of like how we first build up all of host, and then all of target. But
> really, it doesn't make much of a difference.

Agreed, I also like the two loops.

>  More importantly, however: should we also sync the target directory for host
> packages? We *are* syncing the staging directory, of course...

Does it really matter ? A host package will only have other host
packages as its dependencies, so its per-package target directory will
always be empty. I did a test build, and all
output/per-package/host-*/target/ folders were empty.

So I am not sure it is really worth adding more conditionals in the
code just to avoid those empty target directories. But if you really
want that, I'll do it.

> >> +            [[ ${dir} =~ ${perpackagedir}/[^/]*/host/lib ]] && return  
>  The commit message doesn't explain why this is needed, and it took me some time
> to understand...

I updated the commit log with an explanation about this. It's obviously
not trivial, so it definitely deserves an explanation.

Best regards,

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target
  2018-11-23 13:25       ` Thomas Petazzoni
@ 2018-11-23 15:44         ` Arnout Vandecappelle
  0 siblings, 0 replies; 20+ messages in thread
From: Arnout Vandecappelle @ 2018-11-23 15:44 UTC (permalink / raw)
  To: buildroot



On 23/11/2018 14:25, Thomas Petazzoni wrote:
> Hello Arnout,
> 
> Thanks for the review!
> 
> On Wed, 21 Nov 2018 00:32:11 +0100, Arnout Vandecappelle wrote:
> 
>>>>   output/per-package/busybox/target
>>>>   output/per-package/busybox/host
>>>>   output/per-package/host-fakeroot/target
>>>>   output/per-package/host-fakeroot/host  
>>> I'm not fond of the naming here...  
>>  Yay bikeshedding!
>  :-)
> 
>>> In the end, can we expect to have a layout that would look like:
>>>
>>>     output/build/busybox/host/
>>>     output/build/busybox/target/
>>>     output/build/busybox/busybox-1.2.3/     # Source tree, and
>>>                                             # currently, build dir  
>>  If we're changing the world anyway, what about:
>>
>> output/work/busybox-1.2.3/source
>> output/work/busybox-1.2.3/host
>> output/work/busybox-1.2.3/target
>> output/work/busybox-1.2.3/build
>
> You need two build folders: one for the target variant and one for the
> host variant.

 Clearly not a good name then, because with "host" I meant the per-package host
directory, i.e. the target package's host directory. There would also be a

output/work/host-busybox-1.2.3/source
output/work/host-busybox-1.2.3/host
output/work/host-busybox-1.2.3/target
output/work/host-busybox-1.2.3/build

 Oh, but that would defeat the point of the source/build directory split...
Never mind then :-)


>>  Or maybe even:
>>
>> output/work/busybox-1.2.3/source
>> output/work/busybox-1.2.3/dependencies/host
>> output/work/busybox-1.2.3/dependencies/staging -> host/tuple/sysroot
>> output/work/busybox-1.2.3/dependencies/target
>> output/work/busybox-1.2.3/build
> Also, I don't really like the word "work", that's what is used in OE
> and I find it very fuzzy.

 That was the whole point, we should become OE! :-) BTW, OE also uses "tmp",
that's a lot better of course.

 But I see that in the end you kept the per-package directory. Reviewing all the
options, I agree that that is still the best one.

[snip]

>>  More importantly, however: should we also sync the target directory for host
>> packages? We *are* syncing the staging directory, of course...
> 
> Does it really matter ? A host package will only have other host
> packages as its dependencies, so its per-package target directory will
> always be empty. I did a test build, and all
> output/per-package/host-*/target/ folders were empty.
> 
> So I am not sure it is really worth adding more conditionals in the
> code just to avoid those empty target directories. But if you really
> want that, I'll do it.

 No, let's keep it as it is, it's not worth it.

 In fact, host packages may install stuff in target (e.g. host-gcc-final
installs libstdc++.so I think). So if a target package depends indirectly on
such a package through a different host package, we should still have its target
populated.


 Regards,
 Arnout

[snip]

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-11-23 15:44 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 16:35 [Buildroot] [PATCH next v5 0/9] Per-package host/target directory support Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 1/9] Makefile: evaluate CCACHE and HOST{CC, CXX} at time of use Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 2/9] support/scripts/check-host-rpath: split condition on two statements Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 3/9] Makefile: rework main directory creation logic Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 4/9] Makefile: move .NOTPARALLEL statement after including .config file Thomas Petazzoni
2018-11-20 20:18   ` Yann E. MORIN
2018-11-21 13:44     ` Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 5/9] Makefile: define TARGET_DIR_WARNING_FILE relative to TARGET_DIR Thomas Petazzoni
2018-11-20 20:32   ` Yann E. MORIN
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 6/9] package/pkg-generic: adjust config scripts tweaks for per-package folders Thomas Petazzoni
2018-11-20 20:53   ` Yann E. MORIN
2018-11-23 12:46     ` Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 7/9] core: implement per-package SDK and target Thomas Petazzoni
2018-11-20 21:36   ` Yann E. MORIN
2018-11-20 23:32     ` Arnout Vandecappelle
2018-11-23 13:25       ` Thomas Petazzoni
2018-11-23 15:44         ` Arnout Vandecappelle
2018-11-23 13:14     ` Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 8/9] package/pkg-generic: make libtool .la files compatible with per-package folders Thomas Petazzoni
2018-11-20 16:35 ` [Buildroot] [PATCH next v5 9/9] package/pkg-kconfig: handle KCONFIG_DEPENDENCIES " Thomas Petazzoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.