RobotLocomotion / drake

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

replace PODS build system with cmake external projects #1169

Closed RussTedrake closed 9 years ago

patmarion commented 9 years ago

I think there are conflicting goals. When you bump the listed sha1 of drake, you want ExternalProject to automatically update drake. But if a developer changes drake, then you want ExternalProject to not automatically update drake. Git submodule solves this by never automatically updating submodules. But then there are problems when people forget to run git submodule update, they run make when the subprojects are out of date.

If you want to mirror the git submodule functionality, then you can set the UPDATE_COMMAND to be empty, and have an explicit make target like make update-drake or a shell script ./update_drake.sh to do the update, and ExternalProject will never override your checked out project.

Or, there is another design that I prefer to use (thought it's not perfect either!) which is: avoid the two repository split of drake-distro and drake. Instead, just have one repository. A "superbuild" uses ExternalProject to build the dependencies, and then it does the drake build (also as ExternalProject) but there is no need to have a download and update command, since you're already in the drake repository.

RussTedrake commented 9 years ago

Sorry - i don't quite understand the last suggestion. Aren't we already doing the superbuild model?

patmarion commented 9 years ago

Another way to explain the alternate design I just described: just move all the files that are currently in drake-distro into a subdirectory of drake. rm drake-distro. Now there's just one repository. Build everything exactly as you are currently doing, but set UPDATE_COMMAND to empty string for drake.

RussTedrake commented 9 years ago

got it. i've thought about that, but i think that works well for drake, but not for using drake in other places.

RussTedrake commented 9 years ago

just played with Pat's idea of having update disabled by default, and adding a new target for update-all. that's probably my favorite so far. just pushed it if you want to try.

psiorx commented 9 years ago

I like the make update approach as well.

rdeits commented 9 years ago

I'm not sure I like the update target approach yet. The problem is that the user has no way of knowing whether or not they need to run the update target (unless they're looking at the changes to the top-level CMakeLists). It also doesn't provide a way to update pods individually (unless we write one), and it forces us to teach our users a workflow they've never used before and may never use again. Submodules are awkward, but at least they're a useful skill to learn. I think we'd just switch from spending time explaining what git submodule update does to explaining what make update does.

RussTedrake commented 9 years ago

i’m still on the fence, but do have update-POD as targets and make status to replace the missing git status on the root.

The BIG thing that we get here that we don’t get any other way is allowing the user to easily pick and choose which prerequisites they want/need for their system. Any combination is trivially possible. Branches will never give us that.

On Jul 31, 2015, at 5:00 PM, Robin Deits notifications@github.com wrote:

I'm not sure I like the update target approach yet. The problem is that the user has no way of knowing whether or not they need to run the update target (unless they're looking at the changes to the top-level CMakeLists). It also doesn't provide a way to update pods individually (unless we write one), and it forces us to teach our users a workflow they've never used before and may never use again. Submodules are awkward, but at least they're a useful skill to learn. I think we'd just switch from spending time explaining what git submodule update does to explaining what make update does.

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

rdeits commented 9 years ago

Ok, and I admit I don't know of a better answer. But I do feel like we're just going to end up re-implementing most of the functionality of git submodules.

Here's another idea, similar to what pat's suggesting: don't have two repos, bring all the code from drake into drake-distro/drake. Then someone who wants to use drake-distro can run make in drake-distro as usual (or put drake-distro in their tobuild.txt or add_subdirectory(drake-distro) in their cmakelists). Someone who just wants drake without the dependencies can just run make inside drake-distro/drake (or put drake-distro/drake in tobuild.txt or add_subdirectory(drake-distro/drake) in cmakelists).

rdeits commented 9 years ago

The idea with that is that modifying drake is easy (since the code is right there without a submodule abstraction), and all of the other dependent pods auto-update through the external projects. That doesn't make it any easier to, say, update the mosek pod, but I think that's a lot less important than making it easy to work on drake itself.

rdeits commented 9 years ago

To clarify: I'm proposing that we keep the external project setup with the automatic updating for all of the non-drake pods, but then we include the drake codebase as drake-distro/drake and invoke it as an external project in drake-distro/CMakeLists.txt (using the SOURCE_DIR property instead of GIT_REPO).

RussTedrake commented 9 years ago

if we are going to try that, best to do it now. one thing i like is that switching people from drake-distro to drake is an obvious change with consequences anticipated. i worry a bit that we change drake-distro behind the scenes and people get confused.

what do others think?

On Jul 31, 2015, at 5:27 PM, Robin Deits notifications@github.com wrote:

The idea with that is that modifying drake is easy (since the code is right there without a submodule abstraction), and all of the other dependent pods auto-update through the external projects. That doesn't make it any easier to, say, update the mosek pod, but I think that's a lot less important than making it easy to work on drake itself.

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

RussTedrake commented 9 years ago

nitpick: i don't really like having a directory structure starting with drake/drake . but i guess it's inevitable

RussTedrake commented 9 years ago

if we really embrace the way cmake wants to work, then we would also remove the forceconfigure step for the external projects, and really treat them much more like our drc/software/externals.

rdeits commented 9 years ago

Yup. I'm starting to like the idea of treating drake differently from the externals.

patmarion commented 9 years ago

You could easily modify your current CMakeLists.txt with ExternalProject to only build the dependencies, and not drake. This would be just like the DRC workflow where you run make in two different locations:

git clone drake

cd drake/externals make # uses ExternalProject to clone and build and install

cd .. make # the regular drake pod Makefile runs here

If you wanted, you could make a top-level alias: make everything

Everything is installed to drake/build.

patmarion commented 9 years ago

Sorry, one flaw with this approach (running make separately for externals) is that you couldn't support externals that depend on drake. Right now there are none, but director can optionally build after drake and use drake as a dependency. It would work ok with my original suggestion, where drake is built with ExternalProject. My suggestion and Robins are the same, difference is whether you want to nest the drake source code in an extra subdirectory relative to the root, but it would functionally be the same I think.

RussTedrake commented 9 years ago

Ok. Newest version is in. Here's my thinking:

Goals:

My current solution

keep drake-distro with all git externalprojects.

I like this better than the drake/drake option, because that would break our current master —> tag release approach, and the master/next branches option seems like more work.

rdeits commented 9 years ago

I'd still like to push for nesting the drake code in the /drake folder and only having one repo. The layout would be:

--- drake
    |--- CMakeLists.txt
    |--- drake
         |--- CMakeLists.txt
         |--- (all the actual drake code)
    |--- externals
         |--- CMakeLists.txt

Externals would be implemented exactly as you suggested (with the cmake menu) and automatically kept up to date. But whether that folder is built is controlled by the top-level cmakelists (through a cmake variable). Anyone who wants to add drake to their own project just has to decide whether to let drake build its own externals or not.

The result of this would be that, with only one repo, every version of drake is always intimately tied to the exact set of externals that it needs. I see a few really big advantages to doing it this way:

I should explain that last point. Right now, drake-distro master points to a stable release of the drake submodule. But what happens when one of the externals needs to be updated to support a new drake feature? If we want the build servers to test with it, then we have to update the revisions that drake-distro points to. But that breaks our existing stable version for our users. Or we don't update the externals in drake-distro, but then the build servers can't properly test any new features.

Basically, the problem we have right now (I think) is that there's no way to atomically make a change to drake and its externals. Having only one repo would make that possible (and in fact really easy). It would also get rid of all submodule confusion and mean that you never again had to make a commit with the message "update drake" :wink:

RussTedrake commented 9 years ago

I'm almost convinced (even though I spent the last 48 hours making all of the build servers work with the other version). Problems I foresee:

rdeits commented 9 years ago

Yeah, I'm not sure what the right answer is with cmake. A couple of options:

As for the master/next scheme, I think this would actually make things easier. We don't have to have a master and next branch, we can just have tagged revisions of drake (or a "stable" branch which we periodically update to point at newer versions). I think that's how a lot of projects on github normally work, in which there are "release" tags or a "stable" branch, and then master is considered bleeding-edge.