GMock: fix dependency declarations #135

muggenhor · 2017-01-07T19:12:45Z

This ensures CMake knows how to build the library files, so that if they're used as dependencies it knows how to order the build chain, even on fully parallelized builds. Additionally this switches Travis over to use Ninja for a build system instead of Make, which makes use of those parallel builds.

This partially replaces what PR #109 does and unlike that PR it's mergeable with master. It will also have a merge conflict with #130, but I've been careful to do as few changes as possible to keep that conflict as small as possible. At this time I'm most interested in getting this reviewed. If it's more convenient I'm happy to rebase this after merging #130.

Logically this change does one thing only (inside FindGMock.cmake): declare the produced libraries as being produced by the ExternalProject_Add declarations. (There's also the fix of a small typo in the libgmock import declaration which rendered that target completely useless).

konserw · 2017-01-09T21:09:21Z

This is exactly what I tried to achieve with #109 but I'm not as good with CMake as you :) If this is merged I'll close my PR.

muggenhor · 2017-01-09T21:36:32Z

Yeah CMake is not the easiest to get started with. There's a lot of bad tutorials out there and CMake's own documentation is more of a reference manual than a guide.

Anyway thanks for your vote of confidence!

paoloambrosio · 2017-01-10T10:49:37Z

I believe this assumes that Ninja requires a superset of what is required for Make (given that we are not going to compile with Make anymore). Am I correct?

Also it's unfortunate that Ninja is not enabled on AppVeyor given how slow NMake is (I believe this has an example of how to install it).

We'll wait for #130 to be merged and this rebased as you said.

muggenhor · 2017-01-10T13:13:46Z

If you would use Make with the -j$N option with $N > 1 it would require the same level of correctness from the dependencies as Ninja does. It's just that Ninja by default requires a fully correct dependency graph (it's more strict and enforces more of its requirements before even starting a build) and by default already runs in multiple jobs (I think the default is $amount_of_processors + 1).

I've got experience with using Ninja on Linux and OSX (OSX only through CI systems at work and Travis), I have none for Windows. So I think it's smart to perform experiments for using it on Windows separately from this PR.

paoloambrosio · 2017-01-23T09:58:42Z

#130 merged now :)

muggenhor · 2017-01-23T15:57:27Z

Hmm, apparently BUILD_BYPRODUCTS only gets <BINARY_DIR> expanded on CMake 3.3+. Will rewrite that to the manual approach.

muggenhor · 2017-01-23T16:11:32Z

~~If this builds it can be merged (provided no one has review comments).~~

This can be merged.

paoloambrosio · 2017-01-23T21:28:56Z

Tried it with "make -j5" and it failed but with "make" it works (see gist https://gist.github.com/paoloambrosio/787377fe5229e286bb742bafcefb4e68). GNU Make 4.1 on Ubuntu 16.04.

Any idea why?

muggenhor · 2017-01-24T16:10:33Z

Could you show the content of /home/blues/Workspace/Cucumber-CPP/cucumber-cpp-135-ninja/build/gmock/src/gmock-stamp/gmock-configure-*.log? That should show why configuring the external project fails.

This is succeeding for me btw.

paoloambrosio · 2017-01-24T16:25:01Z

I will do it later when I have access to that machine. The same thing happens on OSX with GNU Make 3.81.

mkdir build && cd build
cmake ..
make -j5

Can you confirm that make -j5 works for you in a fresh build directory (running parallel make for the first time). Either I'm doing something completely stupid or it must be repeatable!

muggenhor · 2017-01-24T17:08:31Z

My full set of commands, that work with c67dbe4:

rm -rf build
mkdir build
cmake -E chdir build cmake \
    -DCUKE_ENABLE_EXAMPLES=ON \
    -DCUKE_DISABLE_BOOST_TEST=OFF \
    -DGMOCK_VER=$x \
    -G Unix\ Makefiles ..
make -C build -j5

Where x=1.6.0, x=1.7.0 and x=1.8.0.

I am running this on a Debian Stretch laptop with only two cores though, so the diminished concurrency may be why this doesn't trigger for me.

paoloambrosio · 2017-01-24T17:42:37Z

On the Mac I have an Intel i5 5287U that has 2 cores but 4 threads, on Linux an Intel i7 3720QM with 4 cores and 8 threads. Both have an SSD.

Unfortunately Travis is too slow and does not show the issue.

paoloambrosio · 2017-01-24T21:34:10Z

I've updated the gist, but there isn't much information in those logs.

muggenhor · 2017-01-24T21:43:12Z

Hmm, just a warning, that's indeed not much.

What CMake version are you using? I'm running 3.7.1, maybe an older version contains some bug or something (although they generally try to be backwards compatible even with bugs...).

muggenhor · 2017-01-24T21:52:50Z

Okay this is strange. I do suspect a bug in CMake now, because for Ninja it properly generates a build rule for libgtest.a. The makefile generator does no such thing however. This can be circumvented with an explicit add_dependencies though, so I've added that.

Do you still see this problem now?

paoloambrosio · 2017-01-25T07:37:51Z

I've tried with Ninja as well and it doesn't work either. Not sure if you pushed changes (I'm at commit 0578f67).

From the Ninja output it is clear that it tries to compile GTestDriver.cpp before it has downloaded GoogleTest:

[15/85] Building CXX object src/CMakeFiles/cucumber-cpp.dir/drivers/GTestDriver.cpp.o
FAILED: src/CMakeFiles/cucumber-cpp.dir/drivers/GTestDriver.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++    -I/Users/blues/Workspace/Cucumber-CPP/deps/include -I../include -Igmock/src/gtest/include -DBOOST_ALL_DYN_LINK -MD -MT src/CMakeFiles/cucumber-cpp.dir/drivers/GTestDriver.cpp.o -MF src/CMakeFiles/cucumber-cpp.dir/drivers/GTestDriver.cpp.o.d -o src/CMakeFiles/cucumber-cpp.dir/drivers/GTestDriver.cpp.o -c /Users/blues/Workspace/Cucumber-CPP/cucumber-cpp-135-ninja/src/drivers/GTestDriver.cpp
/Users/blues/Workspace/Cucumber-CPP/cucumber-cpp-135-ninja/src/drivers/GTestDriver.cpp:3:10: fatal error: 'gtest/gtest.h' file not found
#include <gtest/gtest.h>
         ^
1 error generated.
[18/85] Performing download step (git clone) for 'gmock'
-- gmock download command succeeded.  See also /Users/blues/Workspace/Cucumber-CPP/cucumber-cpp-135-ninja/build/gmock/src/gmock-stamp/gmock-download-*.log
[20/85] Performing download step (git clone) for 'gtest'
-- gtest download command succeeded.  See also /Users/blues/Workspace/Cucumber-CPP/cucumber-cpp-135-ninja/build/gmock/src/gtest-stamp/gtest-download-*.log
ninja: build stopped: subcommand failed.

Versions: CMake 3.7.2 and Ninja 1.7.2.

muggenhor · 2017-01-25T09:54:54Z

The problem you see is definitely caused by a missing dependency of the cucumber-cpp library targets on GTest::GTest. I've pushed another commit that adds that change, but I'm not sure that's enough, looking at the generated Makefiles I still cannot find the dependency I'm now expecting to see.

muggenhor · 2017-01-25T10:13:54Z

Okay, so apparently that's not improving things. I'm a bit rushed right now, busy with work stuff, so it may be that I'll only be able to look at this again on Monday.

konserw · 2017-01-25T15:13:46Z

I have had similar issue and resolved it by adding:

if(GMOCK_FOUND) 
    add_dependencies(cucumber-cpp gmock) 
    add_dependencies(cucumber-cpp-nomain gmock) 
endif()

at the end of cucumber-cpp/src/CMakeLists.txt.
It is not the most elegant solution though.

muggenhor · 2017-01-30T14:10:58Z

Right, so these kind of add_dependencies (which don't affect usage) do fix things for the Ninja generator:
add_dependencies(GTest::GTest gtest)

Somehow the Makefile generator still doesn't produce the correct dependency though. This seems to be related to this sentence from CMake's documentation on add_dependencies:

Dependencies added to an imported target or an interface library are followed transitively in its place since the target itself does not build.

That's relevant because we're dealing with an imported target. Except that it's not clear what they mean: adding a dependency on an imported target or a dependency of an imported target on something else.

@konserw: your approach produces no differences at all in the produced makefiles for me. Unlike these add_dependencies(GTest::GTest gtest) which do cause several additional dependencies to be emitted by the Makefile generator.

For reference this is the diff between not having the add_dependencies calls in FindGMock.cmake and having them:

diff -Nur build.orig/CMakeFiles/Makefile2 build/CMakeFiles/Makefile2
--- build.orig/CMakeFiles/Makefile2	2017-01-30 14:56:22.021991467 +0100
+++ build/CMakeFiles/Makefile2	2017-01-30 15:00:54.778795893 +0100
@@ -132,6 +132,7 @@
 # Target rules for target CMakeFiles/e2e-steps.dir
 
 # All Build rule for target.
+CMakeFiles/e2e-steps.dir/all: CMakeFiles/gmock.dir/all
 CMakeFiles/e2e-steps.dir/all: src/CMakeFiles/cucumber-cpp.dir/all
 	$(MAKE) -f CMakeFiles/e2e-steps.dir/build.make CMakeFiles/e2e-steps.dir/depend
 	$(MAKE) -f CMakeFiles/e2e-steps.dir/build.make CMakeFiles/e2e-steps.dir/build
@@ -248,7 +249,7 @@
 # Target rules for target src/CMakeFiles/cucumber-cpp-nomain.dir
 
 # All Build rule for target.
-src/CMakeFiles/cucumber-cpp-nomain.dir/all:
+src/CMakeFiles/cucumber-cpp-nomain.dir/all: CMakeFiles/gmock.dir/all
 	$(MAKE) -f src/CMakeFiles/cucumber-cpp-nomain.dir/build.make src/CMakeFiles/cucumber-cpp-nomain.dir/depend
 	$(MAKE) -f src/CMakeFiles/cucumber-cpp-nomain.dir/build.make src/CMakeFiles/cucumber-cpp-nomain.dir/build
 	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --progress-dir=/home/giel/git/cucumber-cpp/build/CMakeFiles --progress-num=53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69 "Built target cucumber-cpp-nomain"

Full diff here, but it's just more of the same: http://pastebin.com/YxtqCSz1

As I see it exactly those dependencies (of cucumber-cpp-nomain.dir/all on gmock.dir/all) should be enough to get this working correctly, even in parallel builds. Although looking at the output produced by Ninja, those are even stronger, where it lists a dependency on the gmock target for every object file to be built for cucumber-cpp-nomain:

build cmake_order_depends_target_cucumber-cpp-nomain: phony || gmock
build src/CMakeFiles/cucumber-cpp-nomain.dir/drivers/GTestDriver.cpp.o: CXX_COMPILER__cucumber-cpp-nomain ../src/drivers/GTestDriver.cpp || cmake_order_depends_target_cucumber-cpp-nomain

So maybe that's necessary there: the object file dependencies, in which case we're possibly dealing with a bug in CMake's Makefile generator.

PS I know about a CMake bug related to object file dependencies, but that's related to generating them too eagerly and that's in the Ninja generator. Although they talk about things being done there "because the Makefile generator does it that way": https://gitlab.kitware.com/cmake/cmake/issues/15555

muggenhor · 2017-01-30T15:01:05Z

I downloaded the generated build.ninja from Travis (after basically catting it in the log output) and see plenty of differences, but the same cmake_order_depends_target_XXX: gmock and XXX/drivers/GTestDriver.cpp.o: cmake_order_depends_target_XXX rules exist for Travis as well. That gmock target includes gmock-download which is only done when git clone succeeded. ~~So I'm currently stumped why this is happening (also I'm wondering why I cannot reproduce this on any of my own systems).~~

Apparently #130 contained a minor typo (with not-so-minor consequences): it used ${GTEST_INCLUDE_DIRS} to set the INTERFACE_INCLUDE_DIRECTORIES target property. But that variable doesn't yet exist at that time, instead it should have used ${GTEST_INCLUDE_DIR} (note the missing plural S).

Of course I only see that when about to give up ;-)

So it was apparently completely unrelated to dependency ordering: just the include path that didn't get expanded properly. As for why it did work on my systems, apparently on all (3) of them I've got libgtest-dev installed, which adds these header files to /usr/include.

@paoloambrosio do you still see problems after my latest push?

Because _DIRS is an output variable that doesn't (yet) exist.

konserw · 2017-01-30T16:21:04Z

I haven't tried this before, but now (at 8a9a61a ) I'm able to compile using both ninja and make -j5 on my linux host - so you've probably nailed it 👍
EDIT: No global gtest installed of course :)

This prevents attempting to include things from /usr/src/gmock/include/gmock/gmock.h, which in Ubuntu Trusty's google-mock package doesn't exist at that location.

This gives CMake the information it needs to figure out the dependencies _properly_ by itself. As a result fully parallel builds now actually work.

muggenhor · 2017-01-30T21:14:43Z

Had to fix a remaining merge problem. (Use of libgtest/libgmock name instead of GTest/GMock.) It should now be ready to go. Provided that @paoloambrosio cannot reproduce the build problem anymore.

paoloambrosio · 2017-03-10T07:06:55Z

It still fails, and now I've created a Docker container to run those builds to verify that it's not my project setup: https://github.com/paoloambrosio/cucumber-cpp-build

Now this PR doesn't solve the problem of parallel builds in general (it still doesn't work for me), but reduces the likelihood of failures (it works for you two). We are stalled as you cannot reproduce the issue and I don't have time to work on it. I suggest we merge this PR anyway and keep working on other issues. What are your thoughts?

muggenhor · 2017-03-13T10:10:00Z

@paoloambrosio I think that may be the best approach. FYI: I can sometimes get a build failure with your docker container (1 in 3 times or so). It seems like every time that does happen the configure step starts before the download step finishes.

muggenhor · 2017-03-13T12:57:56Z

After testing a bit more with your docker container the problem seems to be that the git clone command returns before it finished checking out. I.e. if I change build/gmock/tmp/gmock-gitclone.cmake (after running CMake, before running make/ninja) and add this it sometimes shows an empty directory (except for a .git dir) and sometimes shows a filled one:

execute_process(COMMAND sh -c "ls -lA /usr/src/cucumber-cpp/build/gmock/src/gtest > /dev/tty")

~~If I change the git clone command to sleep 0.1 seconds it happens less frequently but still happens. At 1 second it just doesn't happen.~~

Apparently I didn't account for the separation between the GTest and GMock external projects that are used for 1.6/1.7. In that case the GMock project should not be configured before GTest is downloaded. That's very simple to fix luckily, a single add_dependencies(gmock gtest) solves it as I've done in f3d2458. Then there's the GMOCK_VER=1.8 ./travis.sh line from your dockerfile's readme: that fails because the version should be a triplet: GMOCK_VER=1.8.0 ./travis.sh should work.

This way other properties such as the list of libraries to link to transitively and what header files to use get handled automatically by CMake.

Because after making fully parallel builds possible we'd better start using them to keep testing them.

paoloambrosio · 2017-03-14T07:15:43Z

YES! YES! YES! GREAT JOB!

Fixes parallel builds (including Ninja)

muggenhor · 2017-03-14T08:15:32Z

Thanks for your dockerfile and readme. I'm not sure I would have been able to reproduce and fix this problem without the accompanying readme.

FindGMock: fix typo: use _DIR instead of _DIRS

6f24dba

Because _DIRS is an output variable that doesn't (yet) exist.

muggenhor added 2 commits January 30, 2017 22:12

FindGMock: only add existing and correct include dirs

8746811

This prevents attempting to include things from /usr/src/gmock/include/gmock/gmock.h, which in Ubuntu Trusty's google-mock package doesn't exist at that location.

GMock: specify external project build products

b782d74

This gives CMake the information it needs to figure out the dependencies _properly_ by itself. As a result fully parallel builds now actually work.

GMock: declare dependency on GTest

f3d2458

muggenhor added 2 commits March 13, 2017 14:55

GMock: declare it a dependency where used

ff68d90

This way other properties such as the list of libraries to link to transitively and what header files to use get handled automatically by CMake.

Travis: build with Ninja

1073674

Because after making fully parallel builds possible we'd better start using them to keep testing them.

paoloambrosio merged commit 1073674 into cucumber:master Mar 14, 2017

paoloambrosio added a commit that referenced this pull request Mar 14, 2017

Merge #135 'GMock: fix dependency declarations'

6b960a2

Fixes parallel builds (including Ninja)

paoloambrosio mentioned this pull request Mar 14, 2017

Ninja build #109

Closed

muggenhor deleted the gmock-dependency-fix branch March 14, 2017 08:15

muggenhor added this to the v0.3.1 milestone Jun 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GMock: fix dependency declarations #135

GMock: fix dependency declarations #135

muggenhor commented Jan 7, 2017

konserw commented Jan 9, 2017

muggenhor commented Jan 9, 2017

paoloambrosio commented Jan 10, 2017

muggenhor commented Jan 10, 2017

paoloambrosio commented Jan 23, 2017

muggenhor commented Jan 23, 2017

muggenhor commented Jan 23, 2017 •

edited

Loading

paoloambrosio commented Jan 23, 2017 •

edited

Loading

muggenhor commented Jan 24, 2017 •

edited

Loading

paoloambrosio commented Jan 24, 2017

muggenhor commented Jan 24, 2017

paoloambrosio commented Jan 24, 2017

paoloambrosio commented Jan 24, 2017

muggenhor commented Jan 24, 2017

muggenhor commented Jan 24, 2017

paoloambrosio commented Jan 25, 2017 •

edited

Loading

muggenhor commented Jan 25, 2017

muggenhor commented Jan 25, 2017

konserw commented Jan 25, 2017

muggenhor commented Jan 30, 2017 •

edited

Loading

muggenhor commented Jan 30, 2017 •

edited

Loading

konserw commented Jan 30, 2017 •

edited

Loading

muggenhor commented Jan 30, 2017

paoloambrosio commented Mar 10, 2017

muggenhor commented Mar 13, 2017 •

edited

Loading

muggenhor commented Mar 13, 2017 •

edited

Loading

paoloambrosio commented Mar 14, 2017

muggenhor commented Mar 14, 2017

GMock: fix dependency declarations #135

GMock: fix dependency declarations #135

Conversation

muggenhor commented Jan 7, 2017

konserw commented Jan 9, 2017

muggenhor commented Jan 9, 2017

paoloambrosio commented Jan 10, 2017

muggenhor commented Jan 10, 2017

paoloambrosio commented Jan 23, 2017

muggenhor commented Jan 23, 2017

muggenhor commented Jan 23, 2017 • edited Loading

paoloambrosio commented Jan 23, 2017 • edited Loading

muggenhor commented Jan 24, 2017 • edited Loading

paoloambrosio commented Jan 24, 2017

muggenhor commented Jan 24, 2017

paoloambrosio commented Jan 24, 2017

paoloambrosio commented Jan 24, 2017

muggenhor commented Jan 24, 2017

muggenhor commented Jan 24, 2017

paoloambrosio commented Jan 25, 2017 • edited Loading

muggenhor commented Jan 25, 2017

muggenhor commented Jan 25, 2017

konserw commented Jan 25, 2017

muggenhor commented Jan 30, 2017 • edited Loading

muggenhor commented Jan 30, 2017 • edited Loading

konserw commented Jan 30, 2017 • edited Loading

muggenhor commented Jan 30, 2017

paoloambrosio commented Mar 10, 2017

muggenhor commented Mar 13, 2017 • edited Loading

muggenhor commented Mar 13, 2017 • edited Loading

paoloambrosio commented Mar 14, 2017

muggenhor commented Mar 14, 2017

muggenhor commented Jan 23, 2017 •

edited

Loading

paoloambrosio commented Jan 23, 2017 •

edited

Loading

muggenhor commented Jan 24, 2017 •

edited

Loading

paoloambrosio commented Jan 25, 2017 •

edited

Loading

muggenhor commented Jan 30, 2017 •

edited

Loading

muggenhor commented Jan 30, 2017 •

edited

Loading

konserw commented Jan 30, 2017 •

edited

Loading

muggenhor commented Mar 13, 2017 •

edited

Loading

muggenhor commented Mar 13, 2017 •

edited

Loading