-
Notifications
You must be signed in to change notification settings - Fork 9
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
#1581 add option to choose VT library compilation type #1585
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1585 +/- ##
========================================
Coverage 83.00% 83.00%
========================================
Files 785 785
Lines 29747 29747
========================================
Hits 24691 24691
Misses 5056 5056 |
PR tests (clang-10, ubuntu, mpich) Build for a72669f
|
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.
Looks good after you fix the name of the flag as suggested.
4b1dda3
to
bdd584f
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.
Before we go through adding our own variable for this, I want to ask is there a cmake standard or convention that we can/should adopt? Hopefully that would handle the position independent code setting, library target, etc itself.
If there is a standard, we can see which of our included libraries don't follow it, and send them patches |
@PhilMiller As far as I understand |
Converting to draft, as there is a problem with vt-trace and fmt, that I didn't see earlier. |
bdd584f
to
eb711b8
Compare
I agree that we should conform to BUILD_SHARED_LIBS since that also interacts well if VT is included as a subproject (subdirectory). |
eb711b8
to
6144355
Compare
0f6393b
to
ad61ed9
Compare
70023ba
to
ad61ed9
Compare
ad61ed9
to
6788f1d
Compare
${VT_TRACE_LIB} PROPERTIES | ||
POSITION_INDEPENDENT_CODE ${BUILD_SHARED_LIBS} | ||
) | ||
|
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.
We have this for the tracing library; do we need it for others as well? I thought it was there at some point, but maybe it was part of a commit that got rebased out?
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.
It's there for the main VT library... look at the diff
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.
Looks like this may not actually be necessary:
For SHARED and MODULE libraries the POSITION_INDEPENDENT_CODE target property is set to ON automatically.
https://cmake.org/cmake/help/latest/command/add_library.html
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 must have overlooked that.
Fixed -> #1599
Fixes #1581