[concurrency] Provide missing header file. #37004

3405691582 · 2021-04-22T00:37:51Z

HAVE_PTHREAD_H is supplied in this config file.

The __has_include pattern is discouraged, but there is no other way of
conditioning on pthread_np.h, which is required for pthread_main_np
on OpenBSD. Therefore, use it, but call it out in a comment.

I don't know where HAVE_PTHREAD_H comes from on other platforms, but this
seems like a flaky strategy. While we are here, since we are relying on
pthread_main_np, include pthread_np.h if it is present, since it is
required on some platforms.

(This is a buildfix for OpenBSD.)

3405691582 · 2021-04-26T22:27:46Z

cc @DougGregor

3405691582 · 2021-05-05T13:13:05Z

ping.

3405691582 · 2021-05-12T18:11:10Z

ping. It would be nice if we could resolve this, since it is breaking out-of-the-box builds on this platform.

tbkka · 2021-05-12T18:24:48Z

__has_include is not the best way to determine whether or not a header should be included, since it does not provide a way to suppress broken headers. (On some platforms, it is impossible to compile programs that include certain combinations of system headers. It is easy to check for this at configuration time, but __has_include can't handle this.)

stdlib/public/Concurrency/Actor.cpp

3405691582 · 2021-05-12T20:36:28Z

As mentioned, the trouble is that HAVE_PTHREAD_H isn't defined on other platforms, so the alternative is to define those symbols statically from the build system on the platform. Here, the approach was taken to just add some cflags in the right library rather than trying to push these defines across the entire build, but if doing the latter is the preferred approach let me know and I'll make the necessary changes.

stdlib/public/Concurrency/Actor.cpp

stdlib/public/Concurrency/CMakeLists.txt

stdlib/public/Concurrency/Actor.cpp

HAVE_PTHREAD_H is supplied in this config file. The `__has_include` pattern is discouraged, but there is no other way of conditioning on `pthread_np.h`, which is required for `pthread_main_np` on OpenBSD. Therefore, use it, but call it out in a comment.

tbkka · 2021-05-12T21:27:51Z

Thank you for making those changes.
Does this work for you on OpenBSD? If not, let me know and I'll help you work out something better.

3405691582 · 2021-05-12T21:29:12Z

Sorry for the confusion; thank you for finding the symbol. I agree that just supplying the missing include is the right fix. I've called out the __has_include usage in a comment as you suggest.

Does this work for you on OpenBSD? If not, let me know and I'll help you work out something better.

Just rerunning the build now, though I don't anticipate breakage.

tbkka · 2021-05-12T21:30:53Z

Nicely worded comment! Someday, when I have a chance to work on this, I'll be able to search for HAVE_PTHREAD_NP_H and see where that needs to go.

tbkka

Assuming the tests pass (I'm sure they will), this looks good. Thank you!

3405691582 · 2021-05-12T22:42:29Z

FYI: builds clean/this change works on this platform now.

tbkka · 2021-05-12T22:56:20Z

@swift-ci Please test

3405691582 · 2021-05-13T13:09:35Z

Windows failure appears unrelated.

tbkka · 2021-05-13T16:13:53Z

Merged. Thank you for patiently working through this!

HAVE_PTHREAD_H is supplied in this config file. The `__has_include` pattern is discouraged, but there is no other way of conditioning on `pthread_np.h`, which is required for `pthread_main_np` on OpenBSD. Therefore, use it, but call it out in a comment. (cherry picked from commit eb945c8)

gottesmm requested a review from ktoso May 12, 2021 18:14