From 926d70ec0483c80878726296667c1d39e06c8dda Mon Sep 17 00:00:00 2001 From: Dave Bort Date: Thu, 25 Apr 2024 10:48:28 -0700 Subject: [PATCH] Add EXECUTORCH_SEPARATE_FLATCC_HOST_PROJECT cmake option (#3356) Summary: Currently, we always build two copies of the flatcc targets, just in case we happen to be cross-compiling. But because the flatcc project puts its binaries in the source directory, those two copies can interfere with each other. We don't need to build two copies when not cross-compiling, so add a new option to avoid the second "host" build. Eventually we should only enable this when cross-compiling, but for now disable it when building the pip package (which is never cross-compiled). Pull Request resolved: https://github.com/pytorch/executorch/pull/3356 Test Plan: `rm -rf pip-out && ./install_requirements.sh` succeeded. Looking in the `pip-out/temp.*/cmake-out` directory, there is no `_host_build` directory, but the etdump headers were successfully generated under `pip-out/temp.*/cmake-out/sdk/include/executorch/sdk/etdump/`. Reviewed By: malfet, larryliu0820 Differential Revision: D56582507 Pulled By: dbort fbshipit-source-id: 4ce6c680657bc57cfcf016826364a3f46c4c953e --- sdk/CMakeLists.txt | 81 ++++++++++++++++++++++++++++++---------------- setup.py | 5 +++ 2 files changed, 59 insertions(+), 27 deletions(-) diff --git a/sdk/CMakeLists.txt b/sdk/CMakeLists.txt index 96e2526f8de..73b30008c0b 100644 --- a/sdk/CMakeLists.txt +++ b/sdk/CMakeLists.txt @@ -69,18 +69,33 @@ include(ExternalProject) set(_program_schema__include_dir "${CMAKE_BINARY_DIR}/sdk/include") set(_bundled_schema__include_dir "${CMAKE_BINARY_DIR}/sdk/bundled_program") -# Add the host project -# lint_cmake: -readability/wonkycase -ExternalProject_Add( - flatcc_project - PREFIX ${CMAKE_BINARY_DIR}/_host_build - SOURCE_DIR ${_flatcc_source_dir} - BINARY_DIR ${CMAKE_BINARY_DIR}/_host_build - CMAKE_CACHE_ARGS -DFLATCC_TEST:BOOL=OFF -DFLATCC_REFLECTION:BOOL=OFF +# TODO(dbort): Only enable this when cross-compiling. It can cause build race +# conditions (libflatcc.a errors) when enabled. +option(EXECUTORCH_SEPARATE_FLATCC_HOST_PROJECT + "Whether to build the flatcc commandline tool as a separate project" ON) + +if(EXECUTORCH_SEPARATE_FLATCC_HOST_PROJECT) + # Add the host project. We build this separately so that we can generate + # headers on the host during the build, even if we're cross-compiling the + # flatcc runtime to a different architecture. + + # lint_cmake: -readability/wonkycase + ExternalProject_Add( + flatcc_project + PREFIX ${CMAKE_BINARY_DIR}/_host_build + SOURCE_DIR ${_flatcc_source_dir} + BINARY_DIR ${CMAKE_BINARY_DIR}/_host_build + CMAKE_CACHE_ARGS + -DFLATCC_TEST:BOOL=OFF -DFLATCC_REFLECTION:BOOL=OFF # See above comment about POSITION_INDEPENDENT_CODE. -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON - INSTALL_COMMAND "" # Prevent the install step, modify as needed -) + INSTALL_COMMAND "" # Prevent the install step + ) + set(_etdump_schema_gen_dep flatcc_project) +else() + # If we're not cross-compiling, we can just use the plain commandline target. + set(_etdump_schema_gen_dep flatcc_cli) +endif() set(_etdump_schema__outputs) foreach(fbs_file ${_etdump_schema_names}) @@ -114,33 +129,45 @@ file(MAKE_DIRECTORY ${_program_schema__include_dir}/executorch/sdk/etdump) file(MAKE_DIRECTORY ${_program_schema__include_dir}/executorch/sdk/bundled_program) -add_custom_command( - OUTPUT ${_etdump_schema__outputs} - COMMAND - # Note that the flatcc project actually writes its outputs into the source - # tree instead of under the binary directory, and there's no way to change - # that behavior. - ${_flatcc_source_dir}/bin/flatcc -cwr -o - ${_program_schema__include_dir}/executorch/sdk/etdump - ${_etdump_schema__srcs} +if(EXECUTORCH_SEPARATE_FLATCC_HOST_PROJECT) + # If we cross-compiling, we need to use the version of the commandline tool + # built for the host. + set(_etdump_schema_gen_dep flatcc_project) + # TODO(dbort): flatcc installs its files directly in its source directory - # instead of under CMAKE_BINARY_DIR, and it has no options to avoid - # doing this. We build flatcc twice in the executorch build: once to get - # the `flatcc` host commandline tool, and once to get the (potentially + # instead of under CMAKE_BINARY_DIR, and it has no options to avoid doing + # this. We build flatcc twice in the executorch build: once to get the + # `flatcc` host commandline tool, and once to get the (potentially # cross-compiled) target runtime library. The host build will put its outputs - # in the source tree, making the cross-compiling target build think that - # the outputs have already been built. It will then try to link against the + # in the source tree, making the cross-compiling target build think that the + # outputs have already been built. It will then try to link against the # host-architecture libraries, failing when cross-compiling. To work around # this, delete the host outputs after running this command (which only runs # when setting up the cmake files, not when actually building). This leaves # room for the target build to put its own files in the source tree. We should # try to remove this hack, ideally by submitting an upstream PR that adds an # option to change the installation location. + set(_etdump_schema_cleanup_paths ${_flatcc_source_dir}/bin/* + ${_flatcc_source_dir}/lib/*) +else() + # If we're not cross-compiling we can use the plain commandline target, and we + # don't need to delete any files. + set(_etdump_schema_gen_dep flatcc_cli) + set(_etdump_schema_cleanup_paths "") +endif() + +add_custom_command( + OUTPUT ${_etdump_schema__outputs} COMMAND - rm -f ${_flatcc_source_dir}/bin/* - ${_flatcc_source_dir}/lib/* + # Note that the flatcc project actually writes its outputs into the source + # tree instead of under the binary directory, and there's no way to change + # that behavior. + ${_flatcc_source_dir}/bin/flatcc -cwr -o + ${_program_schema__include_dir}/executorch/sdk/etdump + ${_etdump_schema__srcs} + COMMAND rm -f ${_etdump_schema_cleanup_paths} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/sdk - DEPENDS flatcc_project + DEPENDS ${_etdump_schema_gen_dep} COMMENT "Generating etdump headers" VERBATIM) diff --git a/setup.py b/setup.py index 92fd1e3f778..058af17940e 100644 --- a/setup.py +++ b/setup.py @@ -430,6 +430,11 @@ def run(self): "-DEXECUTORCH_ENABLE_LOGGING=ON", "-DEXECUTORCH_LOG_LEVEL=Info", "-DCMAKE_OSX_DEPLOYMENT_TARGET=10.15", + # The separate host project is only required when cross-compiling, + # and it can cause build race conditions (libflatcc.a errors) when + # enabled. TODO(dbort): Remove this override once this option is + # managed by cmake itself. + "-DEXECUTORCH_SEPARATE_FLATCC_HOST_PROJECT=OFF", ] build_args = [f"-j{self.parallel}"]