Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

Commit

Permalink
cmake: stop including files from the install directory
Browse files Browse the repository at this point in the history
Summary:
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](https://github.com/caffe2/caffe2/blob/v0.8.1/cmake/Dependencies.cmake#L98-L100))
* On installation, the benchmark sub-project installs its headers to `CMAKE_INSTALL_PREFIX` ([see here](https://github.com/google/benchmark/blob/4bf28e611b/src/CMakeLists.txt#L41-L44))
* On a rebuild, CMake searches the system again for a benchmark installation (see #916 for details on why the first search is not cached)
* CMake includes `CMAKE_INSTALL_PREFIX` when searching the system ([docs](https://cmake.org/cmake/help/v3.0/variable/CMAKE_SYSTEM_PREFIX_PATH.html))
* 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](https://github.com/caffe2/caffe2/blob/v0.8.1/cmake/Dependencies.cmake#L97)). 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.
Closes #1112

Differential Revision: D5761750

Pulled By: Yangqing

fbshipit-source-id: 2240088994ffafdb6eedb3626d898b505a4ba564
  • Loading branch information
lukeyeager authored and facebook-github-bot committed Sep 2, 2017
1 parent fb4bf52 commit 251d2d0
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 6 deletions.
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)

message(STATUS "Setting CMAKE_FIND_NO_INSTALL_PREFIX")
set(CMAKE_FIND_NO_INSTALL_PREFIX TRUE)

project(Caffe2 CXX C)

# TODO(bwasti): versioning
Expand Down
14 changes: 8 additions & 6 deletions cmake/Modules/FindBenchmark.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,24 @@
# Benchmark_LIBRARIES - libraries needed to use benchmark

find_path(Benchmark_INCLUDE_DIR
NAMES benchmark/benchmark.h
DOC "The directory where benchmark includes reside"
NAMES benchmark/benchmark.h
NO_SYSTEM_ENVIRONMENT_PATH
DOC "The directory where benchmark includes reside"
)

find_library(Benchmark_LIBRARY
NAMES benchmark
DOC "The benchmark library"
NAMES benchmark
NO_SYSTEM_ENVIRONMENT_PATH
DOC "The benchmark library"
)

set(Benchmark_INCLUDE_DIRS ${Benchmark_INCLUDE_DIR})
set(Benchmark_LIBRARIES ${Benchmark_LIBRARY})

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Benchmark
FOUND_VAR Benchmark_FOUND
REQUIRED_VARS Benchmark_INCLUDE_DIR Benchmark_LIBRARY
FOUND_VAR Benchmark_FOUND
REQUIRED_VARS Benchmark_INCLUDE_DIR Benchmark_LIBRARY
)

mark_as_advanced(Benchmark_FOUND)

0 comments on commit 251d2d0

Please sign in to comment.