cmake: stop including files from the install directory

[lukeyeager]

Here is the buggy behavior which this change fixes:

• On the first configure with CMake, a system-wide benchmark installation is not found, so we use the version in third_party/ (see here)

• On installation, the benchmark sub-project installs its headers to CMAKE_INSTALL_PREFIX (see here)

• On a rebuild, CMake searches the system again for a benchmark installation (see cmake build system not properly caching some variables #916 for details on why the first search is not cached)

• CMake includes CMAKE_INSTALL_PREFIX when searching the system (docs)

• Voila, a "system" installation of benchmark is found at CMAKE_INSTALL_PREFIX

• On a rebuild, -isystem $CMAKE_INSTALL_PREFIX/include is added to every build target (see here). e.g:

  cd /caffe2/build/caffe2/binaries && ccache /usr/bin/c++    -I/caffe2/build -isystem /caffe2/third_party/googletest/googletest/include -isystem /caffe2/install/include -isystem /usr/include/opencv -isystem /caffe2/third_party/eigen -isystem /usr/include/python2.7 -isystem /usr/lib/python2.7/dist-packages/numpy/core/include -isystem /caffe2/third_party/pybind11/include -isystem /usr/local/cuda/include -isystem /caffe2/third_party/cub -I/caffe2 -I/caffe2/build_host_protoc/include  -fopenmp -std=c++11 -O2 -fPIC -Wno-narrowing -O3 -DNDEBUG   -o CMakeFiles/split_db.dir/split_db.cc.o -c /caffe2/caffe2/binaries/split_db.cc
  

This causes two issues:

1. Since the headers and libraries at CMAKE_INSTALL_PREFIX have a later timestamp than the built files, an unnecessary rebuild is triggered
2. Out-dated headers from the install directory are used during compilation, which can lead to strange build errors (which can usually be fixed by rm -rf'ing the install directory)

Possible solutions:

• Stop searching the system for an install of benchmark, and always use the version in third_party/
• Cache the initial result of the system-wide search for benchmark, so we don't accidentally pick up the installed version later
• Hack CMake to stop looking for headers and libraries in the installation directory

This PR is an implementation of the first solution. Feel free to close this and fix the issue in another way if you like.

[lukeyeager]

#1103 fixes back-to-back calls to make.

This fixes back-to-back calls to make install.

[lukeyeager]

@ezyang did you have any thoughts about how #916 should be solved?

@bradking why does CMake search the install directory in find_*()? Doesn't this cause similar problems for other users?

[ezyang]

The problem is that there truly is no way to distinguish between a purposely installed copy of a library, and a vendor installed copy. In my opinion, the correct way to solve this problem is to NOT install vendored copies. This solves an additional problem where if caffe2 dynamically links against a vendored library, you basically are SOL if you ever want to use this caffe2 with another version of the dynamic library.

[Yangqing]

Since benchmark is almost always going to be only linked into final binaries, one possibility we can do is to make it static library, and always link it into the final benchmark binaries. I think we can do something like

set(CAFFE2_OLD_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
set(BUILD_SHARED_LIBS OFF)
add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/benchmark)
include_directories(SYSTEM ${PROJECT_SOURCE_DIR}/third_party/benchmark/include)
set(BUILD_SHARED_LIBS ${CAFFE2_OLD_BUILD_SHARED_LIBS})

so that benchmark is included as a static library target.

(see e.g. google/benchmark#369)

[bradking]

why does CMake search the install directory

IIRC that was added to support the use case of building a series of projects in dependency order and installing them all into the same installation prefix.

no way to distinguish between a purposely installed copy of a library, and a vendor installed copy

Some projects mangle their vendored dependencies to avoid interfering with independent copies. One can mangle symbol names via preprocessor macros and/or namespace changes. The library names themselves are easy to change. The headers can be placed in a prefixed directory and included via prefixed names.

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[lukeyeager]

IIRC that was added to support the use case of building a series of projects in dependency order and installing them all into the same installation prefix.

@bradking thanks for the explanation - that makes sense.

In my opinion, the correct way to solve this problem is to NOT install vendored copies.

@ezyang good idea.

I think we can do something like ... so that benchmark is included as a static library target.

@Yangqing nice suggestion. I see that you did something similar for gloo (here).

I put both your suggestions together and updated the PR. This seems to work well (and matches the strategy used for gloo).

NOTE: if you try to test this, you'll still get bogus rebuilds unless you also cherry-pick the change in #1103.

[lukeyeager]

Suggested here: http://public.kitware.com/pipermail/cmake/2012-April/050023.html.

Necessary because Benchmark doesn't have a BENCHMARK_INSTALL option like gloo does (here).

[Yangqing]

Out of curiosity, would this mean that all other install() commands will be no-ops as well? Or do we recover it?

[Yangqing]

I assume that this recovers the original install behavior?

(Really nice trick :) )

[lukeyeager]

Well, it was supposed to. But it looks like there are only 55 files in install/ (instead of 761). So this doesn't work. My guess is _install() only existed in some particular version of CMake. Back to the drawing board ...

[Yangqing]

Thanks!

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[lukeyeager]

I found a surprising way to disable install() for subdirectories (here). Setting EXCLUDE_FROM_ALL seems to do the trick.

$ diff install-before.txt install-after.txt    
359,362d358               
< install/include/benchmark/benchmark_api.h         
< install/include/benchmark/benchmark.h             
< install/include/benchmark/macros.h                
< install/include/benchmark/reporter.h              
710d705                   
< install/lib/libbenchmark.so.0.0.0

PR updated.

[lukeyeager]

I just read another thread (here) which makes me think EXCLUDE_FROM_ALL is causing benchmark not to be built at all. I would expect that to cause the build to fail, but maybe nothing is actually linking to benchmmark?

When I have time to look into this again, I want to see if I can temporarily remove CMAKE_INSTALL_DIR from CMAKE_SYSTEM_PREFIX_PATH when adding the benchmark subdirectory. Then we can install the vendored install (sorry @ezyang) without picking it up on a re-build.

Or, I might put together an upstream benchmark PR to optionally disable the install target (seems unlikely that they would accept such a PR).

Vendoring dependencies is messy.

[lukeyeager]

@Yangqing it doesn't appear that any binary or shared library in the install directory actually links to libbenchmark.so in the OSS code:

$ find install/ -type f | xargs -n1 readelf -d 2>/dev/null | grep 'Shared library' | grep -i bench | wc -l
0

Who uses this? How do I test the fix? As far as OSS is concerned, it looks like we could just remove benchmark entirely.

[pietern]

@lukeyeager This is exactly what was causing my problem in #1140, and what caused the installed Caffe2 headers to take precedence over the ones in the source tree.

[lukeyeager]

Yeah this is a pain. I got around it today by running rm -rf .../install/include/benchmark/ before a rebuild.

Do you know if anyone is actually using benchmark for anything? If not, I think we can just remove it for now. It doesn't address the design flaw of bundling dependencies as explained here (#1112 (comment)), but it would get us unblocked and back to clean builds for now.

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[lukeyeager]

I thought something like this might work, but it seems not.

# Remove CMAKE_INSTALL_PREFIX from CMAKE_SYSTEM_PREFIX_PATH
# Otherwise, third_party libraries will be found there for rebuilds
# Only remove one copy in case the install prefix is /usr/local
list(FIND CMAKE_SYSTEM_PREFIX_PATH "${CMAKE_INSTALL_PREFIX}" found)
if(NOT found EQUAL -1)
    list(REMOVE_AT CMAKE_SYSTEM_PREFIX_PATH ${found})
endif()

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[lukeyeager]

Latest version:

• Link statically to benchmark and try to avoid installing it
  • I'm still not sure how this works exactly, but it's hard to test because we don't appear to actually use this dependency in the project at all
  • @ezyang said this is generally a good idea
• Remove CMAKE_INSTALL_PREFIX from all CMake find_*() functions by hacking CMAKE_SYSTEM_PREFIX_PATH.
  • According to @bradking this was supposed to be a convenience feature, so it shouldn't do any harm to disable it
• Use NO_SYSTEM_ENVIRONMENT_PATH when searching for for benchmark
  • Despite all these changes, I still pick up the install prefix if $PREFIX/bin is in my path (which is probably common)

This is working now. Ready for review.

[ezyang]

I'd quite like it if cmake told me that it was futzing with my CMAKE_SYSTEM_PREFIX_PATH; it would probably save me some debugging if it was triggering when I didn't expect it to!

[lukeyeager]

Yes, a message() is appropriate here, I agree.

[lukeyeager]

Also, I'm not convinced this is exactly right. If the install prefix is /usr/local and the default value for CMAKE_SYSTEM_PREFIX_PATH is /usr/local;/usr;/;/usr;/usr/local, then this will remove the first /usr/local (standard search path) instead of the last one (install prefix). That will mess with the ordering in a weird way. But I'm not sure we can hard-code removing the 4th value for all CMake versions.

[bradking]

You can use CMAKE_FIND_NO_INSTALL_PREFIX to tell CMake not to add the install prefix to the search path in the first place.

[lukeyeager]

@bradking thanks for chiming in, but that doesn't seem to work, either. The docs say that variable only affects find_package(), but we're calling find_path() and find_library(): https://github.com/caffe2/caffe2/blob/v0.8.1/cmake/Modules/FindBenchmark.cmake

[bradking]

It needs to be set very early, such as before the first project or enable_language command, in order to influence the initialization of CMAKE_SYSTEM_PREFIX_PATH.

[lukeyeager]

Oh wow, that's early. Thanks for the tip - that works!

[ezyang]

I'm willing to accept this but I definitely feel like a lot more testing is necessary.

[lukeyeager]

I'm willing to accept this but I definitely feel like a lot more testing is necessary.

That's fair. But there's only one build configuration I really use and care about. For a wider testing matrix, it would be great if you or @pietern could try this out (since you both have already looked at it).

It would be great if the Travis and Appveyor builds were reliable enough to be helpful here.

[facebook-github-bot]

@lukeyeager updated the pull request - view changes

[lukeyeager]

Thanks for the tip about CMAKE_FIND_NO_INSTALL_PREFIX, @bradking! Could you make that easier to find in the CMake docs? Maybe mention it here or here, for example?

@ezyang now the diff is much more simple.

[bradking]

make that easier to find in the CMake docs

See CMake MR 1213.

[facebook-github-bot]

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.