Skip to content

CMake fixes#624

Merged
Ninja3047 merged 9 commits intomasterfrom
will/cmake-fixes
Sep 1, 2022
Merged

CMake fixes#624
Ninja3047 merged 9 commits intomasterfrom
will/cmake-fixes

Conversation

@Ninja3047
Copy link
Collaborator

  • remove thirdparty_ interfaces
  • undo llvm jank
  • update git_watcher.cmake, fix quoting
  • update FindZ3.cmake, remove testz3.cpp (now embedded in the cmake)

it compiles but may have unintentially broke things

)

# Windows SDK
add_library(thirdparty_win32 INTERFACE)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't appear to be used so i just deleted it?

)

set(THIRDPARTY_LIBRARY_LIST thirdparty_llvm thirdparty_xed thirdparty_glog thirdparty_gflags thirdparty_sleigh)
target_link_libraries(remill_settings INTERFACE
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should deps go into _settings as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how precise you want to be with the linking of the dependencies to the various libraries/executables.

Ideally, you only link to the dependencies you need, but adding them here means you only need to change one spot 🤷

# Exported Targets
include("${CMAKE_CURRENT_LIST_DIR}/remillTargets.cmake")

if(TARGET thirdparty_llvm)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure what this was doing so i just deleted it and it didn't appear to break anything

@Ninja3047 Ninja3047 requested a review from ekilmer August 30, 2022 19:08
Copy link
Contributor

@ekilmer ekilmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

)

set(THIRDPARTY_LIBRARY_LIST thirdparty_llvm thirdparty_xed thirdparty_glog thirdparty_gflags thirdparty_sleigh)
target_link_libraries(remill_settings INTERFACE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how precise you want to be with the linking of the dependencies to the various libraries/executables.

Ideally, you only link to the dependencies you need, but adding them here means you only need to change one spot 🤷

@Ninja3047 Ninja3047 marked this pull request as ready for review August 31, 2022 01:49
@Ninja3047 Ninja3047 merged commit 0e8459b into master Sep 1, 2022
@Ninja3047 Ninja3047 deleted the will/cmake-fixes branch September 1, 2022 20:06
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