1. OpenSourceRoboticsFoundation
  2. Simulation
  3. gazebo
  4. Pull requests

Pull requests

#2344 Merged at e438156
Repository
gazebo_win_fixes_fork
Branch
win_build_fixes
Repository
gazebo
Branch
default

Config and cmake fixes for Windows build

Author
  1. Aaron Clauson
Reviewers
Description
  • Surgery to config.bat for setting up cmake with Windows. Updated Cmakelist files to include ign-tranpsort and dependencies.

  • Added missing zmq lib path input to cmake.

This pull request also includes changes minor changes in a previous pull request that allowed gazebo to build on Windows.

Comments (47)

  1. Peter Mitrano

    Thanks so much! Windows contributions are very appreciated. I will try this out tonight. But just after a quick scan I can see there are some hard-coded paths (references to F:) that won't work for everyone, so those will need fixing

  2. Aaron Clauson author

    ign-math has always built fine for me. ign-transport requires ZeroMQ and the corresponding C++ binding header. Is your issue at the configure stage or the build stage?

    1. Peter Mitrano

      Linking stage. I downloaded both zeromq and the c++ bindings.

      [ 25%] Linking CXX executable UNIT_Discovery_TEST.exe
      ignition-transport1.lib(Discovery.cc.obj) : error LNK2019: unresolved external symbol __imp_zmq_errno referenced in function "public: __cdecl zmq::error_t::error_t(void)" (??0error_t@zmq@@QEAA@XZ)
      ignition-transport1.lib(Discovery.cc.obj) : error LNK2019: unresolved external symbol __imp_zmq_strerror referenced in function "public: virtual char const * __cdecl zmq::error_t::what(void)const " (?what@error_t@zmq@@UEBAPEBDXZ)
      ignition-transport1.lib(Discovery.cc.obj) : error LNK2019: unresolved external symbol __imp_zmq_poll referenced in function "int __cdecl zmq::poll(struct zmq_pollitem_t const *,unsigned __int64,long)" (?poll@zmq@@YAHPEBUzmq_pollitem_t@@_KJ@Z)
      

      Side note, I'm going to PR the window install instructions to add zeromq and cppzmq.

      1. Peter Mitrano

        dumpbin /SYMBOLS on the libzmq says it's not there, but dumpbin /EXPORTS shows it. My guess is that this has something to do with static/shared libraries

      2. Aaron Clauson author

        After that Protobuf fix below ign-transport builds correctly for me with no other modifications:

        F:\gazebo-build\ign-transport\build_debug_nmake>nmake install
        
        Microsoft (R) Program Maintenance Utility Version 12.00.21005.1
        Copyright (C) Microsoft Corporation.  All rights reserved.
        
        [ 12%] Built target ignition-transport1
        [ 14%] Built target protobuf_compilation
        [ 16%] Built target gtest
        [ 18%] Built target gtest_main
        [ 22%] Built target UNIT_Helpers_TEST
        [ 26%] Built target UNIT_AdvertiseOptions_TEST
        [ 29%] Built target UNIT_Discovery_TEST
        [ 33%] Built target UNIT_NodeOptions_TEST
        [ 37%] Built target UNIT_HandlerStorage_TEST
        [ 40%] Built target UNIT_Node_TEST
        [ 43%] Built target UNIT_NetUtils_TEST
        [ 47%] Built target UNIT_Packet_TEST
        [ 51%] Built target UNIT_Publisher_TEST
        [ 54%] Built target UNIT_TopicStorage_TEST
        [ 58%] Built target UNIT_TopicUtils_TEST
        [ 61%] Built target UNIT_Uuid_TEST
        [ 65%] Built target INTEGRATION_scopedTopic
        [ 68%] Built target INTEGRATION_twoProcessesPublisher_aux
        [ 72%] Built target INTEGRATION_twoProcessesSrvCall
        [ 76%] Built target INTEGRATION_twoProcessesPubSub
        [ 79%] Built target INTEGRATION_scopedTopicSubscriber_aux
        [ 83%] Built target INTEGRATION_twoProcessesSrvCallStress
        [ 86%] Built target INTEGRATION_fastPub_aux
        [ 89%] Built target INTEGRATION_twoProcessesSrvCallSync1
        [ 93%] Built target INTEGRATION_twoProcessesPubSubSubscriber_aux
        [ 97%] Built target INTEGRATION_twoProcessesSrvCallReplier_aux
        [100%] Built target INTEGRATION_twoProcessesSrvCallReplierIncreasing_aux
        Install the project...
        

        Paths in configure.bat (and only lines changed):

        @set PROTOBUF_PATH=F:\gazebo-build\protobuf-2.6.0-win64-vc12
        @set ZEROMQ_PATH=F:\Program Files\ZeroMQ 4.0.4
        @set CPPZMQ_PATH=F:\gazebo-build\cppzmq
        
            1. Peter Mitrano

              Huh.... so the osrf jenkins uses 3.2.4 for ZeroMQ. When I used that I got linker errors. Using the version/installer you linked to works great. This is very annoying >.>

  3. Peter Mitrano

    Ok, so on a first pass over this, I have two main comments:

    1. You're correct we need to update zmq cppzmq stuff, but I prefer to use the FindZeroMQ.cmake used in ign-transport. That way we only need to set ZeroMQ_ROOT_DIR.

    2. There are lots of additions to LINK_DIRECTORIES and INCLUDE_DIRECTORES, and I guess I'm wondering how these are currently working on linux? We want consistent behavior. So if they work on linux for some good reason, maybe we should condition their addition with an ifdef on windows.

    I will surely have more comments as I continue to look through this. Thanks for the contribution though. Are you able to work on this PR as we review it?

    1. Jose Luis Rivero
      and I guess I'm wondering how these are currently working on linux?
      

      Peter, note that in Linux under an standard (from packages) installation the compiler and the linker are able to find the headers and libraries since they are installed in standard locations (i.e /usr/include or /usr/lib/<triplet>) so we can expect patches in Windows (sometimes happen the same in Brew for Mac) that solves compilation/linking and are correct. Linux is really providing a workaround to a bug in our cmake files.

  4. Aaron Clauson author

    I checked my notes for my ign-transport build and I ended up getting cmake to generate Visual Studio 2013 project files and built it that way. Since I wasn't that familiar with cmake at that point I found it quicker to fix missing includes and libraries via Visual Studio rather than CMakeLists.txt.

    I agree that using the cmake findpackage is a better way then having to explicitly set all the variables for the include and link directories. My goal was to be a light as possible on the CMakeLists.txt changes so as not to affect other platforms but also get the Windows build working.

    I'll revisit the ign-transport build now that I can work cmake a bit better and see if it generates a workable cmake config for Windows.

    Apart from that the biggest problem is protobuf. The protobuf-2.6.0-win64-vc12.zip does not include any cmake config files so the gazebo build is relying on pkgconfig to set the variables for it. That doesn't work easily for Windows (posibly pkgconfig can be set up via cygwin or msys but that's another step). I did build protobuf version 3.0.0 from source and that generated usable cmake config files but gazebo could not compile with that version. I'll re-visit this as well and see if I can build protobuf-2.6.0 from source and get a cmake config.

    And yes I'm more than happy to keep re-working and testing the build files.

    1. Peter Mitrano

      I see your point, but we're much more interested in a robust solution that will work for everyone and remove differences between linux and windows.

      As for protobuf, I wouldn't advice mucking with the version. Protobuf is extremely picky about versions for good reasons. I haven't had much issues (lately, but I set all this up a while ago). I'll try to come back here with an explanation what I believe is the correct way for it to be found.

        1. Aaron Clauson author

          With cmake v3.5.2 FindProtobuf.cmake fails when passed gz-ws\protobuf-2.6.0-win64-vc12. I tracked it down to the FindProtobuf.cmake is looking in ${PROTOBUF_SRC_ROOT_FOLDER}/vsprojects/${_PROTOBUF_ARCH_DIR}Release|Debug. Creating an x64 directory in gz-ws\protobuf-2.6.0-win64-vc12\vsprojects\ and copying the Debug and Release trees into it fixed that one.

          I'll update the answer on the gazebo Q&A site with this since it's a better option than what I previously suggested.

            1. Aaron Clauson author

              How would you do that?

              _PROTOBUF_ARCH_DIR is set internally in FindProtobuf.cmake. Sure you could edit that file but that's part of the cmake install tree so is even more intrusive.

              1. Peter Mitrano

                It only gets set if CMAKE_SIZEOF_VOID_P is 8, and for me it seems not to be set at all, so it works. I'm not sure if this "correct" though. seems sketchy

              2. Peter Mitrano

                After rm -rf build on ign-transport I can reproduce your issues with protobuf. Nathan Koenig I believe we should update the protobuf zip file we distribute to place libraries in a x64 subdirectory. That was FindProtobuf.cmake will work as expected on windows

          1. Jose Luis Rivero

            To solve the mess with protobuf I'm going to do the following:

            • Install cmake 3.5.2 on our jenkins
            • Update gazebo tutorial and cmake 3.5.x or higher a requirement
            • Update our protobuf zip to include the necessary layout to make it work with cmake 3.5.x
                1. Aaron Clauson author

                  My rational to use Ogre 1.9.0 was that the link on the Windows instructions page stated that version even though the download is for version 1.8.1. I can vouch that 1.9.0 works fine on Windows and has the added benefit of allowing a few pre-processor checks in the gazebo source that were related to 1.9.0 NOT working on Windows to be removed.

  5. Peter Mitrano

    So easy step to make this closer to merging: replace hardcoded paths to F: or Program Files with environment variables: PROGRAMFILES refers to the non-x86 version. using SYSTEMDRIVE will get you just the F: or C: part if you need it.

    1. Aaron Clauson author

      I've been able to get a ign-transport (which includes zeromq and cppzmq), ign-math and protobuf package resolutions working which enabled a big consolidation of the configure.bat script.

      I've removed my hard-coded F:\ paths and set all dependencies referred to in the configure script to be under the current directory.

      Those changes are committed.

      One problem I have encountered is that the ign-transport cmake script is written to detect the ign-transport and zeromq libraries as dll's. I can't think why it's done that way?

      ignition-transport1-config.cmake line 40: SET(CMAKE_FIND_SUFFIXES ".lib" ".dll") FindZeroMQ.cmake line 95: find_library(ZeroMQ_LIBRARY_RELEASE NAMES ${_zmq_release_names} zmq libzmq HINTS ${ZeroMQ_ROOT_DIR}/bin ${ZeroMQ_ROOT_DIR}/lib )

      I had to remove the ".dll" suffix or the gazebo cmake script ended up trying to link against libzmq-v120-mt-gd-4_0_4.dll.

      Update: Actually the .lib .dll problem for zeromq might be caused by a different cmake lookup script. Removing the ".dll" suffix didn't actually fix the problem. I had to rename the bin directory under the zeromq tree to make it pick the .lib file. I'll have a bit more of a look into this.

      1. Peter Mitrano

        Thanks for the fixes. A smaller diff is always preferable.

        I'll look over it again, and try building gazebo and see if I run into you problem with zeromq/ign-transport as shared vs static libs

  6. Peter Mitrano

    Jose Luis Rivero Aaron Clauson, I'm looking at all the library paths you added for plugins/tests ect.... and I don't think they are necessary. I've made my own set of windows changes which you can see on the windows_zmq branch on my fork here, but I didn't need to add any of that. So convince me why we need them I guess? I haven't tried running it yet, but everything linked.

    1. Jose Luis Rivero

      Peter, the link does not work, could you please use the compare mechanism in bitbucket instead the pull request one?

      If we can demonstrate that some code is not necessary to run test/application then probably is good to remove unless we come up with a good reason.

      1. Aaron Clauson author

        I updated my ign-transport tree with that pull request and confirmed that gazebo still builds properly on Windows with zmq removed from the gazebo cmake files.

  7. Peter Mitrano

    Jose Luis Rivero I've going to approve this PR. However, we need to be sure to update both jenkins and our tutorials. Here's what I can think of that needs to happen:

    I like assigning tasks. Do we agree on these changes? If so, I can PR to update the tutorial since you're the jenkins wizard

    1. Jose Luis Rivero

      I've added the ogre version 1.9 and zeromq 4.0.4 to our packages server. I've already have a branch that use these new dependencies. I also have the tutorial changed, you can leave that to me. I will add you as reviewer when I have the PR ready.

    1. Aaron Clauson author

      I'd stopped using that script and forgotten about it. Updated now. There were a few merge conflicts for the ign-math 2.4 update and a few other things. Hopefully I've put them all back.

      1. Peter Mitrano

        So did you just add everything to your path permanently? I'm not sure if that script is a good idea, or if we should just do that. What will an installer likely do?

        1. Aaron Clauson author

          I currently still run a bath script to set up the environment for the gazebo exe's but it's a very cut down version. The Windows PATH environment variable is limited to 2048 characters by default and the setenv.bat ended up exceeding that for me. Instead of increasing the PATH limit I just copied a number of the dll's to my execution directory.

          I would think a Windows installer would place all the dll's in the program directory with probable exceptions being qt and ogre.