Find g mock #130

meshell · 2016-12-02T23:19:28Z

This pull request fixes the cmake files for building with MinGW under windows.
Further it modifies the FindGMock.cmake file to only download and build googlemock and googletest when not already available. This is IMHO helpful, because usually you already have googletest and/or googlemock installed for your unit tests.

konserw · 2016-12-04T09:38:15Z

Why have you inserted all those spaces in if() and else() ?

meshell · 2016-12-04T15:37:45Z

@konserw I edited the files with CLion and the spaces sneaked in when I reformated the file. I didn't recognize it because whitespace differences are turned of in my diff tool. If this is a problem I will reformat the files.

paoloambrosio

Thanks for the contribution. I have added a few questions in this review.

In open source it is important to respect the same coding conventions, spaces, etc. as the rest of the project.

@konserw given that you contributed the latest changes to this file, can you take a look at this PR?

paoloambrosio · 2016-12-04T18:34:44Z

CMakeLists.txt

-            ${CMAKE_THREAD_LIBS_INIT}
-        )
+            GTest::GTest
+            )


Why this change?

'Threads' is set as interface link library for GTest therefore the dependency is already set and it's not necessary to specify it again.
For the GTest driver the GTest main is not needed, therefore only the GTest library is set as dependency.

paoloambrosio · 2016-12-04T18:40:19Z

CMakeLists.txt

+            GTest::GTest
+            GMock::GMock
+            GMock::Main
+            )


Is GMock::Main required because we require GTest for step definitions (without main) but GMock needs a main method to run our tests?

Yes. A main (either gtest_main or gmock_main) is needed to run the tests and because gmock is used anyway I decided to use the gmock_main only.

@meshell doesn't it make more sense to use gtest_main/GTest::Main in locations where GMock isn't required? I.e. do away with these variables completely and make the use of these specific to the target their linked into?

@muggenhor I totally agree. I would also do it differently and would not use this variables at all. However I didn't want to change too many parts of the existing CMake files in this PR.

I did something like that a while back as an experiment: https://github.com/muggenhor/cucumber-cpp/compare/master...link-depends-only, and it's big enough a change that it definitely belongs in a different PR. But it does depend on doing the same kind of change that you're doing happening first.

paoloambrosio · 2016-12-04T18:43:33Z

src/CMakeLists.txt

+    target_link_libraries(cucumber-cpp
+        ws2_32
+        )
+endif(WIN32 AND NOT MSVC)


Is this required by MinGW or by all Windows compilers that are not MSVC like in this if statement?

Actually I don't know if other compilers need this library as well. i only have MinGW and MSVC available on my windows machine. I know that MinGW requires it, but MSVC doesn't. However I think it would also work when the library is added on MSVC also. I will check that.

I'd be happy with if(MINGW) (if I remember correctly) unless further evidence suggests that it's needed for all WIN32 compilers that are NOT MSVC.

konserw · 2016-12-06T11:41:13Z

@paoloambrosio - Looks good to me, but I'm not CMake expert :)
@meshell - Could you add Mingw build to appveyor CI ?

meshell · 2016-12-08T00:59:16Z

@konserw I will try to add Mingw to appveyor CI. But I will do it in a new pull request. Is this ok?

paoloambrosio · 2016-12-08T08:04:13Z

Adding MinGW to AppVeyor makes sure that the build works on MinGW without testing is manually.
Doing it as a separate PR would be like adding tests for code changes as a separate PR.

I understand it's going to be quite painful and involve a lot of commits. What I suggest is to experiment with it in a separate branch in your repository, and add it to this PR when you are reasonably confident it will work :-)

meshell · 2016-12-08T10:52:24Z

@paoloambrosio: Ok I will do that.
The problem are the boost libraries. I'm afraid, that the boost libraries must be built as part of the project. I already did that on an other project, but I don't know if it is the right approach for cucumber-cpp. I could do it similar to the approach done for the googlemock library, i.e. download and build in a FindBoost.cmake file, when not already available. However this means probably to copy the existing FindBoost.cmake provided with cmake. The other approach would be to have a separate target to build external dependencies. This target must than be build before everything else. What do you think, which approach I should try?

paoloambrosio · 2016-12-11T09:00:51Z

If Boost is not compiled for MinGW on AppVeyor, I'd be OK without a MinGW build. Unfortunately this also means that any change might silently break it.

I'm not a big fan of the build script downloading and compiling dependencies (like the current GoogleTest implementation). It is also not how the standard CMake scripts work.

IMO anything related only to the CI build should be in the CI configuration (travis.yml, appveyor.yml) and scripts used by those (travis.sh). I would even move build steps in a scripts/ directory and have travis.yml and appveyor.yml call them instead of having code and configuration mixed.

konserw · 2016-12-12T13:55:24Z

After quick google search i think for we could install boost for mingw via MSYS2 pacman

meshell · 2017-01-03T21:01:12Z

I had to do some minor changes in some CMake files as well, but finally I managed to add Mingw build to appveyor.

muggenhor · 2017-01-05T09:07:49Z

This PR duplicates #109 btw.

meshell · 2017-01-05T15:53:15Z

@muggenhor I can't see the duplication. Could you please explain.

muggenhor · 2017-01-05T20:46:53Z

@meshell no duplication in code. I just meant that this PR seems to try to accomplish the same thing, so that they're mutually exclusive.

meshell · 2017-01-05T21:10:44Z

@muggenhor I do not agree. This PR fixes the cmake files for building with MinGW under windows and it modifies the FindGMock.cmake file to only download and build googlemock and googletest when not already available (this was actually the main goal of this PR but the MinGW stuff took over a bit). PR #109 however adds ninja support and does not change the FindGMock the same way, or does it?
Why do you think it tries to accomplish the same?

muggenhor

I've got some questions and suggestions, but mostly it looks very well done. Nice work!

muggenhor · 2017-01-05T20:56:10Z

cmake/modules/FindGMock.cmake

+    include(ExternalProject)
+
+    if(MSVC)
+        set(Suffix ".lib")


Why not use ${CMAKE_STATIC_LIBRARY_SUFFIX}?

Good point.

muggenhor · 2017-01-05T20:59:40Z

cmake/modules/FindGMock.cmake

+                LOG_BUILD ON
+                CMAKE_ARGS
+                    -Dgtest_disable_pthreads=${MINGW}
+                    ${GTEST_CMAKE_ARGS}


Why don't you specify BUILD_BYPRODUCTS here? E.g.:

BUILD_BYPRODUCTS <BINARY_DIR>/${GTEST_LIBRARY_PREFIX}gtest${Suffix} <BINARY_DIR>/${GTEST_LIBRARY_PREFIX}gtest_main${Suffix}

(same for other ExternalProject_Add calls)

I didn't want to change the existing code done by @konserw in the PR I just wanted to extend it. I could do it, but IMO it would be better to do all this CMake refactorings in a seperate PR

muggenhor · 2017-01-05T21:00:46Z

cmake/modules/FindGMock.cmake

+            )
+
+
+            add_dependencies(GTest::GTest gtest)


If you specify BUILD_BYPRODUCTS you don't need these custom dependency declarations at all (as long as you use one of those byproducts in the library's IMPORT_LOCATION).

muggenhor · 2017-01-05T21:02:22Z

cmake/modules/FindGMock.cmake

            PREFIX ${GMOCK_ROOT}
            INSTALL_COMMAND ""
-            LOG_DOWNLOAD ON
+            LOG_DOWNLOAD OFF


Why don't just leave it as is? It's not displayed on the terminal anyway, but if something goes wrong you'll now have a log file to look at. This is pretty relevant in e.g. containerized builds where sometimes people mess up with the networking.

This is a mistake. I will fix that. Thanks

muggenhor · 2017-01-05T21:05:01Z

cmake/modules/FindGMock.cmake

+find_package_handle_standard_args(GMock DEFAULT_MSG GMOCK_LIBRARY GMOCK_INCLUDE_DIR GMOCK_MAIN_LIBRARY)
+
+include(CMakeFindDependencyMacro)
+find_dependency(Threads)


Just wondering here: what's the advantage of these two lines over the single: find_package(Threads REQUIRED) (which also creates the Threads::Threads target)? Is it maybe related to not being able to use find_package in a FindXXX module?

Yes as far as I know you can not use find_package in FindXXX modules. This FindGMock.cmake is an adapted version of a recent FindGTest.cmake file plus the code from @konserw and this how its done in the FindGTest.cmake.

muggenhor · 2017-01-05T21:08:59Z

cmake/modules/FindGMock.cmake

+endif()
+
+set_target_properties(GMock::Main PROPERTIES
+    INTERFACE_LINK_LIBRARIES "GMock::GMock"


Do you still want to link transitively to the non-main GMock on 1.7+ where the main version contains the same sources, just one more?

I would think that for 1.7 and beyond not specifying it on the link interface and instead only adding the same include directories as GMock::GMock has to the interface include dirs would be the clean thing to do.

This is how it's done in a recent FindGTest.cmake provided with CMake. That's why I didn't consider removing it.

muggenhor · 2017-01-05T21:10:14Z

src/CMakeLists.txt

@@ -35,3 +35,12 @@ endif()

 add_library(cucumber-cpp-nomain STATIC ${CUKE_SOURCES})
 add_library(cucumber-cpp STATIC ${CUKE_SOURCES} main.cpp)
+
+if(MINGW)


The same should work for MSVC too (it does at work). So I suggest you try with if(WIN32) instead.

Please see the comments by @paoloambrosio for that.

muggenhor · 2017-01-05T21:11:51Z

tests/CMakeLists.txt

@@ -23,7 +23,7 @@ if(GMOCK_FOUND)
    cuke_add_test(integration/HookRegistrationTest)
    cuke_add_test(integration/StepRegistrationTest)
    cuke_add_test(integration/TaggedHookRegistrationTest)
-    if (NOT MSVC)
+    if(NOT WIN32)


Have you tried running this with MinGW? Because I don't think there's anything to this test that should prevent it from running on Windows. (Just asking because I'm only noticing that this test is disabled for Windows just now).

Yes I tried to run it and it failed. I suppose for the same reason as when built with MSVC, but I didn't investigate further. Maybe @paoloambrosio knows why this test fails (under MSVC)

It's been there for ages and I tried several times to understand why this fails on Windows but without success. I remember it crashes but can't remember how to be honest :-) Maybe we should create an issue on GitHub and put the link in a comment to that if statement.

muggenhor · 2017-01-05T21:15:39Z

cmake/modules/FindGMock.cmake

+    )
+mark_as_advanced(GMOCK_INCLUDE_DIR)
+
+find_path(GTEST_INCLUDE_DIR gtest/gtest.h


I'm assuming you're searching the content of the google-mock Debian package here. Does this also work with the newer 1.8 google-mock/googletest packages (Ubuntu Trusty only provides 1.6)? Because I needed to do some hacking in the previous implementation of FindGMock.cmake to be able to use those newer packages.

Usually in a project where cucumber-cpp is used, also a unitttest framework is used, often this might be googletest/googlemock. Therefore my intention was to be able to use this existing (already compiled) version of googlemock and googletest and not compiling it again. I have to confess, that I din't try to use installed versions, i.e. debian packages. I will check that. Thanks for the hint.

muggenhor · 2017-01-05T21:31:53Z

@meshell because the underlying cause why Ninja builds don't work is because of a missing dependency declaration from the libraries to the ExternalProject_Add targets. That's caused by using the ${GXXX_XXX_LIBRARIES} variables (which contain filenames, not target names) instead of the created library targets. You're fixing that part of the cause. If you additionally add the BUILD_BYPRODUCTS declarations then using the filenames would also work (and you'd also not need add_dependencies for the target case).

Edit: actually you're right, it's not the same. #109 is making it safe to use the library filenames, your change is currently just stopping of using them.

konserw · 2017-01-09T21:02:12Z

appveyor.yml

@@ -18,11 +42,15 @@ install:
 - gem install bundle
 - bundle install
 - bundle env
+- if "%build%"=="mingw" set PATH=%MINGW_ROOT%\bin;C:\msys64\usr\bin\;%PATH%
+- if "%build%"=="mingw" set MSYSTEM=%MSYSTEM%


I think this line does nothing (assigns variable to itself)

paoloambrosio · 2017-01-10T10:33:43Z

Once @konserw's latest comment is addressed and commits are squashed (it doesn't need to be one single commit but 10 is a bit too much) and the branch rebased (to run the CI on top of the latest master), I think it can be merged.

Well done on the collaborative effort! 👍

meshell · 2017-01-22T19:24:14Z

@paoloambrosio I removed the lines mentioned by @konserw and rebased it. It is ready to merge.
Thanks.

This pull request fixes the cmake files for building with MinGW under windows. Further it modifies the FindGMock.cmake file to only download and build googlemock and googletest when not already available.

paoloambrosio · 2017-01-23T09:56:19Z

Thanks everyone! @meshell for the great PR, @muggenhor and @konserw for the reviews

paoloambrosio reviewed Dec 4, 2016

View reviewed changes

muggenhor requested changes Jan 5, 2017

View reviewed changes

muggenhor mentioned this pull request Jan 7, 2017

GMock: fix dependency declarations #135

Merged

konserw reviewed Jan 9, 2017

View reviewed changes

meshell force-pushed the FindGMock branch 2 times, most recently from 6270a1f to 96c2659 Compare January 11, 2017 23:53

paoloambrosio merged commit 96c2659 into cucumber:master Jan 23, 2017

meshell deleted the FindGMock branch January 24, 2017 14:10

Find g mock #130

Find g mock #130

Conversation

meshell commented Dec 2, 2016

konserw commented Dec 4, 2016

meshell commented Dec 4, 2016

paoloambrosio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Dec 6, 2016

meshell commented Dec 8, 2016

paoloambrosio commented Dec 8, 2016

meshell commented Dec 8, 2016 • edited Loading

paoloambrosio commented Dec 11, 2016

konserw commented Dec 12, 2016

meshell commented Jan 3, 2017

muggenhor commented Jan 5, 2017

meshell commented Jan 5, 2017

muggenhor commented Jan 5, 2017

meshell commented Jan 5, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

paoloambrosio commented Jan 10, 2017

meshell commented Jan 22, 2017

paoloambrosio commented Jan 23, 2017

meshell commented Dec 8, 2016 •

edited

Loading

muggenhor commented Jan 5, 2017 •

edited

Loading