Fix public protobuf interface (#1961)

* Fix public protobuf interface - wip

* Try turn on custom protobuf in mac jenkins.

* Adding back auto-fallback protobuf option

* Address typos pointed out by reviewers

.jenkins/build.sh

@@ -91,6 +91,13 @@ if [ "$(uname)" == "Linux" ]; then
   CMAKE_ARGS+=("-DUSE_REDIS=ON")
 fi
 
+# Currently, on Jenkins mac os, we will use custom protobuf. Mac OS
+# contbuild at the moment is minimal dependency - it doesn't use glog
+# or gflags either.
+if [ "$(uname)" == "Darwin" ]; then
+  CMAKE_ARGS+=("-DBUILD_CUSTOM_PROTOBUF=ON")
+fi
+
 # Configure
 cmake "${ROOT_DIR}" ${CMAKE_ARGS[*]} "$@"
 

CMakeLists.txt

@@ -17,6 +17,7 @@ set(CAFFE2_VERSION
 include(CMakeDependentOption)
 option(BUILD_BINARY "Build C++ binaries" ON)
 option(BUILD_DOCS "Build documentation" OFF)
+option(BUILD_CUSTOM_PROTOBUF "If set, build Caffe2's own protobuf under third_party" OFF)
 option(BUILD_PYTHON "Build Python binaries" ON)
 option(BUILD_SHARED_LIBS "Build libcaffe2.so" ON)
 cmake_dependent_option(
@@ -207,23 +208,20 @@ endif()
 if (USE_GFLAGS)
   list(APPEND CAFFE2_INTERFACE_LIBS gflags)
 endif()
-if (NOT CAFFE2_USE_CUSTOM_PROTOBUF)
-  list(APPEND CAFFE2_INTERFACE_LIBS ${PROTOBUF_LIBRARIES})
-else()
-  # TODO(jiayq): this is not ideal, as during installation one will not
-  # have the libprotobuf target ready. It is here only so that we can
-  # make in-house cmake builds run (link) properly.
-  list(APPEND CAFFE2_INTERFACE_LIBS libprotobuf)
-endif()
+list(APPEND CAFFE2_INTERFACE_LIBS protobuf::libprotobuf)
+
+# TODO: figure out how we should remove this public dependency.
 if (USE_OPENCV)
   list(APPEND CAFFE2_INTERFACE_LIBS ${OpenCV_LIBS})
 endif()
 
-if ((NOT USE_GLOG) OR (NOT USE_GFLAGS) OR CAFFE2_USE_CUSTOM_PROTOBUF)
+if ((NOT USE_GLOG) OR (NOT USE_GFLAGS) OR BUILD_CUSTOM_PROTOBUF)
   message(WARNING
       "Generated cmake files are only fully tested if one builds "
-      "with system glog, gflags, and protobuf.")
+      "with system glog, gflags, and protobuf. Other settings may "
+      "generate files that are not well tested.")
 endif()
+
 target_link_libraries(caffe2 INTERFACE ${CAFFE2_INTERFACE_LIBS})
 if (USE_CUDA)
   target_link_libraries(caffe2_gpu INTERFACE ${CAFFE2_INTERFACE_LIBS})
@@ -252,6 +250,7 @@ if (BUILD_SHARED_LIBS)
   install(FILES
       ${PROJECT_SOURCE_DIR}/cmake/public/glog.cmake
       ${PROJECT_SOURCE_DIR}/cmake/public/gflags.cmake
+      ${PROJECT_SOURCE_DIR}/cmake/public/protobuf.cmake
       DESTINATION share/cmake/Caffe2/public
       COMPONENT dev)
   install(EXPORT Caffe2Targets DESTINATION share/cmake/Caffe2

caffe/proto/CMakeLists.txt

@@ -1,10 +1,9 @@
-# find_package(Protobuf REQUIRED)
-
 file(GLOB Caffe_PROTOBUF_FILES "${CMAKE_CURRENT_SOURCE_DIR}/*.proto")
 
 caffe2_protobuf_generate_cpp_py(Caffe_PROTO_SRCS Caffe_PROTO_HEADERS Caffe_PROTO_PY ${Caffe_PROTOBUF_FILES})
 
 add_library(Caffe_PROTO OBJECT ${Caffe_PROTO_HEADERS} ${Caffe_PROTO_SRCS})
+
 if (MSVC)
   if(BUILD_SHARED_LIBS)
     set(Caffe2_API_DEFINE "-DCAFFE2_API=__declspec(dllexport)")
@@ -14,4 +13,5 @@ if (MSVC)
   target_compile_definitions(
       Caffe_PROTO PRIVATE ${Caffe2_API_DEFINE})
 endif()
+
 install(FILES ${Caffe_PROTO_HEADERS} DESTINATION include/caffe/proto)

caffe2/proto/CMakeLists.txt

@@ -1,10 +1,9 @@
-# find_package(Protobuf REQUIRED)
-
 file(GLOB Caffe2_PROTOBUF_FILES "${CMAKE_CURRENT_SOURCE_DIR}/*.proto")
 
 caffe2_protobuf_generate_cpp_py(Caffe2_PROTO_SRCS Caffe2_PROTO_HEADERS Caffe2_PROTO_PY ${Caffe2_PROTOBUF_FILES})
 
 add_library(Caffe2_PROTO OBJECT ${Caffe2_PROTO_HEADERS} ${Caffe2_PROTO_SRCS})
+
 if (MSVC)
   if(BUILD_SHARED_LIBS)
     set(Caffe2_API_DEFINE "-DCAFFE2_API=__declspec(dllexport)")
@@ -14,4 +13,5 @@ if (MSVC)
   target_compile_definitions(
       Caffe2_PROTO PRIVATE ${Caffe2_API_DEFINE})
 endif()
+
 install(FILES ${Caffe2_PROTO_HEADERS} DESTINATION include/caffe2/proto)

cmake/Caffe2Config.cmake.in

@@ -38,6 +38,26 @@ if (@USE_GLOG@)
   endif()
 endif()
 
+# Protobuf
+include("${CMAKE_CURRENT_LIST_DIR}/public/protobuf.cmake")
+if (NOT TARGET protobuf::libprotobuf)
+  message(FATAL_ERROR
+      "Your installed Caffe2 version uses protobuf but the protobuf library "
+      "cannot be found. Did you accidentally remove it, or have you set "
+      "the right CMAKE_PREFIX_PATH? If you do not have protobuf, you will "
+      "need to install protobuf and set the library path accordingly.")
+endif()
+message(STATUS "Caffe2: Protobuf version " ${Protobuf_VERSION})
+if (NOT (${Protobuf_VERSION} VERSION_EQUAL @CAFFE2_BUILT_PROTOBUF_VERSION@))
+  message(FATAL_ERROR
+      "Your installed Caffe2 is built with protobuf "
+      @CAFFE2_BUILT_PROTOBUF_VERSION@
+      ", while your current cmake setting discovers protobuf version "
+      ${Protobuf_VERSION}
+      ". Please specify a protobuf version that is the same as the built "
+      "version.")
+endif()
+
 # import targets
 include ("${CMAKE_CURRENT_LIST_DIR}/Caffe2Targets.cmake")
 

cmake/Dependencies.cmake

@@ -82,7 +82,6 @@ if(USE_NNPACK)
   endif()
 endif()
 
-
 # ---[ On Android, Caffe2 uses cpufeatures library in the thread pool
 if (ANDROID)
   # ---[ Check if cpufeatures was already imported

cmake/ProtoBuf.cmake

@@ -1,68 +1,40 @@
 # Finds Google Protocol Buffers library and compilers and extends
 # the standard cmake script with version and python generation support
-function(custom_protobuf_find)
-  set(CAFFE2_USE_CUSTOM_PROTOBUF ON PARENT_SCOPE)
+macro(custom_protobuf_find)
   message(STATUS "Use custom protobuf build.")
   option(protobuf_BUILD_TESTS "" OFF)
   option(protobuf_BUILD_EXAMPLES "" OFF)
   if (APPLE)
     # Protobuf generated files triggers a deprecated atomic operation warning
     # so we turn it off here.
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-declarations" PARENT_SCOPE)
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-declarations")
   endif()
-  # If we are building Caffe2 as shared libs, we will also build protobuf as shared libs.
+  # If we are building Caffe2 as shared libs, we will also build protobuf as
+  # shared libs.
   set(protobuf_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
   # We will make sure that protobuf and caffe2 uses the same msvc runtime.
   set(protobuf_MSVC_STATIC_RUNTIME ${CAFFE2_USE_MSVC_STATIC_RUNTIME})
   if (MSVC AND BUILD_SHARED_LIBS)
     add_definitions(-DPROTOBUF_USE_DLLS)
   endif()
   add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/protobuf/cmake)
-  # To build shared Caffe2 libraries that link to a static protobuf,
-  # we need those static libraries to be compiled as PIC.
-  #set_property(TARGET libprotobuf PROPERTY POSITION_INDEPENDENT_CODE ON)
-  set(PROTOBUF_LIBRARIES libprotobuf PARENT_SCOPE)
-  set(PROTOBUF_INCLUDE_DIRS ${PROJECT_SOURCE_DIR}/third_party/protobuf/src PARENT_SCOPE)
-  set(Caffe2_DEPENDENCY_LIBS ${Caffe2_DEPENDENCY_LIBS} PARENT_SCOPE)
-  # Figure out which protoc to use.
-  # If CAFFE2_CUSTOM_PROTOC_EXECUTABLE is set, we assume the user knows
-  # what they're doing and we blindly use the specified protoc. This
-  # is typically the case when cross-compiling where protoc must be
-  # compiled for the host architecture and libprotobuf must be
-  # compiled for the target architecture.
-  # If CAFFE2_CUSTOM_PROTOC_EXECUTABLE is NOT set, we use the protoc
-  # target that is built as part of including the protobuf project.
-  if(EXISTS "${CAFFE2_CUSTOM_PROTOC_EXECUTABLE}")
-    set(PROTOBUF_PROTOC_EXECUTABLE ${CAFFE2_CUSTOM_PROTOC_EXECUTABLE} PARENT_SCOPE)
-  else()
-    # We cannot use a generator expression to refer to protoc,
-    # because we end up using this value in a DEPENDS clause in
-    # add_custom_command. Support for this was added in CMake v3.0.
-    # See bbffccca42d4f209220e833e1a86e735a5c83339 (v3.0.0-rc1-350-gbbffccc).
-    set(PROTOBUF_PROTOC_EXECUTABLE "${PROJECT_BINARY_DIR}/bin/protoc" PARENT_SCOPE)
-  endif()
-  set(Protobuf_FOUND TRUE PARENT_SCOPE)
-endfunction()
 
-# The following cache variables are also available to set or use:
-#
-#   PROTOBUF_LIBRARY - The protobuf library
-#   PROTOBUF_PROTOC_LIBRARY   - The protoc library
-#   PROTOBUF_INCLUDE_DIR - The include directory for protocol buffers
-#   PROTOBUF_PROTOC_EXECUTABLE - The protoc compiler
-#
-# They are available in CMake 2.8.12 and later.
-#
-if (WIN32)
-  find_package(Protobuf NO_MODULE)
-  if(Protobuf_FOUND OR PROTOBUF_FOUND)
-    set(PROTOBUF_LIBRARIES protobuf::libprotobuf)
-    get_target_property(_protobuf_include_dir protobuf::libprotobuf
-                        INTERFACE_INCLUDE_DIRECTORIES)
-    set(PROTOBUF_INCLUDE_DIRS ${_protobuf_include_dir})
-    add_definitions(-DPROTOBUF_USE_DLLS)
+  # Protobuf "namespaced" target is only added post protobuf 3.5.1. As a
+  # result, for older versions, we will manually add alias.
+  if (NOT TARGET protobuf::libprotobuf)
+    add_library(protobuf::libprotobuf ALIAS libprotobuf)
+    add_library(protobuf::libprotobuf-lite ALIAS libprotobuf-lite)
+    add_executable(protobuf::protoc ALIAS protoc)
   endif()
-elseif (ANDROID OR IOS)
+endmacro()
+
+# Main entry for protobuf. If we are building on Android, iOS or we have hard
+# coded BUILD_CUSTOM_PROTOBUF, we will hard code the use of custom protobuf
+# in the submodule.
+if (ANDROID OR IOS)
+  message(STATUS
+      "For Android and iOS cross compilation, I am automatically using "
+      "custom protobuf under third party.")
   custom_protobuf_find()
   # Unfortunately, new protobuf does not support libprotoc and protoc
   # cross-compilation so we will need to exclude it.
@@ -74,72 +46,54 @@ elseif (ANDROID OR IOS)
   set_target_properties(
       libprotoc protoc PROPERTIES
       EXCLUDE_FROM_ALL 1 EXCLUDE_FROM_DEFAULT_BUILD 1)
+elseif (BUILD_CUSTOM_PROTOBUF)
+  message(STATUS "Building using own protobuf under third_party per request.")
+  custom_protobuf_find()
 else()
-  # Always use libprotobuf from tree if a custom PROTOBUF
-  if(EXISTS "${CAFFE2_CUSTOM_PROTOC_EXECUTABLE}")
-    custom_protobuf_find()
-  else()
-    # protoc is searched for in system PATH (see FindProtobuf.cmake).
-    # Include directories and libraries searched for separately.
-    #
-    # This is problematic in the following example case: if we run in an
-    # Anaconda environment, PATH is set accordingly, and protoc is found
-    # in the Anaconda installation if it is installed there. If
-    # libprotobuf is *also* installed as a system package, then
-    # FindProtobuf.cmake will happily use its include directories and
-    # libraries, as there is nothing guiding CMake to use the ones in
-    # the Anaconda installation. To fix this, we can seed the cache
-    # variables used by FindProtobuf.cmake here.
-    #
-    find_program(PROTOBUF_PROTOC_EXECUTABLE
-      NAMES protoc
-      DOC "The Google Protocol Buffers Compiler")
-
-    # Only if protoc was found, seed the include directories and libraries.
-    # We assume that protoc is installed at PREFIX/bin.
-    # We use get_filename_component to resolve PREFIX.
-    if(PROTOBUF_PROTOC_EXECUTABLE)
-      get_filename_component(
-        _PROTOBUF_INSTALL_PREFIX
-        ${PROTOBUF_PROTOC_EXECUTABLE}
-        DIRECTORY)
-      get_filename_component(
-        _PROTOBUF_INSTALL_PREFIX
-        ${_PROTOBUF_INSTALL_PREFIX}/..
-        REALPATH)
-      find_library(PROTOBUF_LIBRARY
-        NAMES protobuf
-        PATHS ${_PROTOBUF_INSTALL_PREFIX}/lib
-        NO_DEFAULT_PATH)
-      find_library(PROTOBUF_PROTOC_LIBRARY
-        NAMES protoc
-        PATHS ${_PROTOBUF_INSTALL_PREFIX}/lib
-        NO_DEFAULT_PATH)
-      find_library(PROTOBUF_LITE_LIBRARY
-        NAMES protobuf-lite
-        PATHS ${_PROTOBUF_INSTALL_PREFIX}/lib
-        NO_DEFAULT_PATH)
-      find_path(PROTOBUF_INCLUDE_DIR
-        google/protobuf/service.h
-        PATHS ${_PROTOBUF_INSTALL_PREFIX}/include
-        NO_DEFAULT_PATH)
-      find_package(Protobuf)
-    endif()
-
-  endif()
+  include(cmake/public/protobuf.cmake)
 endif()
 
-# If Protobuf is not found, do custom protobuf find.
-if(NOT(Protobuf_FOUND OR PROTOBUF_FOUND))
+if ((NOT TARGET protobuf::libprotobuf) AND (NOT TARGET protobuf::libprotobuf-lite))
+  message(WARNING
+      "Protobuf cannot be found. Caffe2 will automatically switch to use "
+      "own protobuf under third_party. Note that this behavior may change in "
+      "the future, and you will need to specify -DBUILD_CUSTOM_PROTOBUF=ON "
+      "explicitly.")
   custom_protobuf_find()
-endif()
 
-if(NOT(Protobuf_FOUND OR PROTOBUF_FOUND))
-  message(FATAL_ERROR "Could not find protobuf or compile local version")
+  # TODO(jiayq): enable this in the future, when Jenkins Mac support is
+  # properly set up with protobuf installs.
+
+  # message(FATAL_ERROR
+  #     "Protobuf cannot be found. Caffe2 will have to build with libprotobuf. "
+  #     "Please set the proper paths so that I can find protobuf correctly.")
 endif()
 
-caffe2_include_directories(${PROTOBUF_INCLUDE_DIRS})
-list(APPEND Caffe2_DEPENDENCY_LIBS ${PROTOBUF_LIBRARIES})
+# TODO: enable using lite protobuf.
+list(APPEND Caffe2_DEPENDENCY_LIBS protobuf::libprotobuf)
+
+# Protobuf generated files use <> as inclusion path, so following normal
+# convention we will use SYSTEM inclusion path.
+get_target_property(__tmp protobuf::libprotobuf INTERFACE_INCLUDE_DIRECTORIES)
+message(STATUS "Caffe2 protobuf include directory: " ${__tmp})
+include_directories(SYSTEM ${__tmp})
+
+# Set variable used in cmake installation path.
+set(CAFFE2_BUILT_PROTOBUF_VERSION ${Protobuf_VERSION})
+
+# Figure out which protoc to use.
+# If CAFFE2_CUSTOM_PROTOC_EXECUTABLE is set, we assume the user knows
+# what they're doing and we blindly use the specified protoc. This
+# is typically the case when cross-compiling where protoc must be
+# compiled for the host architecture and libprotobuf must be
+# compiled for the target architecture.
+# If CAFFE2_CUSTOM_PROTOC_EXECUTABLE is NOT set, we use the protoc
+# target that is built as part of including the protobuf project.
+if(EXISTS "${CAFFE2_CUSTOM_PROTOC_EXECUTABLE}")
+  set(CAFFE2_PROTOC_EXECUTABLE ${CAFFE2_CUSTOM_PROTOC_EXECUTABLE})
+else()
+  set(CAFFE2_PROTOC_EXECUTABLE protobuf::protoc)
+endif()
 
 ################################################################################################
 # Modification of standard 'protobuf_generate_cpp()' with output dir parameter and python support
@@ -178,9 +132,9 @@ function(caffe2_protobuf_generate_cpp_py srcs_var hdrs_var python_var)
              "${CMAKE_CURRENT_BINARY_DIR}/${fil_we}_pb2.py"
       WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
       COMMAND ${CMAKE_COMMAND} -E make_directory "${CMAKE_CURRENT_BINARY_DIR}"
-      COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} -I${PROJECT_SOURCE_DIR} --cpp_out=${DLLEXPORT_STR}${PROJECT_BINARY_DIR} ${abs_fil}
-      COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} -I${PROJECT_SOURCE_DIR} --python_out "${PROJECT_BINARY_DIR}" ${abs_fil}
-      DEPENDS ${PROTOBUF_PROTOC_EXECUTABLE} ${abs_fil}
+      COMMAND ${CAFFE2_PROTOC_EXECUTABLE} -I${PROJECT_SOURCE_DIR} --cpp_out=${DLLEXPORT_STR}${PROJECT_BINARY_DIR} ${abs_fil}
+      COMMAND ${CAFFE2_PROTOC_EXECUTABLE} -I${PROJECT_SOURCE_DIR} --python_out "${PROJECT_BINARY_DIR}" ${abs_fil}
+      DEPENDS ${CAFFE2_PROTOC_EXECUTABLE} ${abs_fil}
       COMMENT "Running C++/Python protocol buffer compiler on ${fil}" VERBATIM )
   endforeach()
 
@@ -189,3 +143,5 @@ function(caffe2_protobuf_generate_cpp_py srcs_var hdrs_var python_var)
   set(${hdrs_var} ${${hdrs_var}} PARENT_SCOPE)
   set(${python_var} ${${python_var}} PARENT_SCOPE)
 endfunction()
+
+

cmake/Summary.cmake

@@ -12,6 +12,7 @@ function (caffe2_print_configuration_summary)
   message(STATUS "  Protobuf compiler     : ${PROTOBUF_PROTOC_EXECUTABLE}")
   message(STATUS "  Protobuf include path : ${PROTOBUF_INCLUDE_DIRS}")
   message(STATUS "  Protobuf libraries    : ${PROTOBUF_LIBRARIES}")
+  message(STATUS "  BLAS                  : ${BLAS}")
   message(STATUS "  CXX flags             : ${CMAKE_CXX_FLAGS}")
   message(STATUS "  Build type            : ${CMAKE_BUILD_TYPE}")
   get_directory_property(tmp DIRECTORY ${PROJECT_SOURCE_DIR} COMPILE_DEFINITIONS)