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

Unbreak build for macOS < 10.12 #1732

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

barracuda156
Copy link

The PR fixes three issues:

  1. Existing macros used for fallbacks on macOS do exactly the opposite of what they are thought to, because MAC_OS_X_VERSION_10_12 is not defined on, well, macOS < 10.12, so the condition compares a positive number to zero:
    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_12
    In result, everything that should not be used on older macOS is applied, and build is broken.
    Less ugly alternative would be MAC_OS_X_VERSION_MIN_REQUIRED >= 101200, which gonna work correctly, but I know some people wanna avoid magic numbers :)

  2. Existing Altivec implementation breaks down spectacularly on Darwin PowerPC. This is not an area I know in any detail, so I cannot say why it fails. Maybe it is something feasibly if not trivially fixable. But unless it is fixed, rather disable it on Apple.

  3. CMakeLists hardcode libc++ flags, which breaks builds with GCC. I do not know if Clang even needs an explicit flag, but GCC is smart enough to add its primary runtime. Restrict the flag to Clang.
    Also, since the current CMake policy uses AppleClang for Xcode Clangs, MATCHES should be used instead of STREQUAL. (Unless, of course, the idea was to exclude Apple Clangs from Clangs for w/e reason.)

@barracuda156 barracuda156 changed the base branch from master to develop July 26, 2023 11:58
@barracuda156
Copy link
Author

I have finally figured out how to fix running tests, and now everything works:

--->  Testing csound
Executing:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_audio_csound/csound/work/build" && /usr/bin/make test 
Running tests...
/opt/local/bin/ctest --force-new-ctest-process 
Test project /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_audio_csound/csound/work/build
      Start  1: testCsoundTypeSystem
 1/12 Test  #1: testCsoundTypeSystem .............   Passed    0.30 sec
      Start  2: testCsoundMessageBuffer
 2/12 Test  #2: testCsoundMessageBuffer ..........   Passed    0.16 sec
      Start  3: testCsoundOrcSemantics
 3/12 Test  #3: testCsoundOrcSemantics ...........   Passed    0.32 sec
      Start  4: testCsoundOrcCompileTest
 4/12 Test  #4: testCsoundOrcCompileTest .........   Passed    5.09 sec
      Start  5: testChannels
 5/12 Test  #5: testChannels .....................   Passed    0.60 sec
      Start  6: testCsoundDataStructures
 6/12 Test  #6: testCsoundDataStructures .........   Passed    0.06 sec
      Start  7: testIo
 7/12 Test  #7: testIo ...........................   Passed    0.77 sec
      Start  8: testCircularBuffer
 8/12 Test  #8: testCircularBuffer ...............   Passed    0.05 sec
      Start  9: testPerfThread
 9/12 Test  #9: testPerfThread ...................   Passed   13.12 sec
      Start 10: testDebugger
10/12 Test #10: testDebugger .....................   Passed    0.18 sec
      Start 11: testEngine
11/12 Test #11: testEngine .......................   Passed    1.07 sec
      Start 12: testServer
12/12 Test #12: testServer .......................   Passed    3.07 sec

100% tests passed, 0 tests failed out of 12

Total Test time (real) =  24.81 sec

@@ -1058,7 +1058,7 @@ int csoundSpinLockInit(spin_lock_t *spinlock) {

#elif defined(MACOSX) // MacOS native locks

#if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_12
Copy link

@kencu kencu Aug 1, 2023

Choose a reason for hiding this comment

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

more compatible is this:

#if MAC_OS_X_VERSION_MIN_REQUIRED >= 101200

as it works even if MAC_OS_X_VERSION_10_12 is not defined.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I prefer numerical macros too, but for some reason not everyone is okay with them (we could not convince Ruby upstream, for example, and still carry our patch).

@barracuda156
Copy link
Author

Wait a bit, there is a proper fix for Altivec advised by @pkubaj which seems to work fine: #1733 (comment)

@vlazzarini
Copy link
Member

Where are with this? Some conflicts have emerged since the PR was open. Let me know the status.

@stekyne
Copy link
Member

stekyne commented Mar 19, 2024

The C++11 stuff is outdated. There are more modern ways of setting it which we do now so that can be removed. The other stuff is maybe still relevant.

@barracuda156
Copy link
Author

@vlazzarini @stekyne Let me review this today and rebase.

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

Successfully merging this pull request may close these issues.

None yet

4 participants