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

Fallback needed for Apple systems without libdispatch #1405

Closed
barracuda156 opened this issue May 12, 2023 · 8 comments
Closed

Fallback needed for Apple systems without libdispatch #1405

barracuda156 opened this issue May 12, 2023 · 8 comments

Comments

@barracuda156
Copy link
Contributor

There is no libdispatch prior to 10.6; also it is not supported on ppc (i.e. on 10.6 for ppc as well).
A fallback is required.

For the record, using posix compat fixes the build. I am not sure though if is works correctly.

Here is the provisional patch (only defines changed, not the code):

--- src/lib/IlmThread/IlmThreadSemaphore.h.orig	2023-03-28 23:25:15.000000000 +0800
+++ src/lib/IlmThread/IlmThreadSemaphore.h	2023-05-12 06:18:37.000000000 +0800
@@ -18,10 +18,14 @@
 #include "IlmThreadConfig.h"
 #include "IlmThreadNamespace.h"
 
+#if defined(__APPLE__)
+#   include <AvailabilityMacros.h>
+#endif
+
 #if ILMTHREAD_THREADING_ENABLED
 #   if ILMTHREAD_HAVE_POSIX_SEMAPHORES
 #      include <semaphore.h>
-#   elif defined(__APPLE__)
+#   elif defined(__APPLE__) && __MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
 #      include <dispatch/dispatch.h>
 #   elif (defined (_WIN32) || defined (_WIN64))
 #      ifdef NOMINMAX
@@ -56,7 +60,7 @@
 
 	mutable sem_t _semaphore;
 
-#elif defined(__APPLE__)
+#elif defined(__APPLE__) && __MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
 	mutable dispatch_semaphore_t _semaphore;
 
 #elif (defined (_WIN32) || defined (_WIN64))

--- src/lib/IlmThread/IlmThreadSemaphorePosixCompat.cpp.orig	2023-03-28 23:25:15.000000000 +0800
+++ src/lib/IlmThread/IlmThreadSemaphorePosixCompat.cpp	2023-05-12 07:25:25.000000000 +0800
@@ -12,8 +12,13 @@
 
 #include "IlmThreadConfig.h"
 
+#if defined(__APPLE__)
+#include <AvailabilityMacros.h>
+#endif
+
 #if ILMTHREAD_THREADING_ENABLED
-#if ( !(ILMTHREAD_HAVE_POSIX_SEMAPHORES) && !defined (__APPLE__) && !defined (_WIN32) && !defined (_WIN64) )
+#if (!(ILMTHREAD_HAVE_POSIX_SEMAPHORES) && !defined (_WIN32) && !defined (_WIN64)) && \
+    (!defined (__APPLE__) || (defined(__APPLE__) && (__MAC_OS_X_VERSION_MIN_REQUIRED < 1060 || defined(__ppc__))))
 
 #include "IlmThreadSemaphore.h"
 
--- src/lib/IlmThread/IlmThreadSemaphoreOSX.cpp.orig	2023-03-28 23:25:15.000000000 +0800
+++ src/lib/IlmThread/IlmThreadSemaphoreOSX.cpp	2023-05-12 07:20:32.000000000 +0800
@@ -11,6 +11,9 @@
 //-----------------------------------------------------------------------------
 
 #if defined(__APPLE__) && !ILMTHREAD_HAVE_POSIX_SEMAPHORES
+#include <AvailabilityMacros.h>
+
+#if __MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
 
 #include "IlmThreadSemaphore.h"
 #include "Iex.h"
@@ -67,3 +70,4 @@
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
 #endif
+#endif
@meshula
Copy link
Contributor

meshula commented May 12, 2023

This seems reasonable. If you submit it as a PR, the CI will tell us if modern systems continue to build, although it won't give us any information about < 10.6 or ppc.

@barracuda156
Copy link
Contributor Author

This seems reasonable. If you submit it as a PR, the CI will tell us if modern systems continue to build, although it won't give us any information about < 10.6 or ppc.

@meshula I am pretty sure this should not affect newer systems, I opened an issue instead of PR directly because I am not sure this is the best solution. If no better suggestions are there, this fallback is certainly better than a broken build.
I can open a PR then.

@meshula
Copy link
Contributor

meshula commented May 12, 2023

Yes, please submit a PR. There might be a better solution, but I don't have any ideas about what that better solution might be.

@barracuda156
Copy link
Contributor Author

barracuda156 commented May 12, 2023

@meshula A potential solution may be using posix semaphores via configure.args-append -DILMTHREAD_HAVE_POSIX_SEMAPHORES=ON, but I do not know if that is supported (and if it will actually work as intended even if it builds). Comments in the code suggest it is not supported on macOS. At the same time, maybe just no one tried.

UPD. Okay, that failed badly. Let us use posix compat.

barracuda156 added a commit to barracuda156/openexr that referenced this issue May 14, 2023
See: AcademySoftwareFoundation#1405
Signed-off-by: Sergey Fedorov <vital.had@gmail.com>
kdt3rd pushed a commit that referenced this issue May 16, 2023
See: #1405

Signed-off-by: Sergey Fedorov <vital.had@gmail.com>
cary-ilm pushed a commit to cary-ilm/openexr that referenced this issue May 18, 2023
kdt3rd pushed a commit that referenced this issue May 19, 2023
See: #1405

Signed-off-by: Sergey Fedorov <vital.had@gmail.com>
@cary-ilm
Copy link
Member

I believe this issue was resolved in v3.1.8, feel free to reopen if something is still amiss.

@barracuda156
Copy link
Contributor Author

I believe this issue was resolved in v3.1.8, feel free to reopen if something is still amiss.

Yes, we should be good here.

Are versions 2.x still maintained btw? Backporting the fix will be nice, if so. (Otherwise we can do it locally. Relevant since there are some ports depending on an older openexr.)

@cary-ilm
Copy link
Member

We don't actively support 2.x releases any more, but if it cleans things up on your end I can make a release, let me know.

@barracuda156
Copy link
Contributor Author

@cary-ilm It would be helpful in fact to backport this, since old version is still in use and looks like will be for quite a while: #1596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants