RobotLocomotion / drake

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

simplify build menu #3772

Closed RussTedrake closed 7 years ago

RussTedrake commented 7 years ago

OK. Here is my proposal. To be discussed.

We should work to simplify the root CMakeLists.txt to have only 4 WITH_* options: DISABLE_MATLAB WITH_SNOPT WITH_GUROBI WITH_MOSEK

These four require licenses, and there is not a simple pattern IMO among the core drake developers of who has which license. So I think they are distinct options, unfortunately.

With one exception (SeDuMi), all other externals are public, and should be ON unless they are explicitly OFF due to DISABLE_MATLAB or an operating system incompatibility. I will open a different issue about SeDuMi.

Even some of the remaining externals might be scheduled for removal as we port more functionality from Matlab to C++. But this will be a big improvement for now.

@soonho-tri -- do we still need WITH_DREAL?

The obvious cost of this change is longer download/build times; which might turn off first time users or folks that only need minimal functionality. A more subtle penalty is that some consumers (like open-humanoids) include drake with options disabled because they've already included some of these externals at their superbuild level; I think this is just adding more inefficiency but not breaking functionality. The obvious win is that we get to simplify our build logic and CI matrix.

@david-german-tri , @patmarion , @jwnimmer-tri , @jamiesnape . PTAL? For your convenience, here is a link to CMakeLists.txt.

soonho-tri commented 7 years ago

@soonho-tri -- do we still need WITH_DREAL?

Should be good now. I merged PR https://github.com/RobotLocomotion/drake/pull/3692 today.

RussTedrake commented 7 years ago

let me be even more ruthless. i think MESHCONVERTERS, AVL, XFOIL, and SIGNALSCOPE should be removed from the build. They are all independent tools (all but signalscope do get called by drake matlab code when they are available). Somebody who wants those features can still get the tools manually, and we will lose coverage on a few aerodynamics-related tests in CI, but in all cases we should be able to restore those tests by checking in the artifacts (e.g. the converted meshes or the aerodynamic coefficients file) into drake. I would want to add a note to checkDependency so that if e.g. AVL/XFOIL is not found, it gives people a pointer to that repo and one line of instructions for adding them to the path.

patmarion commented 7 years ago

I'm fine with having extra tools like signal-scope removed.

I actually like all the WITH_* options, but I don't feel that strongly one way or the other.

oh-distro currently builds drake as an external dependency. It does this by building drake/drake. It does not invoke the drake superbuild. Some of us in lab discussed yesterday whether oh-distro should take advantage of the drake superbuild (after updating to drake/master) to install things like eigen, bullet, snopt, etc. I think oh-distro could do, and it might be easier to maintain. But if the drake build menu is changed to remove WITH_DIRECTOR, for example, then I think oh-distro would be better off keeping its current strategy of building drake/drake only.

jamiesnape commented 7 years ago

I would remove the following options at least:

Current Required Dependencies:

Collision Detection and Dependencies:

LCM:

Logging:

Solvers:

I would merge:

I guess you could merge WITH_DIRECTOR and WITH_SIGNALSCOPE if signalscope is useful, otherwise drop it altogether. AVL and XFOIL are low overhead as far and system dependencies and build time other than having a Fortran compiler.

I am unsure about:

yaml-cpp is also low overhead.

That pretty much leaves @RussTedrake's initial list with a SWIG option, an aerodynamics option, and a director option.

patmarion commented 7 years ago

Does this mean the list of required dependencies will include almost everything that is currently optional?

Food for thought:

I worked on a project that had lots of options. Then we simplified and got rid of many options. Years later, we had reason to build a subset of our codebase for iOS and Android. We added back the options.

Will someone ever want to compile core drake libraries on a small device? A supercomputer?

jamiesnape commented 7 years ago

I would not make collision detection, lcm, open-source solvers, logging optional.

Alternatively, you could bundle WITH_LCM and WITH_BOT_CORE_LCMTYPES, as well as WITH_CCD, WITH_FCL, and WITH_OCTOMAP.

patmarion commented 7 years ago

Perhaps this could be a separate topic, but I'll toss in one more consideration...

Independent of WITH_FOO options, some projects make available USE_SYSTEM_FOO. For example, I want to build with bullet features, but I want to provide my own compatible version of bullet. It enables your library to integrate into a larger ecosystem.

I'm not suggesting that as a near-term priority, but perhaps down the road it will be important to certain users.

jamiesnape commented 7 years ago

@patmarion see #3186.

mwoehlke-kitware commented 7 years ago

I think this is just adding more inefficiency but not breaking functionality.

I would be concerned about breakage if two different versions of some package are being built.

Independent of WITH_FOO options, some projects make available USE_SYSTEM_FOO.

As noted, #3186 adds these.

I think we may want to revisit how we build Drake. There are at least three options:

As more food for thought, I currently have director, dreal, meshconverters, and octomap turned off. Maybe dreal works now, the rest I think have build issues on my machine. Losing the individual options increases the risk that some users can't build drake at all.

RussTedrake commented 7 years ago

Thanks for all the comments. The fact that you are having problems with some externals is the reason to avoid them being optional. We need to resolve any and all problems like that, and are skirting the issue by letting people simply disable them.

I like the merging of options choice. Will keep collecting inputs, but we should aim to make some decisions soon!

sammy-tri commented 7 years ago

FWIW, WITH_IPOPT is an unusual case on Mac... It defaults to "n" because it causes problems linking to mex files, but it's a perfectly reasonably option to have enabled in the DISABLE_MATLAB (or MATLAB not found) case. I'm not sure this is interesting enough to bother dealing with... (also this is based on old memories and could be wrong)

jamiesnape commented 7 years ago

@sammy-tri I think you are correct.

RussTedrake commented 7 years ago

i've already PR'd a conditional solution for WITH_IPOPT to handle that case.

RussTedrake commented 7 years ago

this is being resolved implicitly by the transition to bazel.