[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Jörg Krause]

The yajl build system contains a race condition, which gets triggered by
high BR2_JLEVEL settings - It tries to link the executable gen-extra-close
against libyajl before it is created:

[ 21%] Linking C executable gen-extra-close
[ 26%] Building C object src/CMakeFiles/yajl_s.dir/yajl_buf.c.o
/home/test/autobuild/instance-3/output/host/opt/ext-toolchain/bfin-uclinux/bfin-uclinux/bin/ld.real: cannot find -lyajl

Fix it by linking against the static library libyajl_s like all other targets of
the yajl build system do.

Fixes:
http://autobuild.buildroot.net/results/a9b/a9b5209377acb51e69f376e0c008ee71fe00397a/
http://autobuild.buildroot.net/results/55f/55fe22463d49addb42b635d10be5176522f4a561/
http://autobuild.buildroot.net/results/808/808acca0cfed93465845c2aa055a7a4fc56a8a17/
http://autobuild.buildroot.net/results/b92/b92a9c84b71a8a2d022d307245ca6be36a000e6c/
http://autobuild.buildroot.net/results/55d/55df698ab53f7d94235166e8576eb681ed68668e/
.. and more.

Upstream status: Pending
https://github.com/lloyd/yajl/pull/187

Signed-off-by: J?rg Krause <joerg.krause@embedded.rocks>
---
 ...make-Fix-static-linking-in-parallel-build.patch | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 package/yajl/0003-cmake-Fix-static-linking-in-parallel-build.patch

diff --git a/package/yajl/0003-cmake-Fix-static-linking-in-parallel-build.patch b/package/yajl/0003-cmake-Fix-static-linking-in-parallel-build.patch
new file mode 100644
index 0000000..793e38f
--- /dev/null
+++ b/package/yajl/0003-cmake-Fix-static-linking-in-parallel-build.patch
@@ -0,0 +1,39 @@
+From 24cbb6b482315d7659c04c9470ec9029e825e8ca Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?J=C3=B6rg=20Krause?= <joerg.krause@embedded.rocks>
+Date: Fri, 1 Apr 2016 20:45:35 +0200
+Subject: [PATCH] cmake: Fix static linking in parallel build
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Building yajl in a static context fails in a parallel build:
+
+[ 21%] Linking C executable gen-extra-close
+[ 26%] Building C object src/CMakeFiles/yajl_s.dir/yajl_buf.c.o
+/home/test/autobuild/instance-3/output/host/opt/ext-toolchain/bfin-uclinux/bfin-uclinux/bin/ld.real: cannot find -lyajl
+
+Fix this by linking against the static library yajl_s as all other targets
+in yajl does.
+
+Upstream status: Pending
+https://github.com/lloyd/yajl/pull/187
+
+Signed-off-by: J?rg Krause <joerg.krause@embedded.rocks>
+---
+ test/api/CMakeLists.txt | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/test/api/CMakeLists.txt b/test/api/CMakeLists.txt
+index e0ce2f6..efc85c5 100644
+--- a/test/api/CMakeLists.txt
++++ b/test/api/CMakeLists.txt
+@@ -21,5 +21,5 @@ LINK_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR}/../../${YAJL_DIST_NAME}/lib)
+ FOREACH (test ${TESTS})
+   GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
+   ADD_EXECUTABLE(${testProg} ${test})
+-  TARGET_LINK_LIBRARIES(${testProg} yajl m)
++  TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
+ ENDFOREACH()
+-- 
+2.8.0
+
-- 
2.8.0

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Thomas Petazzoni]

Hello,

On Fri,  1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:

> + FOREACH (test ${TESTS})
> +   GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
> +   ADD_EXECUTABLE(${testProg} ${test})
> +-  TARGET_LINK_LIBRARIES(${testProg} yajl m)
> ++  TARGET_LINK_LIBRARIES(${testProg} yajl_s m)

I am not sure to understand why this would fix the parallel build
issue. The parallel build problem comes from the fact that the library
and tests sit in different directories, and that they are built in
parallel with each other. Both the yajl and yajl_s libraries are built
in the same directory, separate from the tests directory. So I don't
quite see how changing from using yajl to yajl_s is going to make a
difference here. But maybe I'm missing something.

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

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Jörg Krause]

On Sa, 2016-04-02 at 17:13 +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri,??1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:
> 
> > 
> > + FOREACH (test ${TESTS})
> > +???GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
> > +???ADD_EXECUTABLE(${testProg} ${test})
> > +-??TARGET_LINK_LIBRARIES(${testProg} yajl m)
> > ++??TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
> I am not sure to understand why this would fix the parallel build
> issue. The parallel build problem comes from the fact that the
> library
> and tests sit in different directories, and that they are built in
> parallel with each other. Both the yajl and yajl_s libraries are
> built
> in the same directory, separate from the tests directory. So I don't
> quite see how changing from using yajl to yajl_s is going to make a
> difference here. But maybe I'm missing something.

The issue is that we patch yajl (#0002) to build only the static
library in a static build. So there is no shared library target for
CMake to use as a dependency.

J?rg

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Samuel Martin]

On Sat, Apr 2, 2016 at 6:29 PM, J?rg Krause <joerg.krause@embedded.rocks> wrote:
> On Sa, 2016-04-02 at 17:13 +0200, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Fri,  1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:
>>
>> >
>> > + FOREACH (test ${TESTS})
>> > +   GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
>> > +   ADD_EXECUTABLE(${testProg} ${test})
>> > +-  TARGET_LINK_LIBRARIES(${testProg} yajl m)
>> > ++  TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
>> I am not sure to understand why this would fix the parallel build
>> issue. The parallel build problem comes from the fact that the
>> library
>> and tests sit in different directories, and that they are built in
>> parallel with each other. Both the yajl and yajl_s libraries are
>> built
>> in the same directory, separate from the tests directory. So I don't
>> quite see how changing from using yajl to yajl_s is going to make a
>> difference here. But maybe I'm missing something.
>
> The issue is that we patch yajl (#0002) to build only the static
> library in a static build. So there is no shared library target for
> CMake to use as a dependency.

hmm... with that explaination, I nack this.
IMHO, the correct way to fix this would rather be:

IF(BUILD_SHARED_LIBS)
  TARGET_LINK_LIBRARIES(${testProg} yajl m)
ELSE()
  TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
ENDIF()

Regards,

-- 
Samuel

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Jörg Krause]

On Sa, 2016-04-02 at 19:05 +0200, Samuel Martin wrote:
> On Sat, Apr 2, 2016 at 6:29 PM, J?rg Krause <joerg.krause@embedded.ro
> cks> wrote:
> > 
> > On Sa, 2016-04-02 at 17:13 +0200, Thomas Petazzoni wrote:
> > > 
> > > Hello,
> > > 
> > > On Fri,??1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:
> > > 
> > > > 
> > > > 
> > > > + FOREACH (test ${TESTS})
> > > > +???GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
> > > > +???ADD_EXECUTABLE(${testProg} ${test})
> > > > +-??TARGET_LINK_LIBRARIES(${testProg} yajl m)
> > > > ++??TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
> > > I am not sure to understand why this would fix the parallel build
> > > issue. The parallel build problem comes from the fact that the
> > > library
> > > and tests sit in different directories, and that they are built
> > > in
> > > parallel with each other. Both the yajl and yajl_s libraries are
> > > built
> > > in the same directory, separate from the tests directory. So I
> > > don't
> > > quite see how changing from using yajl to yajl_s is going to make
> > > a
> > > difference here. But maybe I'm missing something.
> > The issue is that we patch yajl (#0002) to build only the static
> > library in a static build. So there is no shared library target for
> > CMake to use as a dependency.
> hmm... with that explaination, I nack this.
> IMHO, the correct way to fix this would rather be:
> 
> IF(BUILD_SHARED_LIBS)
> ? TARGET_LINK_LIBRARIES(${testProg} yajl m)
> ELSE()
> ? TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
> ENDIF()
> 

Yes, this would probably be best. However, the static library is build
in shared and static builds.

J?rg

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Samuel Martin]

On Sat, Apr 2, 2016 at 7:11 PM, J?rg Krause <joerg.krause@embedded.rocks> wrote:
> On Sa, 2016-04-02 at 19:05 +0200, Samuel Martin wrote:
>> On Sat, Apr 2, 2016 at 6:29 PM, J?rg Krause <joerg.krause@embedded.ro
>> cks> wrote:
>> >
>> > On Sa, 2016-04-02 at 17:13 +0200, Thomas Petazzoni wrote:
>> > >
>> > > Hello,
>> > >
>> > > On Fri,  1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:
>> > >
>> > > >
>> > > >
>> > > > + FOREACH (test ${TESTS})
>> > > > +   GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
>> > > > +   ADD_EXECUTABLE(${testProg} ${test})
>> > > > +-  TARGET_LINK_LIBRARIES(${testProg} yajl m)
>> > > > ++  TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
>> > > I am not sure to understand why this would fix the parallel build
>> > > issue. The parallel build problem comes from the fact that the
>> > > library
>> > > and tests sit in different directories, and that they are built
>> > > in
>> > > parallel with each other. Both the yajl and yajl_s libraries are
>> > > built
>> > > in the same directory, separate from the tests directory. So I
>> > > don't
>> > > quite see how changing from using yajl to yajl_s is going to make
>> > > a
>> > > difference here. But maybe I'm missing something.
>> > The issue is that we patch yajl (#0002) to build only the static
>> > library in a static build. So there is no shared library target for
>> > CMake to use as a dependency.
>> hmm... with that explaination, I nack this.
>> IMHO, the correct way to fix this would rather be:
>>
>> IF(BUILD_SHARED_LIBS)
>>   TARGET_LINK_LIBRARIES(${testProg} yajl m)
>> ELSE()
>>   TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
>> ENDIF()
>>
>
> Yes, this would probably be best. However, the static library is build
> in shared and static builds.

I'm aware of that. Unfortunately, CMake is not equivalent to autotools
on that point.
CMake only provide one boolean flag to control the shared libs. build,
instead of a tristate option allowing building: shared libs only, or
static libs only, or shared and static libs.
That's why I made the choice of always building the static libs, and
keep BUILD_SHARED_LIBS driving the shared libs. build.

Though the static libs are always built, if the shared libs are built
as well, I'd tend to use the *.so at least to reduce the footprint...

Side note, this is breaking test. We usually don't care much about
building tests...

Regards,

-- 
Samuel

[Buildroot] [PATCH] yajl: fix static linking in a parallel build

[Jörg Krause]

On Sa, 2016-04-02 at 19:43 +0200, Samuel Martin wrote:
> On Sat, Apr 2, 2016 at 7:11 PM, J?rg Krause <joerg.krause@embedded.ro
> cks> wrote:
> > 
> > On Sa, 2016-04-02 at 19:05 +0200, Samuel Martin wrote:
> > > 
> > > On Sat, Apr 2, 2016 at 6:29 PM, J?rg Krause <joerg.krause@embedde
> > > d.ro
> > > cks> wrote:
> > > > 
> > > > 
> > > > On Sa, 2016-04-02 at 17:13 +0200, Thomas Petazzoni wrote:
> > > > > 
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > On Fri,??1 Apr 2016 21:18:12 +0200, J?rg Krause wrote:
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > + FOREACH (test ${TESTS})
> > > > > > +???GET_FILENAME_COMPONENT(testProg ${test} NAME_WE)
> > > > > > +???ADD_EXECUTABLE(${testProg} ${test})
> > > > > > +-??TARGET_LINK_LIBRARIES(${testProg} yajl m)
> > > > > > ++??TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
> > > > > I am not sure to understand why this would fix the parallel
> > > > > build
> > > > > issue. The parallel build problem comes from the fact that
> > > > > the
> > > > > library
> > > > > and tests sit in different directories, and that they are
> > > > > built
> > > > > in
> > > > > parallel with each other. Both the yajl and yajl_s libraries
> > > > > are
> > > > > built
> > > > > in the same directory, separate from the tests directory. So
> > > > > I
> > > > > don't
> > > > > quite see how changing from using yajl to yajl_s is going to
> > > > > make
> > > > > a
> > > > > difference here. But maybe I'm missing something.
> > > > The issue is that we patch yajl (#0002) to build only the
> > > > static
> > > > library in a static build. So there is no shared library target
> > > > for
> > > > CMake to use as a dependency.
> > > hmm... with that explaination, I nack this.
> > > IMHO, the correct way to fix this would rather be:
> > > 
> > > IF(BUILD_SHARED_LIBS)
> > > ? TARGET_LINK_LIBRARIES(${testProg} yajl m)
> > > ELSE()
> > > ? TARGET_LINK_LIBRARIES(${testProg} yajl_s m)
> > > ENDIF()
> > > 
> > Yes, this would probably be best. However, the static library is
> > build
> > in shared and static builds.
> I'm aware of that. Unfortunately, CMake is not equivalent to
> autotools
> on that point.
> CMake only provide one boolean flag to control the shared libs.
> build,
> instead of a tristate option allowing building: shared libs only, or
> static libs only, or shared and static libs.
> That's why I made the choice of always building the static libs, and
> keep BUILD_SHARED_LIBS driving the shared libs. build.
> 
> Though the static libs are always built, if the shared libs are built
> as well, I'd tend to use the *.so at least to reduce the footprint...

Yes, this is a good argument. This way, we should fix the other (6 or
7) targets in the yajl build system, too.

However, this was the easiest one to fix the build error.

Maybe someone else has an opinion about this issue?

J?rg