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

#4722: Remove Shared build workaround #4725

Merged
merged 1 commit into from
Jan 27, 2022
Merged

#4722: Remove Shared build workaround #4725

merged 1 commit into from
Jan 27, 2022

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jan 27, 2022

Fixes #4722

@PhilMiller
Copy link
Contributor Author

In Jenkins, looks like the OpenMPTarget-Clang build failed due to the compiler crashing. It's seemingly in the front-end, so nothing to do with this as far as I can tell.

Same failure appears on https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/7811/pipeline for PR #4724

@PhilMiller
Copy link
Contributor Author

I just tested that this doesn't break shared library builds on Intel 18 either - tests link and pass

Comment on lines -205 to -208
# Workaround for building position independent code.
IF(BUILD_SHARED_LIBS)
SET(CMAKE_POSITION_INDEPENDENT_CODE ON)
ENDIF()
Copy link
Member

Choose a reason for hiding this comment

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

This code was introduced in #2288. We need to make sure removing these lines do not break support for shared libraries.
@junghans @masterleinad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4722 (comment) I tested on gcc and Intel with shared libraries

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works now, I'm very happy without this workaround. It would be interesting what "fixed" it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely some subsequent change in how Kokkos defines its targets in CMake, so that the need for PIC actually gets picked up like it's supposed to. It would take combing through the combined history of Kokkos's build files and releases of CMake itself to pinpoint it, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

The workaround just implements the CMake default for shared libs, so from my side it is ok to drop that.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

It seems that there is some testing on the Trilinos side if I read #4722 correctly.

@PhilMiller
Copy link
Contributor Author

It seems that there is some testing on the Trilinos side if I read #4722 correctly.

I tested Kokkos with the reproducer @ndellingwood posted with the fix I proposed here, with both Intel and GCC.

@dalg24
Copy link
Member

dalg24 commented Jan 27, 2022

Failure (Clang crash in OpenMPTarget build) is clearly unrelated and under investigation elsewhere.
Merging as is.

@dalg24 dalg24 merged commit d896909 into kokkos:develop Jan 27, 2022
@PhilMiller PhilMiller added this to Done, pending release in Developer: Phil Miller Feb 9, 2022
@PhilMiller PhilMiller moved this from Done, pending release to Done in Release 3.6 in Developer: Phil Miller Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Developer: Phil Miller
Done in Release 3.6
4 participants