Skip to contentSkip to conversation

core/pkg-generic: check proper package installation

PATCH Superseded Yann E. MORIN 4 participants 11 messages Nov 6, 2015 – Nov 29, 2015

lore
Share & tools
lore

Yann E. MORIN

author
#0 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

---
Chamges v3 -> v4:
- don't use step hooks; do the checks in the command blocks (Arnout)

Changes v2 -> v3:
- also check for $(HOST_DIR) in case the user specified an out-of-build-dir location (Arnout)

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

---
 package/pkg-generic.mk | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
package/pkg-generic.mk +19 -0
index a5d0e57..82ec32e 100644
@@ -229,6 +229,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
+	$(call check-install-dirs,$(STAGING_DIR))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(call MESSAGE,"Fixing package configuration files") ;\
 			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \

@@ -275,6 +276,7 @@ $(BUILD_DIR)/%/.stamp_target_installed:
 	$(if $(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),\
 		$($(PKG)_INSTALL_INIT_SYSV))
 	$(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
+	$(call check-install-dirs,$(TARGET_DIR))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \
 	fi

@@ -286,6 +288,23 @@ $(BUILD_DIR)/%/.stamp_dircleaned:
 	rm -Rf $(@D)
 
 ################################################################################
+# check-install-dirs -- check that packages do not incorrectly install files
+# into incorrect locations
+#
+# argument 2 is the base directory to check for
+################################################################################
+define check-install-dirs
+	$(Q)if [ -d $(1)/$(HOST_DIR) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(HOST_DIR); \
+		exit 1; \
+	fi
+	$(Q)if [ -d $(1)/$(O) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \
+		exit 1; \
+	fi
+endef
+
+################################################################################
 # virt-provides-single -- check that provider-pkg is the declared provider for
 # the virtual package virt-pkg
 #
Signature · 1 line
1.9.1
#1 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Quoted 56 lines · Yann E. MORIN

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

---
Chamges v3 -> v4:
- don't use step hooks; do the checks in the command blocks (Arnout)

Changes v2 -> v3:
- also check for $(HOST_DIR) in case the user specified an out-of-build-dir location (Arnout)

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

---
 package/pkg-generic.mk | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
package/pkg-generic.mk +2 -0
index a5d0e57..82ec32e 100644
@@ -229,6 +229,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
+	$(call check-install-dirs,$(STAGING_DIR))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(call MESSAGE,"Fixing package configuration files") ;\
 			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \

@@ -275,6 +276,7 @@ $(BUILD_DIR)/%/.stamp_target_installed:
 	$(if $(BR2_INIT_SYSV)$(BR2_INIT_BUSYBOX),\
 		$($(PKG)_INSTALL_INIT_SYSV))
 	$(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
+	$(call check-install-dirs,$(TARGET_DIR))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \
 	fi

@@ -286,6 +288,23 @@ $(BUILD_DIR)/%/.stamp_dircleaned:
 	rm -Rf $(@D)

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 298

Quoted 10 lines
package/pkg-generic.mk +8 -0
 
 ################################################################################
+# check-install-dirs -- check that packages do not incorrectly install files
+# into incorrect locations
+#
+# argument 2 is the base directory to check for
+################################################################################
+define check-install-dirs
+	$(Q)if [ -d $(1)/$(HOST_DIR) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(HOST_DIR); \

Missing / between $(1) and $(HOST_DIR).

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 302

Quoted 4 lines
package/pkg-generic.mk +4 -0
+		exit 1; \
+	fi
+	$(Q)if [ -d $(1)/$(O) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \

Ditto.

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 304

Quoted 2 lines
package/pkg-generic.mk +2 -0
+		exit 1; \
+	fi

I think it's better without this duplication so with a second parameter and
calling the function twice.

Regards,
Arnout

Quoted 7 lines
package/pkg-generic.mk +3 -0
+endef
+
+################################################################################
 # virt-provides-single -- check that provider-pkg is the declared provider for
 # the virtual package virt-pkg
 #
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
author
#2 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Arnout, All,

On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:

Quoted 12 lines · Yann E. MORIN
Quoted 11 lines

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Quoted 8 lines
Quoted 8 lines
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

---

[--SNIP--]

Quoted 11 lines

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 298

Quoted 9 lines

################################################################################

package/pkg-generic.mk +8 -0
+# check-install-dirs -- check that packages do not incorrectly install files
+# into incorrect locations
+#
+# argument 2 is the base directory to check for
+################################################################################
+define check-install-dirs
+	$(Q)if [ -d $(1)/$(HOST_DIR) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(HOST_DIR); \

Missing / between $(1) and $(HOST_DIR).

Normally not needed, because $(HOST_DIR) and $(O) are absolute paths, so there is already a leading '/' .

So:
- I kept a separating '/' in the tests to be extra-ultra sure (but it arguably could be dropped),
- I did not add that extra '/' in the message so it is nicer when presented to the user.

Quoted 6 lines

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 302

Quoted 4 lines
package/pkg-generic.mk +4 -0
+		exit 1; \
+	fi
+	$(Q)if [ -d $(1)/$(O) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \

Ditto.

Ditto. ;-)

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		exit 1; \
+	fi

I think it's better without this duplication so with a second parameter and
calling the function twice.

Well, it's either duplication of the if-block, or duplication of the call sites. And I think it is better to have duplication of the if-blocks, in cas we need to check for mor eloactions, otherwise we'd have to duplicate even more the call sites...

Regards,
Yann E. MORIN.

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.  |
'------------------------------^-------^------------------^--------------------'
#3 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Quoted 21 lines · Yann E. MORIN

Arnout, All,

On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:

Quoted 18 lines · Yann E. MORIN
Quoted 17 lines

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

All my comments answered OK, so

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

Regards,
Arnout

Quoted 47 lines
Quoted 2 lines
Quoted 2 lines

---

[--SNIP--]

Quoted 11 lines

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 298

Quoted 9 lines

################################################################################

package/pkg-generic.mk +8 -0
+# check-install-dirs -- check that packages do not incorrectly install files
+# into incorrect locations
+#
+# argument 2 is the base directory to check for
+################################################################################
+define check-install-dirs
+	$(Q)if [ -d $(1)/$(HOST_DIR) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(HOST_DIR); \

Missing / between $(1) and $(HOST_DIR).

Normally not needed, because $(HOST_DIR) and $(O) are absolute paths, so there is already a leading '/' .

So:
- I kept a separating '/' in the tests to be extra-ultra sure (but it arguably could be dropped),
- I did not add that extra '/' in the message so it is nicer when presented to the user.

Quoted 6 lines

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_dircleaned: · line 302

Quoted 4 lines
package/pkg-generic.mk +4 -0
+		exit 1; \
+	fi
+	$(Q)if [ -d $(1)/$(O) ]; then \
+		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \

Ditto.

Ditto. ;-)

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		exit 1; \
+	fi

I think it's better without this duplication so with a second parameter and
calling the function twice.

Well, it's either duplication of the if-block, or duplication of the call sites. And I think it is better to have duplication of the if-blocks, in cas we need to check for mor eloactions, otherwise we'd have to duplicate even more the call sites...

Regards,
Yann E. MORIN.

Signature · 6 lines
Arnout Vandecappelle      arnout dot vandecappelle at essensium dot com
Senior Embedded Software Architect . . . . . . +32-478-010353 (mobile)
Essensium, Mind division . . . . . . . . . . . . . . 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
#4 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

In reply to Yann E. MORIN

Dear Yann,

Quoted 22 lines · Yann E. MORIN

Arnout, All,

On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:

Quoted 19 lines · Yann E. MORIN
Quoted 18 lines

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

[...]

Quoted 10 lines
Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		exit 1; \
+	fi

I think it's better without this duplication so with a second parameter and
calling the function twice.

Well, it's either duplication of the if-block, or duplication of the call sites. And I think it is better to have duplication of the if-blocks, in cas we need to check for mor eloactions, otherwise we'd have to duplicate even more the call sites...

Is it possible to make check-install-dirs (note the plural) just call multiple times check-install-dir (singular), passing a different path each time?

I'm sure you know all problems in computer science (except one) can be solved by adding another level of indirection! :-)

Obviously this can be a future improvement, especially useful in case the list of paths to check grew longer.

Signature · 1 line
Luca
#5 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Quoted 32 lines · Luca Ceresoli

Dear Yann,

Quoted 22 lines · Yann E. MORIN

Arnout, All,

On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:

Quoted 19 lines · Yann E. MORIN
Quoted 18 lines

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

[...]

Quoted 6 lines
Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+        exit 1; \
+    fi

I think it's better without this duplication so with a second parameter and
calling the function twice.

Quoted 8 lines
Quoted 4 lines

Well, it's either duplication of the if-block, or duplication of the call sites. And I think it is better to have duplication of the if-blocks, in cas we need to check for mor eloactions, otherwise we'd have to duplicate even more the call sites...

Is it possible to make check-install-dirs (note the plural) just call multiple times check-install-dir (singular), passing a different path each time?

That actually increases complexity and decreases readability compared to the
current duplication of the if-block, because you (as a reader) have to parse both macros and keep them in your head simultaneously to understand what's going on. In such a case, I do prefer a bit of duplication.

Regards,
Arnout

Quoted 7 lines

I'm sure you know all problems in computer science (except one) can be solved by adding another level of indirection! :-)

Obviously this can be a future improvement, especially useful in case the list of paths to check grew longer.

Signature · 6 lines
Arnout Vandecappelle      arnout dot vandecappelle at essensium dot com
Senior Embedded Software Architect . . . . . . +32-478-010353 (mobile)
Essensium, Mind division . . . . . . . . . . . . . . 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
#6 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

In reply to Yann E. MORIN

Yann,

Quoted 11 lines · Yann E. MORIN

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Quoted 6 lines
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>

On my side, I am not supper happy with the additional complexity added to the generic package infrastructure (yet again 19 lines added to pkg-generic.mk, to verify for a very specific type of failure).

Could we instead use TARGET_FINALIZE_HOOKS for this check (yes it means you wouldn't know which package has installed the files, but it's generally trivial to find from the name/path of the installed files) ?

Or alternatively, use the existing instrumentation hooks.

And then, regardless of the solution being used, put this stuff in a new .mk file (whose name shall be determined) which would ultimately contain the implementation of other sanity checks (for example the sanity check that all binaries are built for the correct target architecture).

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#7 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Thomas, All,

On 2015-11-29 18:58 +0100, Thomas Petazzoni spake thusly:

Quoted 19 lines · Yann E. MORIN
Quoted 11 lines

Some packages misbehave, and install files in either of;
- $(STAGING_DIR)/$(O) or $(TARGET_DIR)/$(O),
- $(STAGING_DIR)/$(HOST_DIR) or $(TARGET_DIR)/$(HOST_DIR).

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.

Add a check for those locations, as part of the command blocks for the target and staging installs.

Quoted 6 lines
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Gustavo Zacarias <[email protected]>
Cc Arnout Vandecappelle <[email protected]>
Cc Peter Seiderer <[email protected]>
Cc Romain Naour <[email protected]>
Quoted 7 lines

On my side, I am not supper happy with the additional complexity added to the generic package infrastructure (yet again 19 lines added to pkg-generic.mk, to verify for a very specific type of failure).

Could we instead use TARGET_FINALIZE_HOOKS for this check (yes it means you wouldn't know which package has installed the files, but it's generally trivial to find from the name/path of the installed files) ?

Well, I for one would prefer we fail right on the culprit package, rather than port-pone the check until the end. This way, it is obvious which package is the cuplrit.

Except we now have a file-> package mapping (thanks to your graph-size), so we could re-use that in a target-inalise hook, indeed.

Well, except maybe not... Can target-finalise be called before we have all the host packages (most notably the filesystem image generators)?

Quoted 1 line

Or alternatively, use the existing instrumentation hooks.

Arnout did not like that, hence why I put in the common install rule.

Of course, with my comment above, I agree we could postpone it up to target-finalise (if the minor nit above is answered negatively).

Quoted 5 lines

And then, regardless of the solution being used, put this stuff in a new .mk file (whose name shall be determined) which would ultimately contain the implementation of other sanity checks (for example the sanity check that all binaries are built for the correct target architecture).

OK. Thanks! :-)

Regards,
Yann E. MORIN.

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.  |
'------------------------------^-------^------------------^--------------------'
#8 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Hello,

Quoted 3 lines · Yann E. MORIN

Well, I for one would prefer we fail right on the culprit package, rather than port-pone the check until the end. This way, it is obvious which package is the cuplrit.

It's a matter of trade-off between the benefits and the additional complexity. IMO, the benefits of failing immediately on the culprit package are not that big. When we'll look at the incorrectly installed files, it will in 99% of the cases be obvious from which packages the files are coming, and in the 1% remaining cases, a simple "find" on a file with a name that isn't too generic will give us the answer.

Quoted 5 lines

Except we now have a file-> package mapping (thanks to your graph-size), so we could re-use that in a target-inalise hook, indeed.

Well, except maybe not... Can target-finalise be called before we have all the host packages (most notably the filesystem image generators)?

The dependencies of rootfs generators are added to PACKAGES, so they are built before target-finalize:

in fs/common.mk:

PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))

in Makefile:

target-finalize: $(PACKAGES)

Quoted 3 lines
Quoted 1 line

Or alternatively, use the existing instrumentation hooks.

Arnout did not like that, hence why I put in the common install rule.

What was Arnout reasoning?

Thanks,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#9 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

Thomas, All,

On 2015-11-29 19:27 +0100, Thomas Petazzoni spake thusly:

Quoted 11 lines · Yann E. MORIN
Quoted 3 lines

Well, I for one would prefer we fail right on the culprit package, rather than port-pone the check until the end. This way, it is obvious which package is the cuplrit.

It's a matter of trade-off between the benefits and the additional complexity. IMO, the benefits of failing immediately on the culprit package are not that big. When we'll look at the incorrectly installed files, it will in 99% of the cases be obvious from which packages the files are coming, and in the 1% remaining cases, a simple "find" on a file with a name that isn't too generic will give us the answer.

Yes, I understand. My reasoning is that, there is no reason to continue the build as soon as we know it is borked, so we do not loose time.

Quoted 9 lines
Quoted 5 lines

Except we now have a file-> package mapping (thanks to your graph-size), so we could re-use that in a target-inalise hook, indeed.

Well, except maybe not... Can target-finalise be called before we have all the host packages (most notably the filesystem image generators)?

The dependencies of rootfs generators are added to PACKAGES, so they are built before target-finalize:

Quoted 7 lines

in fs/common.mk:

PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))

in Makefile:

target-finalize: $(PACKAGES)

OK, so we can safely grab the list in target-finalize.

I'll update the patch accordingly, thanks! :-)

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Or alternatively, use the existing instrumentation hooks.

Arnout did not like that, hence why I put in the common install rule.

What was Arnout reasoning?

Quoting from:
http://lists.busybox.net/pipermail/buildroot/2015-November/143974.html

I think this would fit better directly in the .stamp_install_* rules rather than as hooks. My reasoning is that it is much more difficult to find out why some commands are being called when they're hidden in hooks. That's not too bad for things like gathering statistics since they're supposedly inobtrusive, but here you're throwing an error so it is really useful to be able to find easily in the source where this error comes from.

Regards,
Yann E. MORIN.

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.  |
'------------------------------^-------^------------------^--------------------'
#10 lore raw

PATCH v4 1/1 core/pkg-generic: check proper package installation

In reply to Thomas Petazzoni
Quoted 8 lines · Thomas Petazzoni

Hello,

Quoted 3 lines · Yann E. MORIN

Well, I for one would prefer we fail right on the culprit package, rather than port-pone the check until the end. This way, it is obvious which package is the cuplrit.

Quoted 6 lines

It's a matter of trade-off between the benefits and the additional complexity. IMO, the benefits of failing immediately on the culprit package are not that big. When we'll look at the incorrectly installed files, it will in 99% of the cases be obvious from which packages the files are coming, and in the 1% remaining cases, a simple "find" on a file with a name that isn't too generic will give us the answer.

And in addition, in 99% of the cases this failure will occur for a package that
was just added.

Quoted 16 lines
Quoted 6 lines

Except we now have a file-> package mapping (thanks to your graph-size), so we could re-use that in a target-inalise hook, indeed.

Well, except maybe not... Can target-finalise be called before we have all the host packages (most notably the filesystem image generators)?

The dependencies of rootfs generators are added to PACKAGES, so they are built before target-finalize:

in fs/common.mk:

PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))

in Makefile:

Quoted 7 lines

target-finalize: $(PACKAGES)

Quoted 3 lines
Quoted 1 line

Or alternatively, use the existing instrumentation hooks.

Arnout did not like that, hence why I put in the common install rule.

What was Arnout reasoning?

The hooks make it more difficult to find out where the commands that are being
executed come from. This makes it more difficult for an infra developer to understand what's going on exactly.

Your proposal of creating a new .mk file for the checks would significantly
decrease the impact of this issue, since in that case an infra developer knows to look in four places:

- the _CMDS definition in the package.mk;
- the default _CMDS definition in pkg-<buildsystem>.mk
- the %-rule in pkg-generic.mk;
- the hooks in pkg-checks.mk.

Four places is still worse than three, but we do need the hooks so we'll always
have those four places.

Regards,
Arnout

Quoted 5 lines

Thanks,

Thomas

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