-
Notifications
You must be signed in to change notification settings - Fork 465
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
Remove openfast_cpp target if BUILD_OPENFAST_CPP_API not selected #1586
Remove openfast_cpp target if BUILD_OPENFAST_CPP_API not selected #1586
Conversation
7a6c341
to
c13f13e
Compare
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.
Were you also going to wrap openfastcpp
in BUILD_OPENFAST_CPP_DRIVER
? If so, the conditional for regression test of_cpp_interface_regression
will also need to be updated.
glue-codes/openfast/CMakeLists.txt
Outdated
RUNTIME DESTINATION bin) | ||
|
||
if(BUILD_OPENFAST_CPP_API AND BUILD_OPENFAST_CPP_DRIVER) |
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.
openfast_cpp_driver
doesn't use the openfastcpplib
, does it need to check for BUILD_OPENFAST_CPP_API
?
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.
I need to make another pass through the logic here. There are a couple other minor things I think should be addressed.
glue-codes/openfast/CMakeLists.txt
Outdated
|
||
install(TARGETS openfast_cpp_driver | ||
RUNTIME DESTINATION bin) | ||
endif(BUILD_OPENFAST_CPP_API AND BUILD_OPENFAST_CPP_DRIVER) |
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.
Could we not repeat the condition at the end of the block? endif()
is sufficient for CMake.
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.
Sorry, old habits ;)
4ee79d8
to
ced67d8
Compare
ced67d8
to
a416303
Compare
Looking back at this, I find it a little strange that |
Ready to merge
Feature or improvement description
If
BUILD_OPENFAST_CPP_API
is false andmake
ormake all
is used to build, theopenfast_cpp
target is built. This can pose issues on some systems if the user is not expecting it and doesn't have locations to all necessary libraries specified.Also noticed that the
NodeClusterType
input for OpenFOAM was inconsistently handled as integers or logicals. Changed it all to integer to maintain consistency with checks used in the code.Impacted areas of the software
Interfacing to SOWFA or other CFD.
CMake default builds
Additional supporting information
@mchurchf ran into compile issue on Eagle.
Test results, if applicable
None.