RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

Drake build system compatibility with DRC #1234

Closed patmarion closed 8 years ago

patmarion commented 9 years ago

I thought I could add drake/drake to the drc tobuild.txt, but this didn't work because the drake build system cannot find the cmake subdir. I can't add drake to tobuild.txt because that will build dependencies that are already provided by drc. How do you guys think we should make this work?

patmarion commented 9 years ago

The control directory in drc also uses the cmake directory. Currently it is working via relative path symlink.

RussTedrake commented 9 years ago

I would think you’d just add drake (not drake/drake) to the drc build, and turn off all of the externals except cmake.

On Aug 6, 2015, at 6:26 PM, Pat Marion notifications@github.com wrote:

I thought I could add drake/drake to the drc tobuild.txt, but this didn't work because the drake build system cannot find the cmake subdir. I can't add drake to tobuild.txt because that will build dependencies that are already provided by drc. How do you guys think we should make this work?

— Reply to this email directly or view it on GitHub https://github.com/RobotLocomotion/drake/issues/1234.

patmarion commented 9 years ago

DRC uses a tobuild.txt file to build everything, so currently there isn't an opportunity to customize the drake build options.

patmarion commented 9 years ago

cc'ing @rdeits and @tkoolen. Thoughts on best way to implement?

rdeits commented 9 years ago

I'd probably vote for just checking the cmake code into drake/drake/cmake and using subtrees to keep it up to date later. Then we can just add drake/drake to tobuild and not worry about it.

On Mon, Aug 10, 2015 at 4:38 PM, Pat Marion notifications@github.com wrote:

cc'ing @rdeits https://github.com/rdeits and @tkoolen https://github.com/tkoolen. Thoughts on best way to implement?

— Reply to this email directly or view it on GitHub https://github.com/RobotLocomotion/drake/issues/1234#issuecomment-129599409 .

Robin L. H. Deits Robot Locomotion Group @CSAIL Massachusetts Institute of Technology

tkoolen commented 9 years ago

I think switching from pods to ExternalProject is kind of viral in the sense that now all of a sudden you have the configure step to deal with, also in all projects that depend on the project you switched over. I think the right way to deal with this is to replace the Makefile in drc/software with a CMakeLists.txt containing ExternalProject_Add calls...

patmarion commented 9 years ago

In that case, we are declaring that drake is no longer pods compliant. Are we ok with that? It seems possible to maintain the drake/drake subdirectory as a compliant pod, if only the cmake/ directory issue could be resolved. One hacky thing I could do: in DRC externals I could clone the cmake/ directory and as a post-install step I could symlink it where drake wants it to be.

RussTedrake commented 9 years ago

Just catching up with these.

I don’t see the problem. If you cd into drake and run make, it does the right thing (clones cmake, builds into the parent build dir, etc). It should be fully pods compliant.

You can pass the CMAKE_FLAGS environment variable into the make command to disable building superfluous externals in drake. That’s what the build servers do.

On Aug 10, 2015, at 5:55 PM, Pat Marion notifications@github.com wrote:

In that case, we are declaring that drake is no longer pods compliant. Are we ok with that? It seems possible to maintain the drake/drake subdirectory as a compliant pod, if only the cmake/ directory issue could be resolved. One hacky thing I could do: in DRC externals I could clone the cmake/ directory and as a post-install step I could symlink it where drake wants it to be.

— Reply to this email directly or view it on GitHub https://github.com/RobotLocomotion/drake/issues/1234#issuecomment-129625009.

patmarion commented 9 years ago

Sorry to leave this hanging. This is still a blocking issue. I think your suggestion is to edit the toplevel Makefile in DRC to set CMAKE_FLAGS before parsing tobuild.txt. This would pass the cmake flags to all projects, not just drake, and would require disabling all drake externals by name, or would drake introduce a new variable to disable all externals (sort of an opposite of WITH_ALL_SUPPORTED_EXTERNALS)?

What if, instead of cloning the cmake scripts repo into the drake/ subdirectory, as in:

set(cmake_GIT_CLONE_DIR "${PROJECT_SOURCE_DIR}/drake/cmake")

what if drake looked for the cmake script folder in build/share/cmake-pod-scripts or something like that? Then the DRC project or any other project that wants to embed drake can just run make in the drake/drake subdir and skip over all the drake superbuild logic?

RussTedrake commented 9 years ago

The problem with that suggestion is that drake is linked with the particular sha of cmake via the superbuild CMakeLists.txt . If you are willing to break that connection, then you could just submodule the cmake pod yourself into drake/drake/cmake and skip over the superbuild logic that way, too.

On Aug 18, 2015, at 7:08 PM, Pat Marion notifications@github.com wrote:

Sorry to leave this hanging. This is still a blocking issue. I think your suggestion is to edit the toplevel Makefile in DRC to set CMAKE_FLAGS before parsing tobuild.txt. This would pass the cmake flags to all projects, not just drake, and would require disabling all drake externals by name, or would drake introduce a new variable to disable all externals (sort of an opposite of WITH_ALL_SUPPORTED_EXTERNALS)?

What if, instead of cloning the cmake scripts repo into the drake/ subdirectory, as in:

set(cmake_GIT_CLONE_DIR "${PROJECT_SOURCE_DIR}/drake/cmake")

what if drake looked for the cmake script folder in build/share/cmake-pod-scripts or something like that? Then the DRC project or any other project that wants to embed drake can just run make in the drake/drake subdir and skip over all the drake superbuild logic?

— Reply to this email directly or view it on GitHub https://github.com/RobotLocomotion/drake/issues/1234#issuecomment-132387284.

patmarion commented 9 years ago

Another option, for DRC we could move drake into software/externals. The projects in DRC's externals are not built with a tobuild.txt, they use cmake's ExternalProject and can be configured individually. If we move drake to software/externals, we could allow its superbuild to provide several of the DRC dependencies: eigen, bullet, snopt, etc. That might be nice in a lot of ways. But, we'd have to resolve the issue of drake using the terrain maps library provided by DRC.

mauricefallon commented 9 years ago

I'm glad you guys are discussing this issue, but keep in mind that there are other modules being build in the drc repo. There are a large number of perception c++/pods. I'm not keen on ditching pods for the drc/software directory

making bespoke modifications to software/Makefile for drake is likely to cause other confusions.

RussTedrake commented 9 years ago

And the fact that drake changes often in ways that will support the humanoids. I don't want to in any way encourage the divergent forks of drake that we had in June.

On Aug 19, 2015, at 11:38 AM, Pat Marion notifications@github.com wrote:

Another option, for DRC we could move drake into software/externals. The projects in DRC's externals are not built with a tobuild.txt, they use cmake's ExternalProject and can be configured individually. If we move drake to software/externals, we could allow its superbuild to provide several of the DRC dependencies: eigen, bullet, snopt, etc. That might be nice in a lot of ways. But, we'd have to resolve the issue of drake using the terrain maps library provided by DRC.

— Reply to this email directly or view it on GitHub.

patmarion commented 9 years ago

So perhaps the most non-disruptive change for now would be to have drc provide the cmake scripts repo and symlink it into software/drake/drake, and then add drake/drake to the tobuild.txt. The cmake scripts repo could be downloaded from software/externals or added as yet another submodule. In order to avoid more submodules, my vote is to download it via software/externals. This will be a sufficient solution for now, and we can work on improving later. This will allow us to move to drake/master. I've verified everything compiles with drake/master, but need to test it on Atlas hardware.

Perhaps in the future drake can offer a pods compliant method for getting the correct version of the cmake scripts. For example, a tobuild.txt file might contain:

drake/cmake # this makefile downloads the cmake scripts drake/drake # this makefile builds drake, assumes all dependencies are provided

Or, as @rdeits suggested, use git subtree to add cmake scripts into drake/drake. Or maybe there can be a non intrusive CMAKE_FLAGS option, but this doesn't seem to be in the spirit of pods, so might suggest moving drake into software/externals.

mauricefallon commented 9 years ago

@RussTedrake : I'm committed to Drake and avoiding forking.

NASA are ok with adding the Val model into Drake. Can I add it to drake? I will also work to resolve issues mentioned here https://github.com/mitdrc/drc/issues/2103

RussTedrake commented 9 years ago

that would be fantastic!

On Aug 19, 2015, at 2:01 PM, Maurice Fallon notifications@github.com wrote:

@RussTedrake : I'm committed to Drake and avoiding forking.

NASA are ok with adding the Val model into Drake. Can I add it to drake? I will also work to resolve issues mentioned here mitdrc/drc#2103

— Reply to this email directly or view it on GitHub.

patmarion commented 9 years ago

So DRC has updated to use drake master and the new drake build system. I'm not completely satisfied with the design of how drake is embedded in DRC. There are still some bad pitfalls for developers. For example, this warning I emailed to the drc mailing list which is referring to build procedures for the DRC repository:

Don't do this:

cd drake make

Do this:

cd drake/drake make

If you cd drake && make, just control-c to kill it. However, if you let it go for a while without stopping, it will do things like overwrite the installed libbot with a different version, and install lcm, that's not good! You'll have to make uninstall.

patmarion commented 9 years ago

Notes for @manuelli

To get the version of cmake scripts required by drake:

cddrc cd software/drake grep cmake_GIT_TAG CMakeLists.txt

That will display the sha1 of the RobotLocomotion/cmake.git repo required to build drake. Paste this sha1 into drc/software/externals/cmake/externals.cmake, near the bottom, search for cmake_GIT_TAG. If you could remove this hard coded value from externals.cmake and replace it with the output of a grep command, that would resolve one small issue.

patmarion commented 9 years ago

To recap the current pain points of embedding drake into DRC:

Reasons why toplevel drake directory does not behave like a pod: is a distro, i.e. it installs other pods. The pods it installs conflict with DRC pods. It has to be configured to disable the conflicting pods. Configuration is not part of the pod workflow, and not easily supported by the existing drc build system. Configure is supported by the drc externals build system, but, we cannot easily move drake to externals because drake depends on drc libraries.

For now, drake is embeded in DRC and the DRC externals build system provides drake/drake/cmake. This is annoying for two reasons:

RussTedrake commented 9 years ago

What if the superbuild drake pod looked for the existence of each dependency before adding it as a (default) external?

On Aug 28, 2015, at 12:25 PM, Pat Marion notifications@github.com wrote:

To recap the current pain points of embedding drake into DRC:

The top level drake directory does not behave like a pod, so it is difficult to embed into DRC using the existing workflow. The drake/drake directory behaves like a pod, but does not compile unless drake/drake/cmake is provided by an external mechanism Reasons why toplevel drake directory does not behave like a pod: is a distro, i.e. it installs other pods. The pods it installs conflict with DRC pods. It has to be configured to disable the conflicting pods. Configuration is not part of the pod workflow, and not easily supported by the existing drc build system. Configure is supported by the drc externals build system, but, we cannot easily move drake to externals because drake depends on drc libraries.

For now, drake is embeded in DRC and the DRC externals build system provides drake/drake/cmake. This is annoying for two reasons:

the disconnected management of the drake/cmake version (note, this is also true for all drake dependencies built by drc: eigen, libbot, bullet, snopt, etc.) if you accidentally run make in software/drake you screw up your drc build. This is most painful and an incredibly common mistake. — Reply to this email directly or view it on GitHub.

patmarion commented 9 years ago

+1, I think that would be an improvement. It would allow drake to provide the cmake pod (and all other default drake dependencies that are not already provided by drc externals). It does make me cringe a bit, because I prefer to not set those types of options dynamically in a build system, but I won't complain in this case :)

It does have some implications: outside of drc, it would prevent drake from building lcm on my system because it wil detect the lcm system install on my computer. Is that good or bad, not sure. It would detect eigen too, so you'd have to be careful to detect package versions. When users report build errors, you'd never know whether it was because drake was configured against their system installed bullet or something odd like that. You might get into tricky situations where there are undetectable version incompatibilities.

This would not resolve the general issue that drc externals has to agree with drake for versions for eigen, bullet, etc, but it resolves the cmake pod which is the trickiest one. It might be nice to move toward supporting drake as an external dependency in DRC, and allow drake to provide all its dependencies, but we need to resolve a couple things:

mauricefallon commented 9 years ago

I don't mean to criticise, but the use of RL/cmake like you have it isn't good. the changes this week have cost several of us here in Edinburgh hours this week. It also adds an extra step when we switch branches (re-inserting drake/drake/cmake)

Moving drake to be an external seems like a good idea. what are the pros and cons? would doing that resolve things going forward permanently?

patmarion commented 9 years ago

I apologize for the time lost, I'm not happy with the situation either. It's causing lost time to me and @manuelli too. My comment here is an attempt to suggest a fix for that extra step, so the cmake sha1 is not hardcoded.

I think moving drake to be an external could simplify a lot of things. Maybe there are gotchas that we haven't thought of, but it seems like a good idea.

In drake, systems/controllers/controlUtil.cpp uses terrain maps. Maybe @rdeits can tell us whether this is required for walking or could be disabled/deprecated.

patmarion commented 9 years ago

p.s. the cmake issue will not appear if you base all drake branches off of the supported drake sha1 that is used by drc submodule. Topics based on this drake version will work, and does not require you to fix anything. Recent drake changes to coordinate frame have broken code in drc, so even though we updated to drake master from a week ago, drake master today is not a good base for topic branches.

mauricefallon commented 9 years ago

BTW: getDrakePath() is commonly used to find files relative to software/drake and has broken in a lot of places because of the move.

perhaps the error in using this and not $DRC_BASE

patmarion commented 9 years ago

getDrakePath() returns /path/to/drc/software/drake/drake, is that not the correct path? Maybe you have something else going on in your environment? I'm not aware of any broken features. I've been using his version on the simulator and hardware.

patmarion commented 9 years ago

Oh, I see. In ipab, files are assuming getDrakePath() returns /path/to/software/drake. It does not. It returns the drake root dir, which has become drake/drake. Yes, using DRC_BASE is definitely the right thing to do in order to build relative paths to DRC files.

patmarion commented 9 years ago

Note, in matlab there is also: pods_get_base_path() which returns /path/to/drc/software/build.

rdeits commented 9 years ago

As far as I'm aware, it should be pretty easy to completely remove the terrain maps dependency from drake, since it's no longer used inside the controller. I'll see what I can do.

RussTedrake commented 8 years ago

i believe this is now resolved. @patmarion -- let me know if i'm wrong.

patmarion commented 8 years ago

Ok to leave closed, but I still feel that it is awkward the way that the cmake scripts directory is downloaded into the drake sub-directory.