Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake fix in install rules #487

Closed
Zbyl opened this issue Jun 16, 2023 · 6 comments
Closed

CMake fix in install rules #487

Zbyl opened this issue Jun 16, 2023 · 6 comments
Assignees
Labels
Bug For issues or changes that describe or fix bugs. CMake Use to associate with the CMake tool.

Comments

@Zbyl
Copy link

Zbyl commented Jun 16, 2023

CMake has a bug:

	# install libraries
	install(TARGETS PlayRho
		EXPORT PlayRho-targets
		LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
		ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
		RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
		INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
		COMPONENT Library
	)

COMPONENT Library must be the second line. When it is the last one, both words are interpreted as include directories!

(See similar issue here: https://gitlab.kitware.com/cmake/cmake/-/issues/20636)

@louis-langholtz louis-langholtz self-assigned this Jun 17, 2023
@louis-langholtz louis-langholtz added Bug For issues or changes that describe or fix bugs. CMake Use to associate with the CMake tool. labels Jun 17, 2023
@louis-langholtz
Copy link
Owner

louis-langholtz commented Jun 17, 2023

@Zbyl Thank you for your interest in PlayRho and for filing this issue!

Looking at the latest CMake Installing Targets help, I see the following usage specification:

install(TARGETS targets... [EXPORT <export-name>]
        [RUNTIME_DEPENDENCIES args...|RUNTIME_DEPENDENCY_SET <set-name>]
        [[ARCHIVE|LIBRARY|RUNTIME|OBJECTS|FRAMEWORK|BUNDLE|
          PRIVATE_HEADER|PUBLIC_HEADER|RESOURCE|FILE_SET <set-name>|CXX_MODULES_BMI]
         [DESTINATION <dir>]
         [PERMISSIONS permissions...]
         [CONFIGURATIONS [Debug|Release|...]]
         [COMPONENT <component>]
         [NAMELINK_COMPONENT <component>]
         [OPTIONAL] [EXCLUDE_FROM_ALL]
         [NAMELINK_ONLY|NAMELINK_SKIP]
        ] [...]
        [INCLUDES DESTINATION [<dir> ...]]
        )

Reading this now and the issue you had linked, I'm recognizing how having the line

COMPONENT Library

after the

INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}

line (found in the PlayRho/PlayRho/CMakeLists.txt file) appears riskier.

I imagine this risk depends though on just how order dependent CMake is, and how greedily CMake evaluates arguments like <dir> ... - versus keywords. At least based on the above usage however, seems like COMPONENT is better listed before INCLUDES DESTINATION as that seems to be closer to the order suggested. Which seems to suggest:

install(TARGETS PlayRho
	EXPORT PlayRho-targets
	COMPONENT Library
	LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
	ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
	RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
	INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
	)

Are you actually running into a problem with this, or just reporting this riskiness?

I ask in part because none of my manual testing had detected this being a problem. CMake seems too fragile to me however and the usage specification suggests it could be a problem. I'd like to see the parse tree CMake builds out of this CMakeLists.txt file but the best I'm finding is the output of CMake with --trace-expand.

louis-langholtz added a commit that referenced this issue Jun 18, 2023
See also response by Craig Scott at
https://gitlab.kitware.com/cmake/cmake/-/issues/20636
asserting that having COMPONENT after INCLUDES DESTINATION is
"malformed".
louis-langholtz added a commit that referenced this issue Jun 18, 2023
See also response by Craig Scott at
https://gitlab.kitware.com/cmake/cmake/-/issues/20636
asserting that having COMPONENT after INCLUDES DESTINATION is
"malformed".
@louis-langholtz
Copy link
Owner

Need to also back port changes from #488 into release-1.1.

@Zbyl
Copy link
Author

Zbyl commented Jun 18, 2023

Hi.
This is an actual problem I've run into.
I wanted to use PlayRho with Vcpkg, so I've created a portfile.
But Vcpkg was reporting errors during installation about include directories (COMPONENT and Library) that don't exist.
This is when I found out that order of the CMake "keywords" in install() matters.

My Vcpkg "port" looks like this (I've took one for box2d as an example to create it)

vcpkg.json

{
  "name": "playrho",
  "version-semver": "1.1.1",
  "port-version": 1,
  "description": "Real-time oriented physics engine and library that's currently best suited for 2D games. ",
  "homepage": "https://louis-langholtz.github.io/PlayRho/",
  "supports": "!uwp",
  "dependencies": [
    {
      "name": "vcpkg-cmake",
      "host": true
    },
    {
      "name": "vcpkg-cmake-config",
      "host": true
    }
  ]
}

portfile.cmake

vcpkg_check_linkage(ONLY_STATIC_LIBRARY)

vcpkg_from_github(
    OUT_SOURCE_PATH SOURCE_PATH
    REPO louis-langholtz/PlayRho
    REF fd50d14d747d892e26d8d445e123d65b4f37c161  #v1.1.1
    SHA512 25a7b403a34aac22e83cc46aff796f39e92f2124e5e3c0ed5a8ba479ce840fba592b7b0216b142e51fb0b4b3c5e33770d0535f78467499e4804717be668c2082
    HEAD_REF master
)

vcpkg_cmake_configure(
    SOURCE_PATH "${SOURCE_PATH}"
    OPTIONS
        -DPLAYRHO_INSTALL=ON
        -DPLAYRHO_BUILD_HELLOWORLD=OFF
        -DPLAYRHO_BUILD_UNIT_TESTS=OFF
        -DPLAYRHO_BUILD_BENCHMARK=OFF
        -DPLAYRHO_BUILD_TESTBED=OFF
)
vcpkg_cmake_install()

file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")

vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/playrho)

vcpkg_copy_pdbs()

file(INSTALL "${SOURCE_PATH}/LICENSE.txt" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}" RENAME copyright)

@louis-langholtz
Copy link
Owner

@Zbyl Thank you again. That's very appreciated insight & your vcpkg port is also super appreciated!

I have merged a change to master to address this. I intend to back port this next to the release 1.1 branch and associated release.

louis-langholtz added a commit that referenced this issue Jun 18, 2023
See also response by Craig Scott at
https://gitlab.kitware.com/cmake/cmake/-/issues/20636
asserting that having COMPONENT after INCLUDES DESTINATION is
"malformed".

(cherry picked from commit 6c207ae)
@louis-langholtz
Copy link
Owner

v1.1.2 has been released now with the change in it to address this. Hopefully it fixes this problem and doesn't introduce any new ones. Please let me know what your experience with it is. Thanks again.

@louis-langholtz
Copy link
Owner

Gonna assume this is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For issues or changes that describe or fix bugs. CMake Use to associate with the CMake tool.
Projects
None yet
Development

No branches or pull requests

2 participants