Skip to content

Fix USE_SYSTEM_OPENAL to use system headers #17163

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

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

Conversation

pastalian
Copy link
Contributor

@pastalian pastalian commented May 4, 2025

Even when USE_SYSTEM_OPENAL was enabled, the code directly included the bundled OpenAL headers, bypassing the system headers.

Since #include <AL/al.h> is not portable, use #include "al.h" instead.
https://cmake.org/cmake/help/latest/module/FindOpenAL.html

@pastalian
Copy link
Contributor Author

Does anyone know why macOS can't find the OpenAL headers?
CMake seems to find OpenAL correctly and is passing the header path:
-- Found OpenAL: /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/System/Library/Frameworks/OpenAL.framework
... -isystem /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/System/Library/Frameworks/OpenAL.framework/Headers ... /Users/runner/work/1/s/rpcs3/Emu/Cell/Modules/cellMic.cpp ...

but it's still failing:
/Users/runner/work/1/s/rpcs3/Emu/Cell/Modules/cellMic.cpp:14:10: fatal error: 'alext.h' file not found

@hcorion
Copy link
Member

hcorion commented May 4, 2025

Try setting -DUSE_SYSTEM_OPENAL=OFF in the build-mac.sh and build-mac-arm64.sh scripts.

@Megamouse Megamouse added the Build and CI Anything related to the build process and continuous integration label May 5, 2025
@pastalian
Copy link
Contributor Author

Thank you, that fixed it.
But this means USE_SYSTEM_OPENAL=ON is still broken on macOS. Is that OK with you?

@hcorion
Copy link
Member

hcorion commented May 6, 2025

Yeah since it's a default we should figure out why it's not including the headers on Mac.
I think it might be straightforward though. Try adding something like
target_include_directories(3rdparty_openal INTERFACE ${OPENAL_INCLUDE_DIR})
to the OpenAL CMakeLists.txt, looks like we're just not actually referencing the include dirs.

@hcorion
Copy link
Member

hcorion commented May 6, 2025

And you should revert the build.sh hacks we added, since the CI should be testing defaults.

@pastalian
Copy link
Contributor Author

Apparently, macOS does not provide alext.h.
https://developer.apple.com/library/archive/documentation/MusicAudio/Conceptual/CoreAudioOverview/CoreAudioFrameworks/CoreAudioFrameworks.html

The OpenAL framework provides an implementation of the the OpenAL specification. This framework includes these two header files:
al.h
alc.h

Maybe USE_SYSTEM_OPENAL should be disabled on macOS?

@elad335
Copy link
Contributor

elad335 commented May 6, 2025

Is there a benefit to USE_SYSTEM_OPENAL? it can be dropped altogether otherwise.

Apparently, macOS does not provide alext.h. https://developer.apple.com/library/archive/documentation/MusicAudio/Conceptual/CoreAudioOverview/CoreAudioFrameworks/CoreAudioFrameworks.html

The OpenAL framework provides an implementation of the the OpenAL specification. This framework includes these two header files:
al.h
alc.h

Maybe USE_SYSTEM_OPENAL should be disabled on macOS?

@pastalian
Copy link
Contributor Author

Is there a benefit to USE_SYSTEM_OPENAL? it can be dropped altogether otherwise.

In general, on Linux, it's preferred to have the option to use system libraries instead of bundled ones.

This might not be as important on Windows or macOS, though.

@hcorion
Copy link
Member

hcorion commented May 6, 2025

Yeah let's change the default of USE_SYSTEM_OPENAL on Windows and Mac to use false and keep it as true on Linux

pastalian added 2 commits May 7, 2025 01:02
Even when USE_SYSTEM_OPENAL was enabled, the code directly included the
bundled OpenAL headers, bypassing the system headers.

Since `#include <AL/al.h>` is not portable, use `#include "al.h"`
instead.
https://cmake.org/cmake/help/latest/module/FindOpenAL.html
On Linux, using system libraries is generally preferred, but this is
less relevant on macOS and Windows. In particular, macOS does not
provide "alext.h" in its OpenAL framework, which causes the build to
fail.
@elad335
Copy link
Contributor

elad335 commented May 6, 2025

68ac05f

If it fails on MacOS, force it being disabled instead of being an option.

@hcorion
Copy link
Member

hcorion commented May 6, 2025

We don't want to disable, the user could install openal via brew in which case it would build fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build and CI Anything related to the build process and continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants