AcademySoftwareFoundation / openexr

The OpenEXR project provides the specification and reference implementation of the EXR file format, the professional-grade image storage format of the motion picture industry.
http://www.openexr.com/
BSD 3-Clause "New" or "Revised" License
1.63k stars 619 forks source link

Fallback needed for Apple systems without libdispatch #1405

Closed barracuda156 closed 1 year ago

barracuda156 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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.

cary-ilm commented 1 year ago

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

barracuda156 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

@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: https://github.com/AcademySoftwareFoundation/openexr/pull/1596