search

Farama-Foundation / Arcade-Learning-Environment

The Arcade Learning Environment (ALE) -- a platform for AI research.
GNU General Public License v2.0
2.1k stars 416 forks source link

Wrong usage of CMAKE_MODULE_PATH in configure_package_config_file #530

Closed maichmueller closed 1 month ago

maichmueller commented 1 month ago

I tried pulling in this project via CPM (a FetchContent CMake wrapper) like so

CPMAddPackage(
        NAME ale  # The unique name of the dependency (should be the exported target's name)
        GIT_TAG v0.9.0
        # Configuration options passed to the dependency (optional)
        SYSTEM TRUE
        BUILD_CPP_LIB=ON
        BUILD_PYTHON_LIB=OFF
        DOWNLOAD_ONLY FALSE       # If set, the project is downloaded, but not configured (optional)
        GITHUB_REPOSITORY Farama-Foundation/Arcade-Learning-Environment
)

but this leads to an error:

CMake Error at /u/michael.aichmueller/cmake-3.26.5-linux-x86_64/share/cmake-3.26/Modules/CMakePackageConfigHelpers.cmake:250 (message):
Unknown keywords given to CONFIGURE_PACKAGE_CONFIG_FILE():
“ale-config.cmake”
Call Stack (most recent call first):
cmake-build-debug-clang-lactose/_deps/ale-src/src/CMakeLists.txt:105 (configure_package_config_file)

due to using CMAKE_MODULE_PATH in configure_package_config_file as such

# Configure installable cmake config
  configure_package_config_file(
    ${CMAKE_MODULE_PATH}/${PROJECT_NAME}-config.cmake.in
    ${PROJECT_NAME}-config.cmake
    INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})
...

in src/CMakeLists.txt. This leads to expanding the CMAKE_MODULE_PATH (which is a list) as multiple args blocking input AND output of the function, rendering ale-config.cmake to no longer be parsed as the output arg, but as a keyword arg (and consequently the error). Without knowing what the intentions were, this usage seems incorrect.

This could be fixed by replacing ${CMAKE_MODULE_PATH} with ${CMAKE_CURRENT_SOURCE_DIR}/../cmake in the 1st argument.

maichmueller commented 1 month ago

The PR #531 would be the quick solution I suggested. With this fix, I can configure the project with CPM without problems.

Happy to hear your thoughts on this!

pseudo-rnd-thoughts commented 1 month ago

Amazing, thanks for the issue and quick debugging with a solution. We love issues like this. If the PR passes the CI then we should be able to merge