-
Notifications
You must be signed in to change notification settings - Fork 5
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
Completes the WIP CMake config for find_package #116
base: main
Are you sure you want to change the base?
Conversation
This PR is ready for review @hariharan-devarajan @JaeseungYeom. Just note that it shouldn't be merged before #117 |
Was this the PR that had |
05b3811
to
f9eb74b
Compare
@JaeseungYeom @hariharan-devarajan I've updated this PR to undo the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
@hariharan-devarajan @JaeseungYeom tests with A4X-Benchmark were successful. I'll make the last couple of minor edits that @hariharan-devarajan suggested, and then this PR is fully ready for final review and merge. |
@ilumsden double check that links/rpath is set correctly by running |
68e66fb
to
00fe881
Compare
RPATH is set correctly. When building with Spack, I get the following:
The last 3 paths are injected by Spack. The first path is coming from DYAD's CMake. |
@@ -89,11 +95,11 @@ macro(dyad_add_c_flags MY_FLAGS) | |||
endmacro() | |||
|
|||
dyad_add_cxx_flags(CMAKE_CXX_FLAGS | |||
-fPIC -Wall -Wextra -pedantic -Wno-unused-parameter -Wnon-virtual-dtor | |||
-Wall -Wextra -pedantic -Wno-unused-parameter -Wnon-virtual-dtor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JaeseungYeom @hariharan-devarajan I slightly misspoke yesterday.
I did not replace -fPIC
with something else. I just removed it here (and below for C) because we were already using the CMake compiler-independent version of specifying it. So, the manual use of -fPIC
here is redundant.
This is the line where the CMake-specific variable is set: https://github.com/TauferLab/dyad/blob/00fe8812a60ffeab37ce5814b3d6947d45d37031/CMakeLists.txt#L195
-Wno-deprecated-declarations) | ||
|
||
dyad_add_c_flags(CMAKE_C_FLAGS | ||
-fPIC -Wall -Wextra -pedantic -Wno-unused-parameter | ||
-Wall -Wextra -pedantic -Wno-unused-parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for -FPIC.
Remove the commented target properties.
@hariharan-devarajan @JaeseungYeom all the comments from yesterday have been addressed. I just have to rebase. |
… in the top level CMakeLists.txt
…me variable names
…ikely to interfere with users' code
…ad_ctx_init to succeed Adds missing curly brace in top level CMake
… variables to improve compatibility with Spack
23d2ce8
to
23601e4
Compare
The big thing this PR does is complete @JaeseungYeom's WIP
DYADConfig.cmake
and similar stuff for exporting DYAD and making it compatible withfind_package(DYAD CONFIG)
.It also does the following more minor things:
DYAD_INSTALL_INCLUDE_DIR
) more consistent and tweaks the values to be more consistent with expected behavior for installation paths-Werror
with a macro that will directly add the flag to targets withtarget_compile_options
In case we want to revert any of these changes, this PR currently does not delete any of the old code. Instead, the old code is just all commented out.
These changes have all been tested with the unit testing from the in-progress refactor/enhancement of the A4MD benchmark. Thanks to that, I can confirm that these changes allow external projects to properly use DYAD as a dependency with
find_package
.