Make path to append_metadata.cmake relative to top-level CMakeLists.txt #11635
Conversation
There was a problem hiding this comment.
Thanks a lot for filing this correcting the limitations introduced.
Could you check whether the proposed change works the same? If so, I would prefer the more generic PROJECT_BINARY_DIR given it's used already elsewhere in the CMake logic.
One additional request, independent from the PR: would it be possible to submit a demo CMake that showcases the setup where this was a problem? Then that can be used as part of CI to check modifying the CMake logic is not impacting your setup.
I can help move that into a CI check, but even just a CMake with a add_subproject or so would be cool to have, possibly in an examples folder.
Thanks!
| @@ -820,7 +820,7 @@ function(build_loadable_extension_directory NAME OUTPUT_DIRECTORY EXTENSION_VERS | |||
| TARGET ${TARGET_NAME} | |||
| POST_BUILD | |||
| COMMAND | |||
| ${CMAKE_COMMAND} -DEXTENSION=$<TARGET_FILE:${TARGET_NAME}> -DPLATFORM_FILE=${CMAKE_BINARY_DIR}/duckdb_platform_out -DDUCKDB_VERSION="${DUCKDB_NORMALIZED_VERSION}" -DEXTENSION_VERSION="${EXTENSION_VERSION}" -DNULL_FILE=${CMAKE_SOURCE_DIR}/scripts/null.txt -P ${CMAKE_SOURCE_DIR}/scripts/append_metadata.cmake | |||
| ${CMAKE_COMMAND} -DEXTENSION=$<TARGET_FILE:${TARGET_NAME}> -DPLATFORM_FILE=${DuckDB_BINARY_DIR}/duckdb_platform_out -DDUCKDB_VERSION="${DUCKDB_NORMALIZED_VERSION}" -DEXTENSION_VERSION="${EXTENSION_VERSION}" -DNULL_FILE=${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/null.txt -P ${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/append_metadata.cmake | |||
There was a problem hiding this comment.
| ${CMAKE_COMMAND} -DEXTENSION=$<TARGET_FILE:${TARGET_NAME}> -DPLATFORM_FILE=${DuckDB_BINARY_DIR}/duckdb_platform_out -DDUCKDB_VERSION="${DUCKDB_NORMALIZED_VERSION}" -DEXTENSION_VERSION="${EXTENSION_VERSION}" -DNULL_FILE=${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/null.txt -P ${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/append_metadata.cmake | |
| ${CMAKE_COMMAND} -DEXTENSION=$<TARGET_FILE:${TARGET_NAME}> -DPLATFORM_FILE=${PROJECT_BINARY_DIR}/duckdb_platform_out -DDUCKDB_VERSION="${DUCKDB_NORMALIZED_VERSION}" -DEXTENSION_VERSION="${EXTENSION_VERSION}" -DNULL_FILE=${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/null.txt -P ${CMAKE_CURRENT_FUNCTION_LIST_DIR}/scripts/append_metadata.cmake |
Would PROJECT_BINARY_DIR work? That is currently used for determining the location of duckdb_platform_binary in:
set_target_properties(duckdb_platform_binary PROPERTIES RUNTIME_OUTPUT_DIRECTORY
${PROJECT_BINARY_DIR})and implicitly for the location of duckdb_platform_out.
I think it should work the same, and avoid the naming that looks more confusing to me.
There was a problem hiding this comment.
PROJECT_BINARY_DIR will not work when the build_loadable_extension function is called from another CMake project. For example, for the Parquet extension:
duckdb/extension/parquet/CMakeLists.txt
Line 68 in e0c4d9c
The PROJECT_BINARY_DIR inside the function would be build/release/extension/parquet, not build/release.
There was a problem hiding this comment.
Thanks for double checking.
Hey @carlopi. I've built a repro using the extension template. But instead of using the Now when running
Let me know if this is helpful. |
|
This is good to go for me. |
|
Thanks! |
Merge pull request duckdb/duckdb#11630 from pdet/gz_buffers Merge pull request duckdb/duckdb#11635 from motherduckdb/flo/fix-scripts-path-cmake
Follow-up to 0ecc682:
CMAKE_SOURCE_DIRis used to specify the location of the scripts directory. However, this variable does not point to the duckdb top-level directory if DuckDB is built from within another CMake project, e.g. for extensions. I replaced the variable withCMAKE_CURRENT_FUNCTION_LIST_DIRwhich gives the directory of the CMakeLists.txt where thebuild_loadable_extension_directoryfunction is defined—in other words, the top-level directory.Similarly, when looking for the
duckdb_platform_outfile,CMAKE_BINARY_DIRis the wrong path when DuckDB is not the top-level CMake project. I replaced it withDuckDB_BINARY_DIRwhere "DuckDB" is thePROJECT-NAME.