RobotLocomotion / drake

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

Superbuild dependencies seem wrong #2251

Closed jwnimmer-tri closed 8 years ago

jwnimmer-tri commented 8 years ago

Breaking out this issue from #2176.

Also FWIW, I would be a lot more confident in my understanding of how to fix the nlopt dependency if the set(drake_dependencies, ... line listed every dependency except for nlopt. As it stands now, there appear to me to be many items missing, which means I must be misunderstanding something about how its supposed to work.

Here is the current code (line-wrapped):

set(drake_dependencies
 cmake
 eigen
 googletest
 lcm
 bot_core_lcmtypes
 libbot
 bullet
 octomap
 snopt
 gurobi
 swig_matlab
 swigmake
 yaml_cpp nlopt)

Why are none of these other externals not listed as Drake dependencies, either directly or transitively?

spotless
director
signalscope
mosek
iris
yalmip
gloptipoly
bertini
sedumi
avl
xfoil
meshconverters

Relatedly, would it be possible to improve the factoring of the superbuild script so that you didn't have to list items related to an external library in 4 different places in order to have it succeed?

tkoolen commented 8 years ago

I think this is the difference between required and optional dependencies.

jwnimmer-tri commented 8 years ago

But plenty of the things currently in set(drake_dependencies, ... are optional (e.g, lcm, snopt). My suspicion is that the dependencies list is the order-to-build, not a must-have-these-things, in which case all of the externals should be listed?

patmarion commented 8 years ago

Looks like build dependencies (headers, libraries) that must be built before drake. Things like spotless and iris are loadable in Matlab at runtime, but not build dependencies. Others, like director and signalscope, are visualization tools, but drake does not need them at build time.

jwnimmer-tri commented 8 years ago

Aha, that sounds plausible. For matlab libraries (bertini, spotless, iris, etc) those could / would be used by tests though, yes? So the list is of the build dependencies, but not the test dependencies?

I can work on a PR to clarify the code / commenting of the CMakeLists.txt to make this a bit clearer to new readers.

jamiesnape commented 8 years ago

I am seeing this on Windows / MATLAB builds:

  WITH_SPOTLESS                       = ON

...

-- Preparing to build spotless with dependencies: 

...

-- Preparing to build drake with dependencies: cmake;eigen;googletest;lcm;bot_core_lcmtypes;bullet;swigmake;yaml_cpp

...

100: Error using checkDependency (line 450)
100: Cannot find required dependency: spotless
jamiesnape commented 8 years ago

Aside from whether it should be considered a build dependency, something is not correct with spotless.

jwnimmer-tri commented 8 years ago

Per discussions today, while the "only build-deps, not test-deps, are listed as deps" hypothesis may be correct and fair, we see no reason to play that game. Why not just build all the externals first, and then build Drake? Why even allow for the possibility that someone makes a build-vs-test mistake?

I think the resolution here is just to add everything as a dep.

david-german-tri commented 8 years ago

Possible caveat: Is there a circular dependency between director and drake? (Even if there is, we should add everything except director as a dependency.)

david-german-tri commented 8 years ago

On further discussion, we think this is fixed: https://github.com/RobotLocomotion/drake/blob/master/CMakeLists.txt#L170