Extend guard for sound support on OS X #13115

sevan · 2023-09-17T22:36:16Z

Fixes build on legacy versions where required coreaudio functionality may not be available.
NSSoundDelegate apparently was introduced in Snow Leopard yet the build breaks on it. Guarding off enabling sound support to El Capitan as that's the next version I had access to for testing (it may work on earlier versions, between 10.6 & 10.11).
Vim builds on OS X Tiger 10.4 and newer with this change.

Fixes build on legacy versions where required coreaudio functionality may not be available. NSSoundDelegate apparently was introduced in Snow Leopard yet the build breaks on it. Guarding off enabling sound support to El Capitan as that's the next version I had access to for testing (it may work on earlier versions) https://developer.apple.com/documentation/appkit/nssounddelegate Vim builds on OS X Tiger 10.4 and newer with this change.

chrisbra · 2023-09-18T18:03:58Z

Thanks. I was slightly worried from the diff that this have some unexpected results on non Mac OS systems, but since the whole block is guarded by #if defined(MACOS_X_DARWIN) I guess it is safe.

ychin · 2023-09-25T19:57:59Z

@sevan How was build breaking for Snow Leopard? Do you have the actual build logs? Feels like we should at least have a cursory look of what's breaking to see what caused build to break. The Apple documentation is pretty accurate about what version supports what so it's usually possible to find from the docs the minimum version we need to support, rather than whatever we have on hand to test.

Also, I have seen this in other macOS-related PRs here before, but I don't think MAC_OS_X_VERSION_MIN_REQUIRED is the correct macro to use here. That macro defines the deployment target of your build, not what SDK you are building against (MAC_OS_X_VERSION_MAX_ALLOWED). It does depend on what exactly is breaking though, so it is useful to have that logs available to see.

I'm mentioning this because MacVim currently supports 10.9, so this change could break sound support for our "legacy" build (which supports 10.9 - 10.12).

Since I maintain MacVim, is it possible for me to be pinged (if possible) on macOS compatibility issues? Thanks. I think proper macOS version compatibility can sometimes be tricky (a combination of MAC_OS_X_VERSION_MIN_REQUIRED, MAC_OS_X_VERSION_MAX_ALLOWED, and runtime checks using @available and NSAppKitVersionNumber), because you can build against a new SDK, but built to be compatible on an old OS version (which is what MacVim does).

sevan · 2023-09-26T21:29:06Z

@ychin it failed on compiling os_macosx.m, I was doing native builds per OS/arch, rather that using the SDKs to target different OS releases using a newer OS/toolchain. It's fairly easy to reproduce, undo the change proposed here and ./configure && make on 10.6 (since that's when NSSoundDelegate was added, and just not available on older releases).
I'm actually interested in 10.4-10.6 support but like I said in the 1st comment here, I set enabling sound support on 10.11 since that's what I had access to, if you reckon it'll compile on 10.9 with the toolchain/sdk for itself, then it could be adjusted.
I'll re-attempt the build on 10.6 and get you the exact error message but in the meantime here's a ticket on macports which reckons sound support is broken 10.10 and prior (when building natively on said versions)

ychin · 2023-09-26T23:54:40Z

From the ticket, it looks like it's an issue with compiler / toolchain not understanding the newer syntax for generic, rather than SDK support for the sound delegate.

How exactly are you building on these older platforms? Are you building on physical hardware or VMs (and what kind of compilation steps/flags are you using)? Is MacPorts also building on VMs?

I can look into fixing this more properly. I do appreciate if in future macOS compatibility issues I can be pinged on this. While it's easy to fix the issue for your specific problem, changes like this can affect other builds of Vim on macOS as well (e.g. MacVim).

sevan · 2023-09-27T00:07:10Z

I'm building it on physical hardware.
Download vim archive, undo my change, run configure, make, on 10.6.8 with GCC 4.2.1 from xcode it fails with

gcc -std=gnu99 -c -I. -Iproto -DHAVE_CONFIG_H   -DMACOS_X -DMACOS_X_DARWIN  -g -O2 -D_REENTRANT -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1        -o objects/netbeans.o netbeans.c
In file included from vim.h:2341,
                 from netbeans.c:26:
proto.h:126: warning: ‘cold’ attribute directive ignored
proto.h:133: warning: ‘cold’ attribute directive ignored
proto.h:136: warning: ‘cold’ attribute directive ignored
os_macosx.m:389: error: expected ‘>’ before ‘*’ token
os_macosx.m:389: error: cannot find protocol declaration for ‘NSNumber’
os_macosx.m: In function ‘-[SoundDelegate init:callback:]’:
os_macosx.m:408: error: ‘_sound_id’ undeclared (first use in this function)
os_macosx.m:408: error: (Each undeclared identifier is reported only once
os_macosx.m:408: error: for each function it appears in.)
os_macosx.m:409: error: ‘_callback’ undeclared (first use in this function)
os_macosx.m: In function ‘-[SoundDelegate sound:didFinishPlaying:]’:
os_macosx.m:416: error: ‘sounds_list’ undeclared (first use in this function)
os_macosx.m:418: error: ‘_callback’ undeclared (first use in this function)
os_macosx.m:420: error: ‘_sound_id’ undeclared (first use in this function)
os_macosx.m: At top level:
os_macosx.m:430: warning: property ‘sound_id’ requires method '-sound_id' to be defined - use @synthesize, @dynamic or provide a method implementation
os_macosx.m:430: warning: property ‘callback’ requires method '-callback' to be defined - use @synthesize, @dynamic or provide a method implementation
os_macosx.m: In function ‘process_cfrunloop’:
os_macosx.m:435: error: ‘sounds_list’ undeclared (first use in this function)
os_macosx.m:441: error: stray ‘@’ in program
os_macosx.m:441: error: ‘autoreleasepool’ undeclared (first use in this function)
os_macosx.m:442: error: expected ‘;’ before ‘{’ token
os_macosx.m: In function ‘sound_mch_play’:
os_macosx.m:453: error: stray ‘@’ in program
os_macosx.m:453: error: ‘autoreleasepool’ undeclared (first use in this function)
os_macosx.m:454: error: expected ‘;’ before ‘{’ token
os_macosx.m: In function ‘sound_mch_stop’:
os_macosx.m:483: error: stray ‘@’ in program
os_macosx.m:483: error: ‘autoreleasepool’ undeclared (first use in this function)
os_macosx.m:484: error: expected ‘;’ before ‘{’ token
os_macosx.m: In function ‘sound_mch_clear’:
os_macosx.m:498: error: ‘sounds_list’ undeclared (first use in this function)
os_macosx.m:500: error: stray ‘@’ in program
os_macosx.m:500: error: ‘autoreleasepool’ undeclared (first use in this function)
os_macosx.m:501: error: expected ‘;’ before ‘{’ token
make[1]: *** [objects/os_macosx.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [first] Error 2

Not using any additional flags, or external components, just what's provided with OS X & XCode. The snippet above was taken from attempting vim-9.0.1907 just now.

chrisbra · 2023-09-27T05:50:57Z

on 10.6.8 with GCC 4.2.1 from xcode

Oh, that is old. GCC 4.2.1 is from 2007, can you upgrade your compiler?

sevan · 2023-09-27T09:14:06Z

Seems heavy handed when the huge feature set builds fine without sound using that compiler (sound support should be optional if you want to go down that route).
vim builds with huge feature set on OS X with GCC 4.0.1 onwards (without sound) by the way.

chrisbra · 2023-09-27T09:29:36Z

I see then. I leave it to @ychin to decide if there is something more actionable here for us :)

ychin · 2023-09-27T20:12:18Z

Using newer/older toolchains / compilers on older macOS is quite annoying to do, as Apple likes to tie Xcode distribution with macOS releases. E.g. I was trying to grab clang-17 on the Mac for testing the undefined behaviors that I fixed recently and gave up and just used a Linux VM instead…

I was thinking of just changing the check to

defined(__clang_major__) && __clang_major__ >= 7 && \
defined(MAC_OS_X_VERSION_MIN_REQUIRED) && MAC_OS_X_VERSION_MIN_REQUIRED >= 1060

The new syntax for supporting types in generics a.k.a NSDictionary<type1, type2> was added in Xcode 7 / Clang 7 (which I think is tied with the macOS 10.11 release) which is what the first line is checking for (look for 6294649 under https://developer.apple.com/library/archive/releasenotes/DeveloperTools/RN-Xcode/Chapters/Introduction.html#//apple_ref/doc/uid/TP40001051-CH1-SW217), and the second line makes sure we only use this feature when deploying to macOS 10.6+ (otherwise we have to do runtime checks).

We could refactor the code to not use the new syntax (or use ifdef to write a different versions) by just declaring untyped dictionaries, but this seems old enough that I think it's fine to just turn off the feature for older platforms.

On the MacVim front, we mostly only support 10.9 - 10.10 currently by building binaries on newer macOS / toolchain versions and setting deployment target to 10.9, so it would still work.

I was also contemplating just turning it into a configure flag like the other platforms but it seemed unnecessary if we just fix the ifdef conditions to be more accurate.

I haven't submitted a PR yet because I wanted to test it on my VMs first.

The ifdef check used in upstream Vim is too tight. Disable it for now. See vim/vim#13115

ychin · 2023-10-02T10:08:32Z

Ok, I just filed #13251 to fix this.

sevan · 2023-10-03T00:06:40Z

Worked fine on older versions. Thanks :)

chrisbra closed this in 063c562 Sep 18, 2023

sevan deleted the legacy-osx-buildfix branch September 18, 2023 20:21

ychin added a commit to ychin/macvim that referenced this pull request Sep 29, 2023

Enable +sound for legacy builds (10.9-10.12) until fixed upstream

c83901a

The ifdef check used in upstream Vim is too tight. Disable it for now. See vim/vim#13115

ychin mentioned this pull request Oct 2, 2023

Fix macOS FEAT_SOUND guards to be less restrictive #13251

Closed

ychin mentioned this pull request Oct 12, 2023

Modernize Objective-C #12337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Extend guard for sound support on OS X #13115

Extend guard for sound support on OS X #13115

Uh oh!

sevan commented Sep 17, 2023

Uh oh!

chrisbra commented Sep 18, 2023

Uh oh!

ychin commented Sep 25, 2023 •

edited

Loading

Uh oh!

sevan commented Sep 26, 2023

Uh oh!

ychin commented Sep 26, 2023

Uh oh!

sevan commented Sep 27, 2023

Uh oh!

chrisbra commented Sep 27, 2023

Uh oh!

sevan commented Sep 27, 2023

Uh oh!

chrisbra commented Sep 27, 2023

Uh oh!

ychin commented Sep 27, 2023 •

edited

Loading

Uh oh!

ychin commented Oct 2, 2023

Uh oh!

sevan commented Oct 3, 2023

Uh oh!

Uh oh!

Uh oh!

Extend guard for sound support on OS X #13115

Extend guard for sound support on OS X #13115

Uh oh!

Conversation

sevan commented Sep 17, 2023

Uh oh!

chrisbra commented Sep 18, 2023

Uh oh!

ychin commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sevan commented Sep 26, 2023

Uh oh!

ychin commented Sep 26, 2023

Uh oh!

sevan commented Sep 27, 2023

Uh oh!

chrisbra commented Sep 27, 2023

Uh oh!

sevan commented Sep 27, 2023

Uh oh!

chrisbra commented Sep 27, 2023

Uh oh!

ychin commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ychin commented Oct 2, 2023

Uh oh!

sevan commented Oct 3, 2023

Uh oh!

Uh oh!

ychin commented Sep 25, 2023 •

edited

Loading

ychin commented Sep 27, 2023 •

edited

Loading