Commit 875dd11c authored by Adam Simpkins's avatar Adam Simpkins Committed by Facebook Github Bot

fbcode_builder: cmake: propagate thrift include dependencies correctly

Summary:
Update `add_thrift_cpp2_library()` to pass in the correct `-I` flags when
invoking the thrift compiler so that it can find all of the other thrift files
that this library depends on.

D16062657 was a previous attempt to do this, but suffered from a few problems:
- It required all dependencies to be defined before
  `add_thrift_cpp2_library()` was called.  This requires users to carefully
  order their CMake files and subdirectory include ordering.
- It only handled one level of dependencies, and did not propagate include
  paths for deeper dependencies.
- It set the include path for dependencies to the source directory path where
  the dependency was originally built, rather than the directory where the
  thrift file for that dependency would be installed.

This change does require CMake 3.8+.  Previous versions of CMake do not
support using generator expressions to generate multiple arguments for a
custom command.

Reviewed By: strager

Differential Revision: D17005381

fbshipit-source-id: 31190beba94b4d1010445375a5e2791450230f7d
parent 8541be76
...@@ -17,13 +17,24 @@ include(FBCMakeParseArgs) ...@@ -17,13 +17,24 @@ include(FBCMakeParseArgs)
# The sub-directory where generated headers will be installed. # The sub-directory where generated headers will be installed.
# Defaults to "include" if not specified. The caller must still call # Defaults to "include" if not specified. The caller must still call
# install() to install the thrift library if desired. # install() to install the thrift library if desired.
# - THRIFT_INCLUDE_DIR <path>
# The sub-directory where generated headers will be installed.
# Defaults to "${INCLUDE_DIR}/thrift-files" if not specified.
# The caller must still call install() to install the thrift library if
# desired.
function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
# Parse the arguments # Parse the arguments
set(one_value_args INCLUDE_DIR) set(one_value_args INCLUDE_DIR)
set(multi_value_args SERVICES DEPENDS OPTIONS) set(multi_value_args SERVICES DEPENDS OPTIONS THRIFT_INCLUDE_DIR)
fb_cmake_parse_args( fb_cmake_parse_args(
ARG "" "${one_value_args}" "${multi_value_args}" "${ARGN}" ARG "" "${one_value_args}" "${multi_value_args}" "${ARGN}"
) )
if(NOT DEFINED ARG_INCLUDE_DIR)
set(ARG_INCLUDE_DIR "include")
endif()
if(NOT DEFINED ARG_THRIFT_INCLUDE_DIR)
set(ARG_THRIFT_INCLUDE_DIR "${ARG_INCLUDE_DIR}/thrift-files")
endif()
get_filename_component(base ${THRIFT_FILE} NAME_WE) get_filename_component(base ${THRIFT_FILE} NAME_WE)
get_filename_component( get_filename_component(
...@@ -39,9 +50,6 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) ...@@ -39,9 +50,6 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
"${CMAKE_CURRENT_SOURCE_DIR}/${THRIFT_FILE}" "${CMAKE_CURRENT_SOURCE_DIR}/${THRIFT_FILE}"
) )
get_filename_component(include_prefix ${include_prefix} DIRECTORY) get_filename_component(include_prefix ${include_prefix} DIRECTORY)
if(NOT DEFINED ARG_INCLUDE_DIR)
set(ARG_INCLUDE_DIR "include")
endif()
if (NOT "${include_prefix}" STREQUAL "") if (NOT "${include_prefix}" STREQUAL "")
list(APPEND ARG_OPTIONS "include_prefix=${include_prefix}") list(APPEND ARG_OPTIONS "include_prefix=${include_prefix}")
...@@ -78,22 +86,21 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) ...@@ -78,22 +86,21 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
) )
endforeach() endforeach()
list(APPEND thrift_include_options -I "${CMAKE_SOURCE_DIR}") # This generator expression gets the list of include directories required
foreach(depends IN LISTS ARG_DEPENDS) # for all of our dependencies.
get_property(thrift_include_directory # It requires using COMMAND_EXPAND_LISTS in the add_custom_command() call
TARGET ${depends} # below. COMMAND_EXPAND_LISTS is only available in CMake 3.8+
PROPERTY THRIFT_INCLUDE_DIRECTORY) # If we really had to support older versions of CMake we would probably need
# to use a wrapper script around the thrift compiler that could take the
if (thrift_include_directory STREQUAL "") # include list as a single argument and split it up before invoking the
message(STATUS "No thrift dependency found for ${depends}") # thrift compiler.
else() if (NOT POLICY CMP0067)
list( message(FATAL_ERROR "add_thrift_cpp2_library() requires CMake 3.8+")
APPEND thrift_include_options
-I "${thrift_include_directory}"
)
endif() endif()
endforeach() set(
thrift_include_options
"-I;$<JOIN:$<TARGET_PROPERTY:${LIB_NAME}.thrift_includes,INTERFACE_INCLUDE_DIRECTORIES>,;-I;>"
)
file( file(
GLOB_RECURSE THRIFT_TEMPLATE_FILES GLOB_RECURSE THRIFT_TEMPLATE_FILES
FOLLOW_SYMLINKS "${FBTHRIFT_TEMPLATES_DIR}/cpp2/*.mustache" FOLLOW_SYMLINKS "${FBTHRIFT_TEMPLATES_DIR}/cpp2/*.mustache"
...@@ -104,6 +111,7 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) ...@@ -104,6 +111,7 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
OUTPUT OUTPUT
${generated_headers} ${generated_headers}
${generated_sources} ${generated_sources}
COMMAND_EXPAND_LISTS
COMMAND COMMAND
"${CMAKE_COMMAND}" -E make_directory "${output_dir}" "${CMAKE_COMMAND}" -E make_directory "${output_dir}"
COMMAND COMMAND
...@@ -111,7 +119,7 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) ...@@ -111,7 +119,7 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
--strict --strict
--templates "${FBTHRIFT_TEMPLATES_DIR}" --templates "${FBTHRIFT_TEMPLATES_DIR}"
--gen "mstch_cpp2:${GEN_ARG_STR}" --gen "mstch_cpp2:${GEN_ARG_STR}"
${thrift_include_options} "${thrift_include_options}"
-o "${output_dir}" -o "${output_dir}"
"${CMAKE_CURRENT_SOURCE_DIR}/${THRIFT_FILE}" "${CMAKE_CURRENT_SOURCE_DIR}/${THRIFT_FILE}"
WORKING_DIRECTORY WORKING_DIRECTORY
...@@ -159,11 +167,27 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE) ...@@ -159,11 +167,27 @@ function(add_thrift_cpp2_library LIB_NAME THRIFT_FILE)
PROPERTY PUBLIC_HEADER ${generated_headers} PROPERTY PUBLIC_HEADER ${generated_headers}
) )
# Define a dummy interface library to help propagate the thrift include
# directories between dependencies.
add_library("${LIB_NAME}.thrift_includes" INTERFACE)
target_include_directories(
"${LIB_NAME}.thrift_includes"
INTERFACE
"$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}>"
"$<INSTALL_INTERFACE:${ARG_THRIFT_INCLUDE_DIR}>"
)
foreach(dep IN LISTS ARG_DEPENDS)
target_link_libraries(
"${LIB_NAME}.thrift_includes"
INTERFACE "${dep}.thrift_includes"
)
endforeach()
set_target_properties( set_target_properties(
"${LIB_NAME}" "${LIB_NAME}"
PROPERTIES PROPERTIES
EXPORT_PROPERTIES "THRIFT_INCLUDE_DIRECTORY" EXPORT_PROPERTIES "THRIFT_INSTALL_DIR"
THRIFT_INCLUDE_DIRECTORY "${CMAKE_SOURCE_DIR}" THRIFT_INSTALL_DIR "${ARG_THRIFT_INCLUDE_DIR}/${include_prefix}"
HEADER_INSTALL_DIR "${ARG_INCLUDE_DIR}/${include_prefix}/gen-cpp2" HEADER_INSTALL_DIR "${ARG_INCLUDE_DIR}/${include_prefix}/gen-cpp2"
) )
endfunction() endfunction()
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment