Use target link library instead of macro #394
Conversation
jh66637
changed the title
jh66637
force-pushed
the
target_link_lib
branch
from
a817a4d
to
a268abe
Compare
|
@jh66637 Could you explain how/whether this is related to dealii/dealii#14962? |
|
This is somewhat independent. With the fix in dealii/dealii#14962 everything should work again. While we need the CMake macro for the tests, we do not necessarily need it for linking the executables. Just using |
| @@ -37,6 +37,8 @@ FFTW=$WORKING_DIRECTORY/fftw/install | |||
| LIKWID=$WORKING_DIRECTORY/likwid/install | |||
|
|
|||
| cmake \ | |||
| -D CMAKE_BUILD_TYPE="DebugRelease" \ | |||
There was a problem hiding this comment.
What is the meaning of DebugRelease here? In the CMakeLists.txt file we have ADD_CUSTOM_TARGET where we can switch between debug and release?
There was a problem hiding this comment.
I forgot about the custom target.
| @@ -37,6 +37,8 @@ FFTW=$WORKING_DIRECTORY/fftw/install | |||
| LIKWID=$WORKING_DIRECTORY/likwid/install | |||
|
|
|||
| cmake \ | |||
| -D CMAKE_BUILD_TYPE="DebugRelease" \ | |||
| -D CMAKE_C_FLAGS="-march=native -Wno-array-bounds" \ | |||
There was a problem hiding this comment.
here you write C, while you write CXX below for supermuc-ng/config_exadg.sh?
There was a problem hiding this comment.
This relates to the deal.II setup script. I suppose CXX should work as well. These lines of code become relevant since the deal.II macro does not take care about it anymore.
| @@ -25,7 +25,7 @@ | |||
| # | |||
| ######################################################################### | |||
|
|
|||
| CMAKE_MINIMUM_REQUIRED(VERSION 3.1.0) | |||
| CMAKE_MINIMUM_REQUIRED(VERSION 3.16) | |||
There was a problem hiding this comment.
Has anyone concerns regarding version 3.16? Is this decision final on the deal.II side?
| @@ -28,8 +28,6 @@ | |||
| MACRO(EXADG_PICKUP_EXE FILE_NAME TARGET_NAME EXE_NAME) | |||
There was a problem hiding this comment.
It looks like EXE_NAME is no longer used?
There was a problem hiding this comment.
Yes, there is no need anymore. I have to remove it from the interface.
|
Thanks for improving/simplying cmake content in ExaDG. For me it looks like the cmake flags and the changes in CMakeLists.txt (and the macro) are somewhat independent changes that would optimally be realized as separate PRs. |
|
This was supposed to be a quick fix for the failing pipeline. Since dealii/dealii#14962 got merged, I think this is not urgent anymore and thus, I will continue here when I find the time (probably not soon). |
|
@nfehn I will split this PR into separate PRs. As mentioned, this is not urgent anymore since the failing pipeline was fixed by deal.II. I still think this is the way to go. I am converting this PR to a draft in the meantime such that I have a reminder to do this. I hope I will find the time soon. |
|
@jh66637 is this PR still work planned for the future, or is this topic already outdated? |
|
I still think we should switch to the new interface. But since currently everything is working using the CMake macros from deal.II, this is not high priority for me. |
|
Would it now make sense to continue with this PR? |
|
Probably yes, since the changes in the build system are finalized in |
This introduces the new way how to link against deal.II.