[Buildroot] [PATCH 0/2 v2] core: check for improper install paths (branch yem/pkg-no-double-build-dir)

[Buildroot] [PATCH 0/2 v2] core: check for improper install paths (branch yem/pkg-no-double-build-dir)

[Yann E. MORIN]

Hello All!

URGENT! Please review...

This series adds a new post-step hook, that applies to both
install-target and install-staging, and checks that packages do not
install files in paths like:
    $(TARGET_DIR)/$(O)
    $(STAGING_DIR)/$(O)

Having one (or both) of those directories means that a package is
confusing DESTDIR, PREFIX and SYSROOT_DIR.

Such a confusion is currently caused by our host-pkgconf, that happilly
(but wrongly) prepends the sysroot directory to all paths it may return,
whether they be queried via one of --cflags, --cflags-only-I, --libs or
--libs-only-L, or whether they be directly queried via --variable=XXX.

Another source of confusion may come from generic-packages, wihch badly
coded custom Makefiles (even though I haven't noticed any so far, it is
bound to happen sooner or later...).

We want to detect such broken packages *ASAP* so we can fix them before
the release (or at best come up with a fallback plan).

Kown to break are quite a few packages from the Xorg stack, with more
than 15 detected so far, but maybe more since I had a contingency plan
to fix them in bulk, see previous RFC:
    http://lists.busybox.net/pipermail/buildroot/2015-November/143562.html

Also, some packages that want to call moc (from Qt4 for sure, possibly
from Qt5 too) also have a similar issue, see:
    http://lists.busybox.net/pipermail/buildroot/2015-October/141429.html

Although the second case is already causing build failures (and will
require a fix of its own), the first case has so far gone unnoticed (I
just stumbled on it by chance while reviewing the EFL bump series, which
is now kind of blocked by this issue).


Changes RFC -> v2:
  - only keep those two patches, drop the Xorg fixups.


Regards,
Yann E. MORIN.


The following changes since commit b8527eb3c017c74c861db8b21f8a0def55988382:

  poco: add patches to fix pcre link issue and library installation (2015-11-03 17:18:33 +0100)

are available in the git repository at:

  git://git.busybox.net/~ymorin/git/buildroot yem/pkg-no-double-build-dir

for you to fetch changes up to fe19a41362f17e9435b0da14097ce9fecda38780:

  core/pkg-generic: check proper package installation (2015-11-03 19:59:31 +0100)

----------------------------------------------------------------
Yann E. MORIN (2):
      core/pkg-generic: allow step hooks to fail a step
      core/pkg-generic: check proper package installation

 package/pkg-generic.mk | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

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

[Buildroot] [PATCH 1/2 v2] core/pkg-generic: allow step hooks to fail a step

[Yann E. MORIN]

Currently, we touch our stamp files before we call the step hooks.
This means a step hook can not properly fail a step, since the stamp
file exists even if the hook exits in error, thus a subsequent 'make'
would not try to redo that step.

Fix that by calling the hooks before we touch the stamp files.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
Cc: Peter Seiderer <ps.report@gmx.net>
Cc: Romain Naour <romain.naour@openwide.fr>
---
 package/pkg-generic.mk | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index ffef4d3..3db616c 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -125,8 +125,8 @@ $(BUILD_DIR)/%/.stamp_extracted:
 # some packages have messed up permissions inside
 	$(Q)chmod -R +rw $(@D)
 	$(foreach hook,$($(PKG)_POST_EXTRACT_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,extract)
+	$(Q)touch $@
 
 # Rsync the source directory if the <pkg>_OVERRIDE_SRCDIR feature is
 # used.
@@ -165,8 +165,8 @@ $(BUILD_DIR)/%/.stamp_patched:
 	done; \
 	)
 	$(foreach hook,$($(PKG)_POST_PATCH_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,patch)
+	$(Q)touch $@
 
 # Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
 $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
@@ -180,8 +180,8 @@ $(BUILD_DIR)/%/.stamp_configured:
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,configure)
+	$(Q)touch $@
 
 # Build
 $(BUILD_DIR)/%/.stamp_built::
@@ -190,8 +190,8 @@ $(BUILD_DIR)/%/.stamp_built::
 	$(foreach hook,$($(PKG)_PRE_BUILD_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_BUILD_CMDS)
 	$(foreach hook,$($(PKG)_POST_BUILD_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,build)
+	$(Q)touch $@
 
 # Install to host dir
 $(BUILD_DIR)/%/.stamp_host_installed:
@@ -200,8 +200,8 @@ $(BUILD_DIR)/%/.stamp_host_installed:
 	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,install-host)
+	$(Q)touch $@
 
 # Install to staging dir
 #
@@ -251,8 +251,8 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 				-e "s:@TOOLCHAIN_EXTERNAL_INSTALL_DIR@:$(TOOLCHAIN_EXTERNAL_INSTALL_DIR):g") \
 			-e "s:@STAGING_DIR@:$(STAGING_DIR):g" \
 			-e "s:@BASE_DIR@:$(BASE_DIR):g"
-	$(Q)touch $@
 	@$(call step_end,install-staging)
+	$(Q)touch $@
 
 # Install to images dir
 $(BUILD_DIR)/%/.stamp_images_installed:
@@ -261,8 +261,8 @@ $(BUILD_DIR)/%/.stamp_images_installed:
 	@$(call MESSAGE,"Installing to images directory")
 	+$($(PKG)_INSTALL_IMAGES_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_IMAGES_HOOKS),$(call $(hook))$(sep))
-	$(Q)touch $@
 	@$(call step_end,install-image)
+	$(Q)touch $@
 
 # Install to target dir
 $(BUILD_DIR)/%/.stamp_target_installed:
@@ -278,8 +278,8 @@ $(BUILD_DIR)/%/.stamp_target_installed:
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \
 	fi
-	$(Q)touch $@
 	@$(call step_end,install-target)
+	$(Q)touch $@
 
 # Remove package sources
 $(BUILD_DIR)/%/.stamp_dircleaned:
-- 
1.9.1

[Buildroot] [PATCH 1/2 v2] core/pkg-generic: allow step hooks to fail a step

[Arnout Vandecappelle]

On 03-11-15 20:06, Yann E. MORIN wrote:
> Currently, we touch our stamp files before we call the step hooks.
> This means a step hook can not properly fail a step, since the stamp
> file exists even if the hook exits in error, thus a subsequent 'make'
> would not try to redo that step.
> 
> Fix that by calling the hooks before we touch the stamp files.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Romain Naour <romain.naour@openwide.fr>

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

 Regards,
 Arnout

> ---
>  package/pkg-generic.mk | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

[snip]


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/2 v2] core/pkg-generic: allow step hooks to fail a step

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Tue,  3 Nov 2015 20:06:03 +0100, Yann E. MORIN wrote:
> Currently, we touch our stamp files before we call the step hooks.
> This means a step hook can not properly fail a step, since the stamp
> file exists even if the hook exits in error, thus a subsequent 'make'
> would not try to redo that step.
> 
> Fix that by calling the hooks before we touch the stamp files.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Romain Naour <romain.naour@openwide.fr>
> ---
>  package/pkg-generic.mk | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Applied, thanks.

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

[Buildroot] [PATCH 2/2 v2] core/pkg-generic: check proper package installation

[Yann E. MORIN]

Some packages misbehave, and install files in either $(STAGING_DIR)/$(O)
or in $(TARGET_DIR)/$(O)

One common reason for that is that pkgconf now prepends the sysroot path
to all the paths it returns. Other reasons vary, but are mostly due to
poorly writen generic-packages.

And a new step hooks to check that no file gets installed in either
location, called after the install-target and install-staging steps.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
Cc: Peter Seiderer <ps.report@gmx.net>
Cc: Romain Naour <romain.naour@openwide.fr>

---
Note that, in the current state of Buildroot, this *will* cause a lot of
build failures until all those packages are fixed. Among the known to
break are quite a few Xorg packages, plus many packages that need to
call moc (from Qt4 for sure, possibly from Qt5 too).
---
 package/pkg-generic.mk | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 3db616c..59b0938 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -87,6 +87,21 @@ define step_pkg_size
 endef
 GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size
 
+define step_check_build_dir_one
+	if [ -d $(2) ]; then \
+		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
+		exit 1; \
+	fi
+endef
+
+define step_check_build_dir
+	$(if $(filter install-staging,$(2)),\
+		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(STAGING_DIR)/$(O))))
+	$(if $(filter install-target,$(2)),\
+		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(TARGET_DIR)/$(O))))
+endef
+GLOBAL_INSTRUMENTATION_HOOKS += step_check_build_dir
+
 # User-supplied script
 ifneq ($(BR2_INSTRUMENTATION_SCRIPTS),)
 define step_user
-- 
1.9.1

[Buildroot] [PATCH 2/2 v2] core/pkg-generic: check proper package installation

[Arnout Vandecappelle]

On 03-11-15 20:06, Yann E. MORIN wrote:
> Some packages misbehave, and install files in either $(STAGING_DIR)/$(O)
> or in $(TARGET_DIR)/$(O)
> 
> One common reason for that is that pkgconf now prepends the sysroot path
> to all the paths it returns. Other reasons vary, but are mostly due to
> poorly writen generic-packages.
> 
> And a new step hooks to check that no file gets installed in either
> location, called after the install-target and install-staging steps.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Romain Naour <romain.naour@openwide.fr>
> 
> ---
> Note that, in the current state of Buildroot, this *will* cause a lot of
> build failures until all those packages are fixed. Among the known to
> break are quite a few Xorg packages, plus many packages that need to
> call moc (from Qt4 for sure, possibly from Qt5 too).

 Well, the latter will not cause _new_ build failures, because there the problem
is that the moc path returned by pkgconfig is incorrect, while it is installed
in the right place.


 There are a few things in here that I don't like, but I don't want to block the
urgency.

> ---
>  package/pkg-generic.mk | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 3db616c..59b0938 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -87,6 +87,21 @@ define step_pkg_size
>  endef
>  GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size
>  
> +define step_check_build_dir_one
> +	if [ -d $(2) ]; then \
> +		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
> +		exit 1; \
> +	fi

 I think this could also be

	$(if $(wildcard $(2)),$(error $(1) installs files in $(2)))

but not sure if that is better readable.

> +endef
> +
> +define step_check_build_dir
> +	$(if $(filter install-staging,$(2)),\
> +		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(STAGING_DIR)/$(O))))

 Is this correct? Shouldn't that be $(HOST_DIR) instead of $(O)? If a custom
BR2_HOST_DIR is configured, then the wrong pkgconfig stuff will put stuff into
$(STAGING_DIR)/$(HOST_DIR), no?


> +	$(if $(filter install-target,$(2)),\
> +		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(TARGET_DIR)/$(O))))

 Actually, I wonder if it shouldn't be HOST_DIR here as well.

 Perhaps, to be sure, both should be checked.

> +endef
> +GLOBAL_INSTRUMENTATION_HOOKS += step_check_build_dir

 I don't see much point to add this as instrumentation hooks, it could be done
directly in the .stamp rule.


Note that this patch will make it impossible to have $(O) equal to some path
that happens to exist on the target, e.g. O=/tmp/build if /tmp/build happens to
exist in the custom skeleton or rootfs overlay. (Same story with HOST_DIR of O
is replaced by HOST_DIR above). Granted, it's not very likely.


 Regards,
 Arnout

> +
>  # User-supplied script
>  ifneq ($(BR2_INSTRUMENTATION_SCRIPTS),)
>  define step_user
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2 v2] core/pkg-generic: check proper package installation

[Yann E. MORIN]

Arnout, All,

On 2015-11-03 23:31 +0100, Arnout Vandecappelle spake thusly:
> On 03-11-15 20:06, Yann E. MORIN wrote:
> > Some packages misbehave, and install files in either $(STAGING_DIR)/$(O)
> > or in $(TARGET_DIR)/$(O)
> > 
> > One common reason for that is that pkgconf now prepends the sysroot path
> > to all the paths it returns. Other reasons vary, but are mostly due to
> > poorly writen generic-packages.
> > 
> > And a new step hooks to check that no file gets installed in either
> > location, called after the install-target and install-staging steps.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> > Cc: Peter Seiderer <ps.report@gmx.net>
> > Cc: Romain Naour <romain.naour@openwide.fr>
> > 
> > ---
> > Note that, in the current state of Buildroot, this *will* cause a lot of
> > build failures until all those packages are fixed. Among the known to
> > break are quite a few Xorg packages, plus many packages that need to
> > call moc (from Qt4 for sure, possibly from Qt5 too).
> 
>  Well, the latter will not cause _new_ build failures, because there the problem
> is that the moc path returned by pkgconfig is incorrect, while it is installed
> in the right place.

Right. That would not be new failures. We would not even catch it with
this patch, since moc *is* installed in the correct place. We would just
notice when a moc-using package tries to use (like the failure noticed
by Peter S.).

>  There are a few things in here that I don't like, but I don't want to block the
> urgency.
> 
> > ---
> >  package/pkg-generic.mk | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 3db616c..59b0938 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -87,6 +87,21 @@ define step_pkg_size
> >  endef
> >  GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size
> >  
> > +define step_check_build_dir_one
> > +	if [ -d $(2) ]; then \
> > +		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
> > +		exit 1; \
> > +	fi
> 
>  I think this could also be
> 
> 	$(if $(wildcard $(2)),$(error $(1) installs files in $(2)))
> 
> but not sure if that is better readable.

Well, I'm always a bit uneasy at using a Makefile construct in a place
where commands are expected (i.e. part of a rule).

So I'd prefer we stick to using shell code here. And as you said, it is
not muh mor ereadable.

> > +endef
> > +
> > +define step_check_build_dir
> > +	$(if $(filter install-staging,$(2)),\
> > +		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(STAGING_DIR)/$(O))))
> 
>  Is this correct? Shouldn't that be $(HOST_DIR) instead of $(O)? If a custom
> BR2_HOST_DIR is configured, then the wrong pkgconfig stuff will put stuff into
> $(STAGING_DIR)/$(HOST_DIR), no?

Hmmm... It looks like you are right. I'll do a few tests here, with
various values for HOST_DIR (in $(O) and elsewhere) and wee what we get
as a result.

Still, we may also want to catch packages that use PREFIX=$(TARGET_DIR)
and DESTDIR=$(TARGET_DIR) which would ultimately lead to
$(TARGET_DIR)/$(TARGET_DIR)  (and since $(TARGET_DIR) is a sub-dir of
$(O), we'd catch that as well.

So I think we should check both XXX/$(O) and XXX/$(HOST_DIR) in both the
staging and target installs.

> > +	$(if $(filter install-target,$(2)),\
> > +		$(if $(filter end,$(1)),$(call step_check_build_dir_one,$(3),$(TARGET_DIR)/$(O))))
> 
>  Actually, I wonder if it shouldn't be HOST_DIR here as well.

Ditto.

>  Perhaps, to be sure, both should be checked.

Yes.

> > +endef
> > +GLOBAL_INSTRUMENTATION_HOOKS += step_check_build_dir
> 
>  I don't see much point to add this as instrumentation hooks, it could be done
> directly in the .stamp rule.
> 
> 
> Note that this patch will make it impossible to have $(O) equal to some path
> that happens to exist on the target, e.g. O=/tmp/build if /tmp/build happens to
> exist in the custom skeleton or rootfs overlay. (Same story with HOST_DIR of O
> is replaced by HOST_DIR above). Granted, it's not very likely.

Hmm.. That's indeed a bit annoying...

So, if we have HOST_DIR=/tmp then the tests above are broken indeed...
OK, I'll investigate more...

Thanks! :-)

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> > +
> >  # User-supplied script
> >  ifneq ($(BR2_INSTRUMENTATION_SCRIPTS),)
> >  define step_user
> > 
> 
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

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