core: check for improper install paths (branch yem/pkg-no-double-build-dir)

PATCH New Yann E. MORIN 3 participants 7 messages Nov 3, 2015 – Nov 4, 2015

lore

Share & tools

lore

Yann E. MORIN

author

2015-11-03 19:05 UTC #0 lore raw

v2 cover core: check for improper install paths (branch yem/pkg-no-double-build-dir)

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(-)

Signature · 6 lines

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

Yann E. MORIN

author

2015-11-03 19:06 UTC #1 lore raw

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

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" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Gustavo Zacarias <[email protected]>

Cc Peter Seiderer <[email protected]>

Cc Romain Naour <[email protected]>

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

package/pkg-generic.mk +8 -8

index ffef4d3..3db616c 100644
@@ -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:

Signature · 1 line

1.9.1

Arnout Vandecappelle

2015-11-03 22:07 UTC #2 lore raw

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

Quoted 5 lines · 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.

Quoted 7 lines

Fix that by calling the hooks before we touch the stamp files.
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Regards,
Arnout

Quoted 3 lines

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

[snip]

Signature · 6 lines

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

Thomas Petazzoni

Applied

2015-11-04 13:20 UTC #3 lore raw

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

In reply to Yann E. MORIN

Dear Yann E. MORIN,

Quoted 7 lines · 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.

Quoted 8 lines

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>
---
 package/pkg-generic.mk | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Applied, thanks.

Thomas

Signature · 3 lines

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

Yann E. MORIN

author

2015-11-03 19:06 UTC #4 lore raw

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

In reply to 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" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Gustavo Zacarias <[email protected]>

Cc Peter Seiderer <[email protected]>

Cc Romain Naour <[email protected]>

---
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(+)

package/pkg-generic.mk +15 -0

index 3db616c..59b0938 100644
@@ -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

Signature · 1 line

1.9.1

Arnout Vandecappelle

2015-11-03 22:31 UTC #5 lore raw

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

Quoted 16 lines · 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" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

Quoted 5 lines

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

Quoted 9 lines

---
 package/pkg-generic.mk | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

package/pkg-generic.mk +0 -0

index 3db616c..59b0938 100644

package/pkg-generic.mk define step_pkg_size · line 94

Quoted 8 lines

package/pkg-generic.mk +5 -0

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

package/pkg-generic.mk define step_pkg_size · line 99

Quoted 5 lines

package/pkg-generic.mk +5 -0

+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?

package/pkg-generic.mk define step_pkg_size · line 101

Quoted 2 lines

package/pkg-generic.mk +2 -0

+	$(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.

package/pkg-generic.mk define step_pkg_size · line 103

Quoted 2 lines

package/pkg-generic.mk +2 -0

+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

Quoted 5 lines

package/pkg-generic.mk +1 -0

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

Signature · 6 lines

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

author

2015-11-04 17:19 UTC #6 lore raw

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

Arnout, All,

On 2015-11-03 23:31 +0100, Arnout Vandecappelle spake thusly:

Quoted 23 lines · Yann E. MORIN

Quoted 16 lines
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" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>
Quoted 5 lines
---
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).

Quoted 3 lines

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

Quoted 23 lines

There are a few things in here that I don't like, but I don't want to block the
urgency.
Quoted 9 lines
---
 package/pkg-generic.mk | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
package/pkg-generic.mk +0 -0
index 3db616c..59b0938 100644
package/pkg-generic.mk define step_pkg_size · line 94
Quoted 8 lines
package/pkg-generic.mk +5 -0
@@ -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

Quoted 3 lines

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

Quoted 9 lines

package/pkg-generic.mk define step_pkg_size · line 99
Quoted 5 lines
package/pkg-generic.mk +5 -0
+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.

Quoted 4 lines

package/pkg-generic.mk define step_pkg_size · line 101
Quoted 2 lines
package/pkg-generic.mk +2 -0
+	$(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.

Quoted 1 line

Perhaps, to be sure, both should be checked.

Yes.

Quoted 11 lines

package/pkg-generic.mk define step_pkg_size · line 103
Quoted 2 lines
package/pkg-generic.mk +2 -0
+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.

Quoted 17 lines

Regards,
Arnout

Quoted 5 lines

package/pkg-generic.mk +1 -0

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

Signature · 6 lines

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

Signature · 6 lines

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