Refactor and "clean up" superbuild logic

[mwoehlke-kitware]

Refactor the top-level CMakeLists.txt (i.e. 'the superbuild') into several files, making use of CMake functions to organize the logic and eliminate some minor duplication. In particular, this converts the mostly-declarative (and somewhat superfluously repetitive) logic for setting up externals into more imperative/procedural logic that is hopefully better organized and easier to read.

One particular change of note is that all special case "externals" have been removed in favor of new options ALWAYS and LOCAL, indicating, respectively, that an external is always enabled, and that an external is local to the repository (does not need download/update steps). This allows drake itself to be simply a LOCAL, ALWAYS subproject rather than having a completely separate block for setting up the subproject.

The overall formatting and documentation (comments) of the superbuild logic has also been improved. Also, the PODS Makefile and PODS-specific options have been removed.

This includes #2749. This is the first part of an overall effort to clean up the CMake logic for drake proper, which will include reducing or eliminating use of "PODS" and pkg-config.

Picking on @bradking for now for feature review, but feel free to punt to someone else. Review-wise, this is sufficiently close to a total rewrite that it likely makes more sense to simply compare the old and new files on the whole rather than try to review the diff/changes. Also, as the intent of this is to make the CMake logic "as clean as possible", please feel free to point out any areas that could benefit from further improvement.

---

This change is

[liangfok]

Definitely goes in the right direction. Unfortunately Jenkins seems to be having issues right now.

---

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[mwoehlke-kitware]

Arf... will dig more tomorrow... I'm seeing at least one bug, also; it's not supposed to print "with dependencies" as part of "Preparing to build..." when there are no dependencies... apparently that's not working (on my machine either, even though I would've sworn I checked...)

[mwoehlke-kitware]

So, turns out dependencies for non-CMake externals were broken because I missed a spot renaming variables. Hopefully things are working now. (Windows CI seems okay as of a few builds back, and the changes to try to get Linux working shouldn't break them.)

[bradking]

Please move the Drake*.cmake files into a cmake/ subdirectory.

Also, the DrakeConfig.cmake file's role is not clear. It seems to do a few unrelated things that are not much different from nearby code in the top-level CMakeLists.txt file. At a minimum we need to rename DrakeConfig.cmake to a name that does not look like a package configuration file.

[bradking]

@mwoehlke-kitware the new names look good. Please squash that in when ready.

[mwoehlke-kitware]

Please move the Drake*.cmake files into a cmake/ subdirectory.

Done.

Also, the DrakeConfig.cmake file's role is not clear. It seems to do a few unrelated things that are not much different from nearby code in the top-level CMakeLists.txt file. At a minimum we need to rename DrakeConfig.cmake to a name that does not look like a package configuration file.

It's intended to be the build environment setup, as opposed to user build options (which would be DrakeOptions.cmake; for now I've left those in the top CMakeLists.txt though). I've used the convention before[1][2], though the file in that case is just config.cmake. Pending better suggestions, I've renamed it to DrakePlatform.cmake.

[RussTedrake]

just tested this on my mac. looks like the source version is not getting passed through properly...

javac: invalid source release: -classpath
Usage: javac <options> <source files>
use -help for a list of possible options
make[5]: *** [lcm-java/jchart2d-code/CMakeFiles/jchart2d.dir/java_compiled_jchart2d] Error 2
make[4]: *** [lcm-java/jchart2d-code/CMakeFiles/jchart2d.dir/all] Error 2
make[3]: *** [all] Error 2
make[2]: *** [lcm-prefix/src/lcm-stamp/lcm-build] Error 2
make[1]: *** [CMakeFiles/lcm.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

note also

drake008% pwd
/Users/russt/drake-distro/build
drake008% more CMakeCache.txt| grep -i java
Java_IDLJ_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/idlj
Java_JARSIGNER_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/jarsigner
Java_JAR_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/jar
Java_JAVAC_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/javac
Java_JAVADOC_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/javadoc
Java_JAVAH_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/javah
Java_JAVA_EXECUTABLE:FILEPATH=/Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/bin/java
CMAKE_JAVA_COMPILE_FLAGS:INTERNAL=-source;1.7;-target;1.7

but

drake008% pwd
/Users/russt/drake-distro/build/externals/lcm
drake008% more CMakeCache.txt| grep -i java
CMAKE_JAVA_COMPILE_FLAGS:STRING=-source

[mwoehlke-kitware]

Arf, sorry; setting the flags needs to be a quoted string. Patch coming shortly.

[mwoehlke-kitware]

jenkins: retest this please

(CI seems clean and looks like I figured out the textbook problem — see last commit — but for some reason CI is showing up as failed in github...)

[mwoehlke-kitware]

Waiting on platform review... @sherm1, can you do that or reassign to someone that can? Thanks!

[mwoehlke-kitware]

Note: expected to fix #2772.

[jwnimmer-tri]

I'll do platform the review of this one.

We will also need @RussTedrake to do a build-and-test on his mac before merging.

[jwnimmer-tri]

Makefile, line 1 [r7] (raw file):

@RussTedrake I believe the deletion of drake-distro/Makefile from the source tree removes Drake's support for being compiled as a POD -- as an external for some other project via the POD conventions. Is that acceptable?

---

Comments from Reviewable

[mwoehlke-kitware]

I believe the deletion of drake-distro/Makefile from the source tree removes Drake's support for being compiled as a POD

Yes, but also there are changes to the CMakeLists.txt with that effect.

It's my understanding that we are moving away from PODS, but if that's not the case, or if we're not ready for this step yet, please let me know and I can back those changes out.

[jwnimmer-tri]

Done with first read-through. I will want to test a bunch of this locally, which I will try to do today.

---

Reviewed 1 of 8 files at r2, 4 of 7 files at r5, 1 of 4 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

CMakeLists.txt, line 102 [r7] (raw file):

# BEGIN external projects

# External projects in order of dependencies; 'trivial' ones first

I don't understand what "in order of dependencies" means here. The immediately subsequent list is alphabetical, and the wider "external projects" section is not strictly in dependency order.

---

CMakeLists.txt, line 202 [r7] (raw file):

# director
drake_add_external(director PUBLIC
  BUILD_COMMAND_DIR distro/pods/drake-distro # FIXME

Unclear what "FIXME" is referring to -- either the problem, or the suggested resolution, or ...

---

Makefile, line 1 [r7] (raw file):

Needs CHANGELOG.md entry documenting the removal of this feature.

---

cmake/DrakeExamples.cmake, line 2 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_add_example EXAMPLE DEFAULT_ENABLED DOCSTRING)

Please add an API comment explaining what this function does.

For this (and the similar defects below) -- brevity is fine, a single sentence or so. I am content to let people read the source code to see the details, but I think capturing the big picture intent atop the function is important.

---

cmake/DrakeExamples.cmake, line 5 [r7] (raw file):

  string(TOUPPER ${EXAMPLE} EXAMPLE_UPPER)

  # Add option for example

BTW -- a META-comment for this PR (though marked as BTW to be non-blocking)... Many comments (such as this one) are sentences, but lack a final period. This PR is a merge conflict landmine, so I'd like to get it in ASAP, so I won't belabor the point. But in general we should endeavor to punctuate appropriately.

---

cmake/DrakeExternals.cmake, line 15 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_add_submodule PATH DOWNLOAD_COMMAND_VAR UPDATE_COMMAND_VAR)

Please add an API comment explaining what this function does.

---

cmake/DrakeExternals.cmake, line 45 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_fixup_commands PREFIX)

Please add an API comment explaining what this function does.

---

cmake/DrakeExternals.cmake, line 58 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_compute_dependencies OUTVAR)

Please add an API comment explaining what this function does.

---

cmake/DrakeExternals.cmake, line 83 [r7] (raw file):

#------------------------------------------------------------------------------
macro(drake_add_cmake_external PROJECT)

Please add an API comment explaining what this function does.

---

cmake/DrakeExternals.cmake, line 136 [r7] (raw file):

#------------------------------------------------------------------------------
macro(drake_add_foreign_external PROJECT)

Please add an API comment explaining what this function does.

---

cmake/DrakeExternals.cmake, line 183 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_add_external PROJECT)

Please add an API comment explaining what this function does.

---

cmake/DrakePlatform.cmake, line 2 [r7] (raw file):

#------------------------------------------------------------------------------
function(drake_get_matlab_jvm_version)

Please add an API comment explaining what this function does.

---

cmake/DrakePlatform.cmake, line 24 [r7] (raw file):

    if(NOT _result AND _output MATCHES "Java ([0-9]+\\.[0-9]+)\\.[0-9_.]+")
      set(MATLAB_JVM_VERSION ${CMAKE_MATCH_1} CACHE INTERNAL "")
      set(_jdk_version "${Java_VERSION_MAJOR}.${Java_VERSION_MINOR}")

I think this relies on find_package(Java ...) having been run. Should that call be included within this macro, or is it conventional in CMake to rely on things like that already having been set by our caller?

---

Comments from Reviewable

[jwnimmer-tri]

Makefile, line 1 [r7] (raw file):

Yes, but also there are changes to the CMakeLists.txt with that effect.
It's my understanding that we are moving away from PODS, but if that's not the case, or if we're not ready for this step yet, please let me know and I can back those changes out.

Let's move this discussion to #2484.

---

Comments from Reviewable

[RussTedrake]

is it still possible to "make clean" for a single external? (even by, e.g., zapping the right subdirectories from build?). for example, I've been trying to test your changes to lcm, and don't know how to do that with confidence without rebuilding all of the externals from scratch?

if missing, can we add back those targets?
if possible, then can we add instructions to the developer docs? (apologies if they are there and I missed it)

---

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

Comments from Reviewable

[RussTedrake]

i think you do not have enough testing capabilities to bring in this change right now. please extract the lcm java fix into a separate PR so that we can resolve that first.

---

Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

Comments from Reviewable

[mwoehlke-kitware]

is it still possible to "make clean" for a single external? (even by, e.g., zapping the right subdirectories from build?).

You can enter the external's build directory and run make clean/ninja -t clean. For CMake externals (i.e. ones that do proper out-of-source builds), you can also nuke <build>/externals/<name>/ and <build>/<name>-prefix/ (the latter is needed to nuke the stamp files that tell the superbuild if it has already executed a build step... in particular, the configure step...). Re-run CMake manually after doing the latter so it can recreate the stamp files tree.

if missing, can we add back those targets?

One of the reasons I removed them is because they were broken. They nuked the build trees but not the stamp files needed to trigger a rebuild, so they'd leave the overall build in a broken state requiring manual intervention to repair.

I've been trying to test your changes to lcm, and don't know how to do that with confidence without rebuilding all of the externals from scratch?

Because CMake projects' clean target do not remove installed files, the only reliable way to do such a test (whether for LCM or for anything else) is to nuke the entire install tree and reinstall everything. While it's possible to nuke just the install-step stamp files, I usually find it easier to nuke all the stamps and rebuild everything. (Actually, since we always rebuild externals, you wouldn't save much trying to be more surgical anyway.)

So the easiest way to do such a test is to remove <install>/, <build>/*-prefix/ and <build>/externals/<name>/. For extra safety, it's not a bad idea to also nuke <source>/externals/<name>/, followed by git checkout externals/<name> (to restore the empty directory that will hold the git submodule). For non-CMake projects (or obnoxious ones like libbot, prior to #2777), you'll need to do this anyway as they are not out-of-source builds.

[RussTedrake]

rebuilding the entire tree is much slower. in the old way, I would manually zap the pod-build folders in the repo of interest, and any repo that I wanted to rebuild because it was a dependent.

[mwoehlke-kitware]

in the old way, I would manually zap the pod-build folders in the repo of interest

I think there is some confusion here. Note:

• PODS: The build directory for an external is <source>/externals/<name>/pod-build
• Current: The build directory for an external is <build>/externals/<name>

There are two ways to force a clean build of a CMake external:

1. Enter the build directory and run cmake --build . --target clean (or make clean / ninja -t clean as appropriate).
2. Delete the entire build directory and also delete <build>/<name>-prefix/.

This is the same as the procedure you are describing, except that the name of the build directory has changed (prior to this PR); see preceding statements in this comment.

You do still need to nuke the install tree to clean it. However, disregard my prior statement about nuking all the stamps; this is not needed for Drake. Since we run the build steps always, and the install steps depend on the build steps, the install steps also always run.

[jwnimmer-tri]

rebuilding the entire tree is much slower

With ccache and a good computer, selective nuking and full-nuking are approximately the same build time (both trivial), and the latter is much less likely to produce an unsound result.