[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Danomi Manchego]

Also, don't ignore errors during installation.

Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>
---
 package/tiff/tiff.mk |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/tiff/tiff.mk b/package/tiff/tiff.mk
index 8d2e087..c61894b 100644
--- a/package/tiff/tiff.mk
+++ b/package/tiff/tiff.mk
@@ -76,7 +76,8 @@ ifneq ($(BR2_PACKAGE_TIFF_JBIG),y)
 endif
 
 define TIFF_INSTALL_TARGET_CMDS
-	-cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/
+	mkdir -p $(TARGET_DIR)/usr/lib/
+	cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/
 	for i in $(TIFF_TOOLS_LIST); \
 	do \
 		$(INSTALL) -m 755 -D $(@D)/tools/$$i $(TARGET_DIR)/usr/bin/$$i; \
-- 
1.7.9.5

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Thomas Petazzoni]

Dear Danomi Manchego,

On Thu, 11 Sep 2014 22:21:07 -0400, Danomi Manchego wrote:
> Also, don't ignore errors during installation.
> 
> Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>
> ---
>  package/tiff/tiff.mk |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Thomas Petazzoni]

Dear Danomi Manchego,

On Thu, 11 Sep 2014 22:21:07 -0400, Danomi Manchego wrote:
> Also, don't ignore errors during installation.
> 
> Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>
> ---
>  package/tiff/tiff.mk |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Could you explain which configuration produced an issue solved by this
patch? I tried a number of combinations (different toolchain
configurations, without C++, etc.) and I did not managed to produce the
issue.

Thanks,

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

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Danomi Manchego]

Thomas,

On Mon, Sep 22, 2014 at 1:25 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Danomi Manchego,
>
> On Thu, 11 Sep 2014 22:21:07 -0400, Danomi Manchego wrote:
>> Also, don't ignore errors during installation.
>>
>> Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>
>> ---
>>  package/tiff/tiff.mk |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Could you explain which configuration produced an issue solved by this
> patch? I tried a number of combinations (different toolchain
> configurations, without C++, etc.) and I did not managed to produce the
> issue.

I do not have a defconfig that demonstrates this.  I tend to encounter
these kind of error when I do the following trick to observe what
exactly is installed to the target directory:

    make tiff-rebuild TARGET_DIR=$(pwd)/TIFF
    tree TIFF

If I hit an error, they I figure that I'm just one mostly empty custom
skeleton and convoluted dependency graph away from hitting the error
in real life.  So then I submit a patch.

Danomi -

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Thomas Petazzoni]

Dear Danomi Manchego,

On Mon, 22 Sep 2014 14:19:03 -0400, Danomi Manchego wrote:

> I do not have a defconfig that demonstrates this.  I tend to encounter
> these kind of error when I do the following trick to observe what
> exactly is installed to the target directory:
> 
>     make tiff-rebuild TARGET_DIR=$(pwd)/TIFF
>     tree TIFF
> 
> If I hit an error, they I figure that I'm just one mostly empty custom
> skeleton and convoluted dependency graph away from hitting the error
> in real life.  So then I submit a patch.

Ok, thanks for the explanation. However, I'm wondering if we want to
support this. By doing this, you completely circumvent the preparation
work that is done by Buildroot: when you do make tiff-rebuild, only the
building and installation steps are re-executed with a different
TARGET_DIR. So any preparation work done before, such as directories
created by the global "dirs" target (on which all packages depend) will
not be done in your new TARGET_DIR.

Peter, Fabio, any opinion on this?

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

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

Hi,

 >> I do not have a defconfig that demonstrates this.  I tend to encounter
 >> these kind of error when I do the following trick to observe what
 >> exactly is installed to the target directory:
 >> 
 >> make tiff-rebuild TARGET_DIR=$(pwd)/TIFF
 >> tree TIFF
 >> 
 >> If I hit an error, they I figure that I'm just one mostly empty custom
 >> skeleton and convoluted dependency graph away from hitting the error
 >> in real life.  So then I submit a patch.

 > Ok, thanks for the explanation. However, I'm wondering if we want to
 > support this. By doing this, you completely circumvent the preparation
 > work that is done by Buildroot: when you do make tiff-rebuild, only the
 > building and installation steps are re-executed with a different
 > TARGET_DIR. So any preparation work done before, such as directories
 > created by the global "dirs" target (on which all packages depend) will
 > not be done in your new TARGET_DIR.

 > Peter, Fabio, any opinion on this?

I don't think it is something we should really officially support, but
the specific patch is ok as such, so I will commit it.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Peter Korsgaard]

>>>>> "Danomi" == Danomi Manchego <danomimanchego123@gmail.com> writes:

 > Also, don't ignore errors during installation.
 > Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>


 > ---
 >  package/tiff/tiff.mk |    3 ++-
 >  1 file changed, 2 insertions(+), 1 deletion(-)

 > diff --git a/package/tiff/tiff.mk b/package/tiff/tiff.mk
 > index 8d2e087..c61894b 100644
 > --- a/package/tiff/tiff.mk
 > +++ b/package/tiff/tiff.mk
 > @@ -76,7 +76,8 @@ ifneq ($(BR2_PACKAGE_TIFF_JBIG),y)
 >  endif
 
 >  define TIFF_INSTALL_TARGET_CMDS
 > -	-cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/
 > +	mkdir -p $(TARGET_DIR)/usr/lib/
 > +	cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/

Hmm, so what happens with a BR2_PREFER_STATIC_LIB=y build?

>>> tiff 4.0.3 Installing to target
mkdir -p /home/peko/source/buildroot/output/target/usr/lib/
cp -a /home/peko/source/buildroot/output/build/tiff-4.0.3/libtiff/.libs/libtiff.so* /home/peko/source/buildroot/output/target/usr/lib/
cp: cannot stat ?/home/peko/source/buildroot/output/build/tiff-4.0.3/libtiff/.libs/libtiff.so*?: No such file or directory
package/pkg-generic.mk:228: recipe for target '/home/peko/source/buildroot/output/build/tiff-4.0.3/.stamp_target_installed' failed
make: *** [/home/peko/source/buildroot/output/build/tiff-4.0.3/.stamp_target_installed] Error 1

Not good. Why are we not just using 'make install' and removing whatever
binaries / extras we don't want like we do for other packages?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Danomi Manchego]

Peter,

On Thu, Sep 25, 2014 at 5:10 AM, Peter Korsgaard <jacmet@uclibc.org> wrote:
>>>>>> "Danomi" == Danomi Manchego <danomimanchego123@gmail.com> writes:
>
>  > Also, don't ignore errors during installation.
>  > Signed-off-by: Danomi Manchego <danomimanchego123@gmail.com>
>
>
>  > ---
>  >  package/tiff/tiff.mk |    3 ++-
>  >  1 file changed, 2 insertions(+), 1 deletion(-)
>
>  > diff --git a/package/tiff/tiff.mk b/package/tiff/tiff.mk
>  > index 8d2e087..c61894b 100644
>  > --- a/package/tiff/tiff.mk
>  > +++ b/package/tiff/tiff.mk
>  > @@ -76,7 +76,8 @@ ifneq ($(BR2_PACKAGE_TIFF_JBIG),y)
>  >  endif
>
>  >  define TIFF_INSTALL_TARGET_CMDS
>  > -    -cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/
>  > +    mkdir -p $(TARGET_DIR)/usr/lib/
>  > +    cp -a $(@D)/libtiff/.libs/libtiff.so* $(TARGET_DIR)/usr/lib/
>
> Hmm, so what happens with a BR2_PREFER_STATIC_LIB=y build?
>
>>>> tiff 4.0.3 Installing to target
> mkdir -p /home/peko/source/buildroot/output/target/usr/lib/
> cp -a /home/peko/source/buildroot/output/build/tiff-4.0.3/libtiff/.libs/libtiff.so* /home/peko/source/buildroot/output/target/usr/lib/
> cp: cannot stat ?/home/peko/source/buildroot/output/build/tiff-4.0.3/libtiff/.libs/libtiff.so*?: No such file or directory
> package/pkg-generic.mk:228: recipe for target '/home/peko/source/buildroot/output/build/tiff-4.0.3/.stamp_target_installed' failed
> make: *** [/home/peko/source/buildroot/output/build/tiff-4.0.3/.stamp_target_installed] Error 1
>
> Not good. Why are we not just using 'make install' and removing whatever
> binaries / extras we don't want like we do for other packages?

Looking throught the git history of tiff.mk, it appears that the
current cp command is a direct descendent of the original
pre-AUTOTARGETS hand written makefile from 2007, which manually
installed the .so and the .a files.  The .a file installation was
dropped in 2008 in commit 9844a8ea2c24f50e3e102eca6562cd879196fcea
when the package was converted to AUTOTARGETS, with the manual cp
command instead of calling 'make install'.  I guess nobody who uses
the prefer-static feature uses tiff.  There are couple other commits
after that that modify the install commands, but the basic cp was
always maintained.  So I imagined that everyone before me just did
what I did - the minimum change to address the issue of the day.

I tried compiling tiff with the TIFF_INSTALL_TARGET_CMDS deleted, and
it seemed to work fine for my configuration - lots of new stuff
installed to /usr/bins, and .a, .la, .so + symlinks installed to
/usr/lib, and a bunch of documentation and man pages.  So, no obvious
disaster.

I could try spinning another patch to use the 'make install'.  But
there are 23 of those tiff bins installed by the default install.
Currently, we have kconfig enables for 2 of those 23.  Should there be
switches for all the other ones, or just a hard-coded deletion
intiff.mk, like libjpeg's bins?

Danomi -


> --
> Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] tiff: ensure directory exists before installing to it

[Peter Korsgaard]

>>>>> "Danomi" == Danomi Manchego <danomimanchego123@gmail.com> writes:

Hi,

 > I tried compiling tiff with the TIFF_INSTALL_TARGET_CMDS deleted, and
 > it seemed to work fine for my configuration - lots of new stuff
 > installed to /usr/bins, and .a, .la, .so + symlinks installed to
 > /usr/lib, and a bunch of documentation and man pages.  So, no obvious
 > disaster.

 > I could try spinning another patch to use the 'make install'.  But
 > there are 23 of those tiff bins installed by the default install.
 > Currently, we have kconfig enables for 2 of those 23.  Should there be
 > switches for all the other ones, or just a hard-coded deletion
 > intiff.mk, like libjpeg's bins?

Thanks. I would prefer to keep the behaviour as it is today (E.G. keep
the options for the 2 and delete everything else). If in the future
people need any of the remaining 21 programs then we can add options
then.

Thanks!

-- 
Bye, Peter Korsgaard