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

Fixed cmake 3.16.0 incompatibility. #9117

Merged
merged 1 commit into from Dec 3, 2019
Merged

Conversation

marcfehling
Copy link
Member

Fixes #9116.

I chose with the most pragmatic way of resolving this issue: By simply imitating what CMake did in previous versions.

There may be other ways of fixing this, e.g. by considering pthreads at the TARGET_LINK_LIBRARIES stage via

LIST(APPEND THREADS_LIBRARIES ${CMAKE_THREAD_LIBS_INIT})

However right now, we just add a linker flag, and I didn't want to overhaul the whole process without being able to assess the consequences.

tamiko
tamiko previously requested changes Dec 1, 2019
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.

see above

@tamiko tamiko dismissed their stale review December 1, 2019 07:03

comment didn't post

#
IF(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.16.0")
LIST(TRANSFORM CMAKE_THREAD_LIBS_INIT PREPEND "-l")
ENDIF()
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind changing this to REGEX_REPLACE a match ^pthreads$ with -lpthreads? That way you don't need the version check and we avoid accidentally producing -l-lpthreads the moment something changes in a future CMake version.

Copy link
Member

Choose a reason for hiding this comment

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

mhm What about you just check for your expectation then?

IF("${CMAKE_THREAD_LIBS_INIT}" MATCHES "^pthread$")
  STRING(PREPEND CMAKE_THREAD_LIBS_INIT "-l")
ENDIF()

@marcfehling
Copy link
Member Author

I understand your concerns and like your suggestion. Adapted changes correspondingly.

@marcfehling
Copy link
Member Author

But on the other hand, FindThreads is able to find other libraries as well which are also affected by this change. Would it make more sense to check whether CMAKE_THREAD_LIBS_INIT starts with -l and prepend it whenever it's missing?

@marcfehling
Copy link
Member Author

Unfortunately, negative lookahead is not available in cmake, so we cannot ask for something like ^(?!-l).*$. Instead, I came up with something different. You'll find my idea in a separate commit.

You may decide which one we should prefer and which one we should drop :)

@tamiko
Copy link
Member

tamiko commented Dec 2, 2019

/rebuild

@masterleinad
Copy link
Member

Somewhat in line with https://gitlab.kitware.com/cmake/cmake/issues/19823, it seems that CMAKE_THREAD_LIBS_INIT is empty for Mac OS X resulting in a lonely -l in the linker flags.

@marcfehling
Copy link
Member Author

Added a check for emptiness for that particular variable. Hopefully Mac OS X will be happy with that.

@tamiko
Copy link
Member

tamiko commented Dec 2, 2019

Sorry for letting you jump through so many hoops. Would you mind testing my suggestion to simply check for ^pthread$?

@marcfehling
Copy link
Member Author

Testers are happy!

@tamiko I gave your proposal a try on my machine as well and it did what expected. However, since CMAKE_THREAD_LIBS_INIT may have values different from pthread, i.e. posix, pthreads and thread at least, I would prefer the current more versatile variant.

@tamiko tamiko merged commit a11b1b5 into dealii:master Dec 3, 2019
@tamiko
Copy link
Member

tamiko commented Dec 3, 2019

As a general remark: We are currently using CMAKE_THREAD_LIBS_INIT as a linker flag which is probably not the intended use here. So if this breaks in the future, we should revisit the whole thread library handling again.

@marcfehling
Copy link
Member Author

As a general remark: We are currently using CMAKE_THREAD_LIBS_INIT as a linker flag which is probably not the intended use here. So if this breaks in the future, we should revisit the whole thread library handling again.

Exactly, we should hand CMAKE_THREAD_LIBS_INIT over during the TARGET_LINK_LIBRARIES stage. But this hotfix does its job for now...

@marcfehling marcfehling deleted the fix-pthread branch December 3, 2019 16:35
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.

CMake 3.16.0: Mandatory test fails
4 participants