[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[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" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Peter Seiderer <ps.report@gmx.net>
Cc: Romain Naour <romain.naour@openwide.fr>

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

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index a5d0e57..82ec32e 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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
 #
-- 
1.9.1

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Arnout Vandecappelle]

On 06-11-15 20:15, Yann E. MORIN wrote:
> 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" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Romain Naour <romain.naour@openwide.fr>
> 
> ---
> 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(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a5d0e57..82ec32e 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -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); \

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

> +		exit 1; \
> +	fi
> +	$(Q)if [ -d $(1)/$(O) ]; then \
> +		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \

 Ditto.

> +		exit 1; \
> +	fi

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

 Regards,
 Arnout

> +endef
> +
> +################################################################################
>  # virt-provides-single -- check that provider-pkg is the declared provider for
>  # the virtual package virt-pkg
>  #
> 


-- 
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] [PATCHv4] core/pkg-generic: check proper package installation

[Yann E. MORIN]

Arnout, All,

On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:
> On 06-11-15 20:15, Yann E. MORIN wrote:
> > 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" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> > Cc: Peter Seiderer <ps.report@gmx.net>
> > Cc: Romain Naour <romain.naour@openwide.fr>
> > 
> > ---
[--SNIP--]
> >  ################################################################################
> > +# 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.

> > +		exit 1; \
> > +	fi
> > +	$(Q)if [ -d $(1)/$(O) ]; then \
> > +		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \
> 
>  Ditto.

Ditto. ;-)

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

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCHv4] core/pkg-generic: check proper package installation

[Arnout Vandecappelle]

On 07-11-15 00:07, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:
>> On 06-11-15 20:15, Yann E. MORIN wrote:
>>> 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" <yann.morin.1998@free.fr>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
>>> Cc: Arnout Vandecappelle <arnout@mind.be>
>>> Cc: Peter Seiderer <ps.report@gmx.net>
>>> Cc: Romain Naour <romain.naour@openwide.fr>

 All my comments answered OK, so

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


 Regards,
 Arnout

>>>
>>> ---
> [--SNIP--]
>>>  ################################################################################
>>> +# 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.
> 
>>> +		exit 1; \
>>> +	fi
>>> +	$(Q)if [ -d $(1)/$(O) ]; then \
>>> +		printf "ERROR: package %s installs files in %s\n" $($(PKG)_NAME) $(1)$(O); \
>>
>>  Ditto.
> 
> Ditto. ;-)
> 
>>> +		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.
> 

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

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> Arnout, All,
>
> On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:
>> On 06-11-15 20:15, Yann E. MORIN wrote:
>>> 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" <yann.morin.1998@free.fr>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
>>> Cc: Arnout Vandecappelle <arnout@mind.be>
>>> Cc: Peter Seiderer <ps.report@gmx.net>
>>> Cc: Romain Naour <romain.naour@openwide.fr>
>>>
[...]
>>> +		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.

-- 
Luca

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Arnout Vandecappelle]

On 09-11-15 14:20, Luca Ceresoli wrote:
> Dear Yann,
> 
> Yann E. MORIN wrote:
>> Arnout, All,
>>
>> On 2015-11-06 23:55 +0100, Arnout Vandecappelle spake thusly:
>>> On 06-11-15 20:15, Yann E. MORIN wrote:
>>>> 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" <yann.morin.1998@free.fr>
>>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>>> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
>>>> Cc: Arnout Vandecappelle <arnout@mind.be>
>>>> Cc: Peter Seiderer <ps.report@gmx.net>
>>>> Cc: Romain Naour <romain.naour@openwide.fr>
>>>>
> [...]
>>>> +        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?

 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


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

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

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Thomas Petazzoni]

Yann,

On Fri,  6 Nov 2015 20:15:30 +0100, Yann E. MORIN wrote:
> 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" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Romain Naour <romain.naour@openwide.fr>

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Yann E. MORIN]

Thomas, All,

On 2015-11-29 18:58 +0100, Thomas Petazzoni spake thusly:
> On Fri,  6 Nov 2015 20:15:30 +0100, Yann E. MORIN wrote:
> > 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" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Gustavo Zacarias <gustavo@zacarias.com.ar>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> > Cc: Peter Seiderer <ps.report@gmx.net>
> > Cc: Romain Naour <romain.naour@openwide.fr>
> 
> 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)?

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

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

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCHv4] core/pkg-generic: check proper package installation

[Thomas Petazzoni]

Hello,

On Sun, 29 Nov 2015 19:10:07 +0100, Yann E. MORIN wrote:

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

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

> > 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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCHv4] core/pkg-generic: check proper package installation

[Yann E. MORIN]

Thomas, All,

On 2015-11-29 19:27 +0100, Thomas Petazzoni spake thusly:
> On Sun, 29 Nov 2015 19:10:07 +0100, Yann E. MORIN wrote:
> > 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.

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

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

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

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

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCHv4] core/pkg-generic: check proper package installation

[Arnout Vandecappelle]

On 29-11-15 19:27, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 29 Nov 2015 19:10:07 +0100, Yann E. MORIN wrote:
> 
>> 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.

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

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

> 
> Thanks,
> 
> Thomas
> 


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