update from @jhoare and @billhoffman to fix windows builds #1843
Conversation
|
@jhoare - PTAL at jenkins 64 bit? |
|
looking into it now. |
|
I don't know why its happening, but it looks like Appveyor (which is passing) is using a path to linker: |
|
Which Jenkins build? Both? |
|
I was referring to the windows 64 bit builds for this pull request. I'm not sure if it was always this way or if something has changed. |
|
John and I tracked down the lcm on win64 issue. https://github.com/RobotLocomotion/drake/blob/master/appveyor.yml The problem is here: We were not setting this environment variable in the Jenkins build script. John made the lcm a non-cmake pod that uses the Makefile. The Makefile does not automatically get the CMake generator flag so it defaults to building lcm 32 bit which has errors. For now, I will add this to the build script we are using to get things working. I would think it odd to require people to set this environment variable to get a correct 64 bit build. Is there another way to pass a CMake generator to a Makefile pod? |
|
i always thought it was odd that cmake did not choose the 64 bit generator by default. isn't that the real source? |
|
No, then it would fail on the 64 bit builds. We can not rely on a CMake default. There are two options on windows 64/32 no matter which one is default, if you try and use the one that is not default it needs to be passed into all the projects. ExternalProject does this by default with CMake projects. But, if we are calling make that then calls cmake the -G needs to be passed along. |
|
I think the right solution here for lcm is to do what @RussTedrake suggested in #1368 and turn the lcm-pod into a cmake pod (with the source at the root of the repository, rather than a subdirectory). I tried to do the "easy" approach of just treating like a Makefile pod, but that seems to be brittle, as we are seeing here. |
|
I agree, but my concern is for other Makefile pods that might not be passing the generator around. In this case, it should be a CMake pod. |
|
what is the resolution? |
|
Do you think it makes sense to add a CMAKE_GENERATOR to the Makefile pods? ifneq "$(CMAKE_GENERATOR)" "" Other option would be to do something like this: For now I have added the env var to the build script we use on Jenkins and builds are looking good. I think for LCM @jhoare will change this to a cmake pod. But that does not address the issue of making sure that make pods calling cmake use the correct generator. |
|
The way I set things up initially is that we always passed the CMAKE_FLAGS through to the makefile subprojects. Is that not working anymore? |
|
You used to use an environment variable for the CMAKE_FLAGS. |
|
yes. not anymore? |
|
We add them to the initial CMake cache now. |
|
on the build server only? the instructions for users still say to set the |
|
On the build servers. Developers would typically use the GUI or pass the flags as command line arguments to CMake. |
|
OK, I missed this: http://drake002.csail.mit.edu/drake/sphinx/windows.html Create a new system environment variable CMAKE_FLAGS and set it to, e.g. -G "Visual Studio 14 2015 Win64". That is lets say a very un-cmake way of doing things... :) This still works, but I guess I would like to change it if possible. If you use a generator on the top level it should get propagated down without having to set environment variables. |
|
I’m happy to update it. But that’s the way that we’ve been working, and might be On Thu, Mar 10, 2016 at 4:29 PM, Bill Hoffman [email protected] wrote: — |
|
retest this please |
|
I did set it in the build scripts, i just want to figure it out for the long term. The build scripts seem to be working very well right now. I think all errors are real at this point. |
|
Except that it is not catching the error that should be caught on #1818 on |
|
What error? The test failure is not being caught by CMake: |
|
appveyor fails on the pendulum urdf test on 32-bit. it should fail. there |
|
I see now: |
|
These tend to be very reliable segfaults on win32. |
|
I can not explain it... Could it have something to do with this stuff: 21: FIND: Parameter format not correct |
|
No. that’s standard output (and a known issue). I’m worried that the win32 build |
|
No, that is not happening, you can see it in the full output: https://drake-jenkins.csail.mit.edu/job/experimental/compiler=msvc-32,label=windows/243/consoleFull If you search for cl.exe you see |
|
travis is failing on "unknown command: cmake_parse_arguments" in lcmtypes.cmake. @jhoare -- Is that from your change? we must be using a cmake feature beyond our minimum required version? |
|
that's the error on drake006, too. |
|
Yes it was likely from my change, since its in the bot_core_lcmtypes package. I copied it from https://github.com/RobotLocomotion/cmake/blob/master/lcmtypes.cmake though, which I thought was being used throughout the system, so I don't see why it is a problem. I apologize but I won't be able to look at it until Monday, since I'll be out of the office for another project. |
|
I think it is just missing an include: The newest version of CMake has that as a built in function. So, all we need to do is add include(CMakeParseArguments) |
|
great. thank you @billhoffman. can you PR? On Thu, Mar 10, 2016 at 8:44 PM, Bill Hoffman [email protected] wrote: The newest version of CMake has that as a built in function. So, all we need to — |
|
@jhoare I put in a PR to your version: Can you merge? If not, I guess we could update this patch to pull from my fork of bot_core_lcmtypes. |
|
I will merge this directly into open-humanoids |
|
the drake005 error is known and unrelated (now #1848). |
update from @jhoare and @billhoffman to fix windows builds
also has the hot fix from @psiorx
resolves #1836