Fix macOS and Windows build failures when threading is disabled (Acad…

…emySoftwareFoundation#1462) * Fix macOS and Windows build failures when threading is disabled Addresses AcademySoftwareFoundation#1455 The logic around which implementation of the IlmThread::Semaphore class to use is now consolidated into the block of code that deciphers the platform and specifies the #includes, rather than spread across multiple files. Signed-off-by: Cary Phillips <[email protected]> * Add macOS/Windows builds with threading disabled Signed-off-by: Cary Phillips <[email protected]> * Fix typo with ILMTHREAD_SEMAPHORE_OTHER, and add missing macOS includes Signed-off-by: Cary Phillips <[email protected]> * Add threads-enabled='ON' so it shows up in the job name Signed-off-by: Cary Phillips <[email protected]> --------- Signed-off-by: Cary Phillips <[email protected]>
cary-ilm · Jul 25, 2023 · d394f92 · d394f92
1 parent 45da397
commit d394f92
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 55 deletions.
diff --git a/.github/workflows/ci_workflow.yml b/.github/workflows/ci_workflow.yml
@@ -314,11 +314,12 @@ jobs:
        C++${{ matrix.cxx-standard }},
        config=${{ matrix.build-type }},
        shared=${{ matrix.build-shared }},
-       cxx=${{ matrix.cxx-standard }}>'
+       cxx=${{ matrix.cxx-standard }},
+       threads=${{ matrix.threads-enabled }}>'
     runs-on: macos-${{ matrix.osver }}
     strategy:
       matrix:
-        build: [1, 2, 3, 4]
+        build: [1, 2, 3, 4, 5]
         include:
           # -------------------------------------------------------------------
           # VFX CY2023 - C++17 - MacOS 11.0 
@@ -327,26 +328,40 @@ jobs:
           - build: 1
             build-type: Release
             build-shared: 'ON'
+            threads-enabled: 'ON'
             compiler-desc: AppleClang11.0
             cxx-standard: 17
             osver: 11.0
             exclude-tests:
             vfx-cy: 23
 
-          # Static, Release
+          # Shared, Release, Threads OFF
           - build: 2
+            build-type: Release
+            build-shared: 'ON'
+            threads-enabled: 'OFF'
+            compiler-desc: AppleClang11.0
+            cxx-standard: 17
+            osver: 11.0
+            exclude-tests:
+            vfx-cy: 23
+
+          # Static, Release
+          - build: 3
             build-type: Release
             build-shared: 'OFF'
+            threads-enabled: 'ON'
             compiler-desc: AppleClang11.0
             cxx-standard: 17
             osver: 11.0
             exclude-tests:
             vfx-cy: 23
 
           # Shared, Debug
-          - build: 3
+          - build: 4
             build-type: Debug
             build-shared: 'ON'
+            threads-enabled: 'ON'
             compiler-desc: AppleClang11.0
             cxx-standard: 17
             osver: 11.0
@@ -357,9 +372,10 @@ jobs:
           # VFX CY2023 - C++17 - MacOS 12
           # -------------------------------------------------------------------
           # Shared, Release
-          - build: 4
+          - build: 5
             build-type: Release
             build-shared: 'ON'
+            threads-enabled: 'ON'
             compiler-desc: AppleClang11.0
             cxx-standard: 17
             osver: 12.0
@@ -425,11 +441,12 @@ jobs:
       <${{ matrix.compiler-desc }},
        config=${{ matrix.build-type }}, 
        shared=${{ matrix.build-shared }}, 
-       cxx=${{ matrix.cxx-standard }}>'
+       cxx=${{ matrix.cxx-standard }},
+       threads=${{ matrix.threads-enabled }}>'
     runs-on: windows-${{ matrix.osver }}
     strategy:
       matrix:
-        build: [1, 2, 3, 4]
+        build: [1, 2, 3, 4, 5]
         include:
           # -------------------------------------------------------------------
           # VFX CY2023 - C++17 - Windows 2022 runner - MSVC 2022 (17.5)
@@ -438,16 +455,29 @@ jobs:
           - build: 1
             build-type: Release
             build-shared: 'ON'
+            threads-enabled: 'ON'
             compiler-desc: msvc17.5
             cxx-standard: 17
             vfx-cy: 2023
             exclude-tests: ''
             osver: 2022
 
-          # Static, Release
+          # Shared, Release, Threads OFF
           - build: 2
+            build-type: Release
+            build-shared: 'ON'
+            threads-enabled: 'OFF'
+            compiler-desc: msvc17.5
+            cxx-standard: 17
+            vfx-cy: 2023
+            exclude-tests: ''
+            osver: 2022
+
+          # Static, Release
+          - build: 3
             build-type: Release
             build-shared: 'OFF'
+            threads-enabled: 'ON'
             compiler-desc: msvc17.5
             cxx-standard: 17
             vfx-cy: 2023
@@ -458,19 +488,21 @@ jobs:
           # VFX CY2022 - C++17 - Windows 2019 runner - MSVC 2019 (16.11) 
           # -------------------------------------------------------------------
           # Shared, Release
-          - build: 3
+          - build: 4
             build-type: Release
             build-shared: 'ON'
+            threads-enabled: 'ON'
             compiler-desc: msvc16.11
             cxx-standard: 17
             vfx-cy: 2022
             exclude-tests: ''
             osver: 2019
 
           # Static, Release
-          - build: 4
+          - build: 5
             build-type: Release
             build-shared: 'OFF'
+            threads-enabled: 'ON'
             compiler-desc: msvc16.11
             cxx-standard: 17
             vfx-cy: 2022

diff --git a/src/lib/IlmThread/IlmThreadSemaphore.cpp b/src/lib/IlmThread/IlmThreadSemaphore.cpp
@@ -10,12 +10,10 @@
 //
 //-----------------------------------------------------------------------------
 
-#include "IlmThreadConfig.h"
-
-#if !(ILMTHREAD_THREADING_ENABLED)
-
 #include "IlmThreadSemaphore.h"
 
+#if ILMTHREAD_SEMAPHORE_DISABLED
+
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_ENTER
 
 
@@ -29,4 +27,4 @@ int Semaphore::value () const {return 0;}
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
-#endif
+#endif // ILMTHREAD_SEMAPHORE_DISABLED
diff --git a/src/lib/IlmThread/IlmThreadSemaphore.h b/src/lib/IlmThread/IlmThreadSemaphore.h
@@ -18,25 +18,41 @@
 #include "IlmThreadConfig.h"
 #include "IlmThreadNamespace.h"
 
-#if defined(__APPLE__)
-#   include <AvailabilityMacros.h>
-#endif
+//
+// Decipher the platform-specific threading support.
+// Set the ILMTHREAD_SEMAPHORE_* defines to indicate the corresponding
+// implementation of the Semaphore class. Only one of these should be
+// defined.
+//
 
 #if ILMTHREAD_THREADING_ENABLED
 #    if ILMTHREAD_HAVE_POSIX_SEMAPHORES
 #        include <semaphore.h>
-#    elif defined(__APPLE__) && MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
-#        include <dispatch/dispatch.h>
+#        define ILMTHREAD_SEMAPHORE_POSIX 1
+#    elif defined(__APPLE__)
+#        include <AvailabilityMacros.h>
+#        if MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
+#            include <dispatch/dispatch.h>
+#            define ILMTHREAD_SEMAPHORE_OSX 1
+#        else
+#            include <condition_variable>
+#            include <mutex>
+#            define ILMTHREAD_SEMAPHORE_OTHER 1
+#        endif
 #    elif (defined(_WIN32) || defined(_WIN64))
 #        ifdef NOMINMAX
 #            undef NOMINMAX
 #        endif
 #        define NOMINMAX
 #        include <windows.h>
+#        define ILMTHREAD_SEMAPHORE_WINDOWS 1
 #    else
 #        include <condition_variable>
 #        include <mutex>
+#        define ILMTHREAD_SEMAPHORE_OTHER 1
 #    endif
+#else
+#    define ILMTHREAD_SEMAPHORE_DISABLED 1
 #endif
 
 ILMTHREAD_INTERNAL_NAMESPACE_HEADER_ENTER
@@ -56,18 +72,20 @@ class ILMTHREAD_EXPORT_TYPE Semaphore
 
   private:
 
-#if ILMTHREAD_HAVE_POSIX_SEMAPHORES
+#if ILMTHREAD_SEMAPHORE_POSIX
 
-	mutable sem_t _semaphore;
+    mutable sem_t _semaphore;
 
-#elif defined(__APPLE__) && MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
+#elif ILMTHREAD_SEMAPHORE_OSX
+
     mutable dispatch_semaphore_t _semaphore;
 
-#elif (defined (_WIN32) || defined (_WIN64))
+#elif ILMTHREAD_SEMAPHORE_WINDOWS
 
-	mutable HANDLE _semaphore;
+    mutable HANDLE _semaphore;
 
-#elif ILMTHREAD_THREADING_ENABLED
+#elif ILMTHREAD_SEMAPHORE_OTHER
+
     //
     // If the platform has threads but no semaphores,
     // then we implement them ourselves using condition variables

diff --git a/src/lib/IlmThread/IlmThreadSemaphoreOSX.cpp b/src/lib/IlmThread/IlmThreadSemaphoreOSX.cpp
@@ -5,18 +5,17 @@
 
 //-----------------------------------------------------------------------------
 //
-//	class Semaphore -- implementation for OSX platform(it don't support unnamed Posix semaphores)
+//	class Semaphore -- implementation for OSX platform (it doesn't
+//	support unnamed Posix semaphores)
+//
 //	std::condition_variable + std::mutex emulation show poor performance
 //
 //-----------------------------------------------------------------------------
 
-#if defined(__APPLE__) && !ILMTHREAD_HAVE_POSIX_SEMAPHORES
-#    include <AvailabilityMacros.h>
+#include "IlmThreadSemaphore.h"
 
-// No libdispatch prior to 10.6, and no support for it on any ppc.
-#if MAC_OS_X_VERSION_MIN_REQUIRED > 1050 && !defined(__ppc__)
+#if ILMTHREAD_SEMAPHORE_OSX
 
-#include "IlmThreadSemaphore.h"
 #include "Iex.h"
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_ENTER
@@ -70,5 +69,4 @@ Semaphore::value () const
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
-#    endif
-#endif
+#endif // ILMTHREAD_SEMAPHORE_OSX
diff --git a/src/lib/IlmThread/IlmThreadSemaphorePosix.cpp b/src/lib/IlmThread/IlmThreadSemaphorePosix.cpp
@@ -10,11 +10,10 @@
 //
 //-----------------------------------------------------------------------------
 
-#include "IlmThreadConfig.h"
+#include "IlmThreadSemaphore.h"
 
-#if ILMTHREAD_HAVE_POSIX_SEMAPHORES
+#if ILMTHREAD_SEMAPHORE_POSIX
 
-#include "IlmThreadSemaphore.h"
 #include "Iex.h"
 #include <assert.h>
 #include <errno.h>
@@ -78,4 +77,4 @@ Semaphore::value () const
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
-#endif
+#endif // ILMTHREAD_SEMAPHORE_POSIX
diff --git a/src/lib/IlmThread/IlmThreadSemaphorePosixCompat.cpp b/src/lib/IlmThread/IlmThreadSemaphorePosixCompat.cpp
@@ -10,19 +10,11 @@
 //
 //-----------------------------------------------------------------------------
 
-#include "IlmThreadConfig.h"
+#include "IlmThreadSemaphore.h"
 
-#if defined(__APPLE__)
-#    include <AvailabilityMacros.h>
-#endif
+#if ILMTHREAD_SEMAPHORE_OTHER
 
 // Use this code as a fallback for macOS versions without libdispatch.
-#if ILMTHREAD_THREADING_ENABLED
-#    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"
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_ENTER
 
@@ -91,5 +83,4 @@ Semaphore::value () const
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
-#endif // posix semaphore compat
-#endif // enable threading
+#endif // ILMTHREAD_SEMAPHORE_OTHER
diff --git a/src/lib/IlmThread/IlmThreadSemaphoreWin32.cpp b/src/lib/IlmThread/IlmThreadSemaphoreWin32.cpp
@@ -9,15 +9,15 @@
 //
 //-----------------------------------------------------------------------------
 
-#include "IlmThreadConfig.h"
+#include "IlmThreadSemaphore.h"
 
-#if (defined(_WIN32) || defined(_WIN64)) && !ILMTHREAD_HAVE_POSIX_SEMAPHORES
+#if ILMTHREAD_SEMAPHORE_WINDOWS
 
-#include "IlmThreadSemaphore.h"
 #include "Iex.h"
-#include <string>
 #include <assert.h>
 #include <iostream>
+#include <string>
+
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_ENTER
 
@@ -121,4 +121,5 @@ Semaphore::value() const
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_EXIT
 
-#endif // _WIN32
+#endif // ILMTHREAD_SEMAPHORE_WINDOWS
+