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

Avoid polluting global include and link dirs #20

Merged
merged 9 commits into from
Nov 29, 2020
Merged

Avoid polluting global include and link dirs #20

merged 9 commits into from
Nov 29, 2020

Conversation

rumblehhh
Copy link
Contributor

The use of include_directories and link_directories pollutes global include and link directories. This change adds IMPORTED targets using the external projects and then links the funchook targets against them.
This also avoids issues seen where captone fails to be found at link time "error: ld returned 1 exit status cannot find -lcapstone" when making use of certain toolchain files.

The use of include_directories and link_directories pollutes global include and link directories. This change adds IMPORTED targets using the external projects and then links the funchook targets against them.
This also avoids issues seen where captone fails to be found at link time "error: ld returned 1 exit status cannot find -lcapstone" when making use of certain toolchain files.
@kubo
Copy link
Owner

kubo commented Nov 25, 2020

This PR doesn't pass tests.
See https://travis-ci.org/github/kubo/funchook/pull_requests

INTERFACE_INCLUDE_DIRECTORIES does not allow non-existent directories so ensure the directory is valid using file(MAKE_DIRECTORY). See  https://gitlab.kitware.com/cmake/cmake/-/issues/15052 for details.
@rumblehhh
Copy link
Contributor Author

Aha I should have cleaned my workspace before making these changes. The ci tests should be fixed now.

rumblehhh and others added 7 commits November 26, 2020 10:32
The capstone target was previously being setup using ExternalProject_Get_Property however installing the external project and using the generic include/lib directories solves issues with config based generators.
GNUInstallDirs may not always output to lib and may instead output to lib or lib64 or lib/<multiarch-tuple> on Debian.
Avoid "Unable to determine default CMAKE_INSTALL_LIBDIR directory because no target architecture is known.  Please enable at least one language before including GNUInstallDirs."
Tag 3.1.0 of Zydis includes GNUInstallDirs prior to a language being enabled meaning its CMAKE_INSTALL_LIBDIR will always result in lib rather than its correct lib/lib64 variant. Note that Zydis master branch has fixed this issue.
@rumblehhh
Copy link
Contributor Author

After 1 or 2 more issues the CI tests are now passing.
Your set of CI tests are pretty great by the way. Pretty comprehensive.

In general I've always found cmake's ExternalProject a bit clunky... When using it previously I've transitioned over to using the mechanism outlined here: https://crascit.com/2015/07/25/cmake-gtest/. Once setup it makes using the external projects far more natural i.e. add_subdirectory + actual targets instead of wrapping up IMPORTED targets which bring a few issues.

@kubo kubo merged commit 9f414f7 into kubo:master Nov 29, 2020
@rumblehhh rumblehhh deleted the patch-3 branch November 29, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants