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

Create dealii::dealii target and add compile options #14971

Merged
merged 1 commit into from Mar 30, 2023

Conversation

masterleinad
Copy link
Member

@masterleinad masterleinad commented Mar 24, 2023

This pull request makes it possible to use

target_link_libraries(${TARGET} dealii::dealii)

including using DEAL_II_CXX_FLAGS.

(tamiko:) In reference to #14947

@sebproell
Copy link
Contributor

@masterleinad I now have DEBUG defined in my release build when linking to dealii::dealii :D

When I look into the installed deal.IITargets.cmake, I see

set_target_properties(dealii::dealii PROPERTIES
  INTERFACE_LINK_LIBRARIES "dealii::dealii_debug;dealii::dealii_release"
)

which doesn't look quite right.

@masterleinad
Copy link
Member Author

masterleinad commented Mar 24, 2023

@sebproell With the latest commit, we now do

# Create imported target dealii::dealii
add_library(dealii::dealii INTERFACE IMPORTED)
  
set_target_properties(dealii::dealii PROPERTIES
  INTERFACE_LINK_LIBRARIES "\$<\$<CONFIG:Debug>:dealii::dealii_debug>;\$<\$<CONFIG:>:dealii::dealii_debug>;\$<\$<CONFIG:Release>:dealii::dealii_release>"
) 

@masterleinad
Copy link
Member Author

/rebuild

@tamiko
Copy link
Member

tamiko commented Mar 25, 2023

@masterleinad I am currently running the FE Rodeo. Would you mind to give me 48h to think about this approach.

In general I think it is neat to introduce a dealii::dealii dummy target but then again we have the problem to hardcode it to Debug and Release.

tamiko

This comment was marked as outdated.

@tamiko
Copy link
Member

tamiko commented Mar 25, 2023

@sebproell For the time being target_link_libraries(TARGET ${DEAL_II_TARGET}) should just work. It uses the inelegant optimized and debug "keywords", though...

Copy link
Member

@tamiko tamiko left a comment

Choose a reason for hiding this comment

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

Looks good, I have some minor technical suggestions.

@@ -127,6 +142,24 @@ foreach(build ${DEAL_II_BUILD_TYPES})
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${DEAL_II_EXECUTABLE_RELDIR}"
)

separate_arguments(_compile_options UNIX_COMMAND
"${DEAL_II_CXX_FLAGS_${build_uppercase}}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"${DEAL_II_CXX_FLAGS_${build_uppercase}}"
"${DEAL_II_CXX_FLAGS_${build}}"

foreach(build ${DEAL_II_BUILD_TYPES})
string(TOLOWER ${build} build_lowercase)
string(TOUPPER ${build} build_uppercase)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string(TOUPPER ${build} build_uppercase)
if("${build}" matches "DEBUG")
set(build_camelcase "Debug")
elseif("${build}" matches "RELEASE")
set(build_camelcase "Release")
endif()

The ${build} variable is already uppercase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... I'm pretty sure I needed that initially but I can confirm that ${build} is uppercase indeed.

source/CMakeLists.txt Outdated Show resolved Hide resolved
source/CMakeLists.txt Outdated Show resolved Hide resolved
@tamiko
Copy link
Member

tamiko commented Mar 30, 2023

@masterleinad Would you mind to squash the reverts?

@masterleinad
Copy link
Member Author

@masterleinad Would you mind to squash the reverts?

I just squashed everything.

@tamiko tamiko merged commit 03e6c36 into dealii:master Mar 30, 2023
11 checks passed
tamiko added a commit to tamiko/dealii that referenced this pull request Mar 31, 2023
We had an unfortunate "in flight" renaming conflict: One pull request
renamed the variable to DEAL_II_TARGET_NAME (PR dealii#14993) while another
one (PR dealii#14971) created the dealii::dealii target. Both on their own
passed the CI, but once both were merged we have an issue.

In reference to dealii#14971
In reference to dealii#14993
tamiko added a commit to tamiko/dealii that referenced this pull request Mar 31, 2023
We had an unfortunate "in flight" renaming conflict: One pull request
renamed the variable to DEAL_II_TARGET_NAME (PR dealii#14993) while another
one (PR dealii#14971) created the dealii::dealii target. Both on their own
passed the CI, but once both were merged we have an issue.

In reference to dealii#14971
In reference to dealii#14993
mschreter pushed a commit to mschreter/dealii that referenced this pull request Apr 2, 2023
We had an unfortunate "in flight" renaming conflict: One pull request
renamed the variable to DEAL_II_TARGET_NAME (PR dealii#14993) while another
one (PR dealii#14971) created the dealii::dealii target. Both on their own
passed the CI, but once both were merged we have an issue.

In reference to dealii#14971
In reference to dealii#14993
singimarson pushed a commit to singimarson/dealii that referenced this pull request Apr 11, 2023
We had an unfortunate "in flight" renaming conflict: One pull request
renamed the variable to DEAL_II_TARGET_NAME (PR dealii#14993) while another
one (PR dealii#14971) created the dealii::dealii target. Both on their own
passed the CI, but once both were merged we have an issue.

In reference to dealii#14971
In reference to dealii#14993
tamiko added a commit to tamiko/dealii that referenced this pull request Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants