Skip to content

Conversation

@barracuda156
Copy link
Contributor

@barracuda156 barracuda156 commented May 26, 2023

This PR addresses the build on macOS < 10.7 and PowerPC, which are currently broken for two reasons:

  1. Unconditionally using libdispatch.
  2. Missing define of __STDC_FORMAT_MACROS.

With these fixed, openal-soft builds successfully.

@kencu
Copy link

kencu commented May 26, 2023

see #610 where Evan has been working on this for several years already…

@barracuda156
Copy link
Contributor Author

barracuda156 commented May 26, 2023

see #610 where Evan has been working on this for several years already…

Thank you for referring. The issue got stalled there, no updates since 2021, but I can see what happens with those test cases.

We still need a fix for PRi* types (well, your fix in fact) and a case of 10.6 with a build for ppc (unsupported whether in Rosetta or natively).
But otherwise using semi-posix semaphores (as implemented in 10.5) is a reasonable and a non-painful-to-add fix. They may not work amazingly, but they do work (at least we have examples with other ports using those).

P. S. I actually have a reason why I want this fixed, being used in my new port. Sure enough, in the worst case I can disable it for PPC and 10.5 specifically, but fixing incrementally is IMO better than leaving it broken indefinitely.

@kcat
Copy link
Owner

kcat commented May 26, 2023

For __STDC_FORMAT_MACROS, it may be better to check if that's necessary in CMakeLists.txt, and add it as a CPP_DEFS entry instead of defining it in each source that might need it (which will almost assuredly change, causing it to break in the future and/or leave the definition when it's no longer used). Interestingly, cppreference says these aren't needed by the standard (C99 suggested it for C++, C++ never adopted the suggestion, and C11 removed the suggestion), but some headers check for the macros anyway, and some compilers work around the check by automatically defining the macros. I guess this is a combination of headers that erroneously check for the macros, and a compiler that doesn't work around it.

For libdispatch, if older versions don't have it or can't use it, and there's no other alternative to its buggy POSIX semaphore implementation, that's the best thing to use for the time being.

@barracuda156
Copy link
Contributor Author

For __STDC_FORMAT_MACROS, it may be better to check if that's necessary in CMakeLists.txt, and add it as a CPP_DEFS entry instead of defining it in each source that might need it (which will almost assuredly change, causing it to break in the future and/or leave the definition when it's no longer used). Interestingly, cppreference says these aren't needed by the standard (C99 suggested it for C++, C++ never adopted the suggestion, and C11 removed the suggestion), but some headers check for the macros anyway, and some compilers work around the check by automatically defining the macros. I guess this is a combination of headers that erroneously check for the macros, and a compiler that doesn't work around it.

@kcat Thank you for responding!

C++ documentation mentions that those may be needed on some platforms. I do not know the precise rule (so by implication I am not sure what exactly to check). From experience, GCC needs either those or <cinttypes> header, while some older Clangs require macros, regardless of the header used. In particular, we had a case with ICU, where this was discovered (our fixes merged to the master since then).
Alternative may be to define those in some general header to be included in project files – just ensure it precedes inttypes.h/cinttypes.

For libdispatch, if older versions don't have it or can't use it, and there's no other alternative to its buggy POSIX semaphore implementation, that's the best thing to use for the time being.

It would be nice to have at least those posix semaphores set as a fallback, FWIW. We may not get a stellar performance, but they are known to work.

@kcat
Copy link
Owner

kcat commented May 26, 2023

Alternative may be to define those in some general header to be included in project files – just ensure it precedes inttypes.h/cinttypes.

Adding the macro to CPP_DEFS in CMakeListst.txt adds it to the compile command line, ensuring it's defined for any source that may need it.

@barracuda156
Copy link
Contributor Author

Adding the macro to CPP_DEFS in CMakeListst.txt adds it to the compile command line, ensuring it's defined for any source that may need it.

Ah, okay, so you just suggest to pass it unconditionally.

@kcat
Copy link
Owner

kcat commented May 27, 2023

Commit 118c729 should take care of __STDC_FORMAT_MACROS. If you can update this PR to handle just the dispatch API issue, I can merge it.

@barracuda156
Copy link
Contributor Author

Commit 118c729 should take care of __STDC_FORMAT_MACROS. If you can update this PR to handle just the dispatch API issue, I can merge it.

@kcat Great, will do today. Thank you.

@barracuda156
Copy link
Contributor Author

@kcat Dropped __STDC_FORMAT_MACROS-related commit, rebased to master.

@kcat kcat merged commit cd781b1 into kcat:master May 28, 2023
@barracuda156 barracuda156 deleted the darwin branch May 28, 2023 06:29
@barracuda156
Copy link
Contributor Author

@kcat Unfortunately, CMakeLists fix for does not work as intended.

Configure gives me this:

-- Performing Test HAVE_STDC_FORMAT_MACROS
-- Performing Test HAVE_STDC_FORMAT_MACROS - Success

And CMake cache:

//Test HAVE_STDC_FORMAT_MACROS
HAVE_STDC_FORMAT_MACROS:INTERNAL=1

Then, flag is not passed and build fails with all the same errors.

:info:build [ 42%] Building CXX object CMakeFiles/OpenAL.dir/alc/context.cpp.o
:info:build /opt/local/bin/g++-mp-12 -DALC_API="__attribute__((visibility(\"protected\")))" -DAL_ALEXT_PROTOTYPES -DAL_API="__attribute__((visibility(\"protected\")))" -DAL_BUILD_LIBRARY -DOpenAL_EXPORTS -DRESTRICT=__restrict -I/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/include -I/opt/local/include/dbus-1.0 -I/opt/local/lib/dbus-1.0/include -F//System/Library/Frameworks -I/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/build -I/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1 -I/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/common -pipe -Os -DNDEBUG -isystem/opt/local/include/LegacySupport -I/opt/local/include -D_GLIBCXX_USE_CXX11_ABI=0 -std=gnu++14 -arch ppc -mmacosx-version-min=10.6 -fPIC -fvisibility=hidden -Winline -Wunused -Wall -Wextra -Wshadow -Wconversion -Wcast-align -Wpedantic -Wold-style-cast -Wnon-virtual-dtor -Woverloaded-virtual -Wno-c++20-attribute-extensions -fno-math-errno -pthread -MD -MT CMakeFiles/OpenAL.dir/alc/context.cpp.o -MF CMakeFiles/OpenAL.dir/alc/context.cpp.o.d -o CMakeFiles/OpenAL.dir/alc/context.cpp.o -c /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/alc/context.cpp
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp: In function 'void alSourcePlayAtTimeSOFT(ALuint, ALint64SOFT)':
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3502:74: error: expected ')' before 'PRId64'
:info:build  3502 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                 ~                                        ^~~~~~~
:info:build       |                                                                          )
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:74:1: note: 'PRId64' is defined in header '<cinttypes>'; did you forget to '#include <cinttypes>'?
:info:build    73 | #include "ringbuffer.h"
:info:build   +++ |+#include <cinttypes>
:info:build    74 | #include "threads.h"
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3502:72: warning: spurious trailing '%' in format [-Wformat=]
:info:build  3502 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                                                        ^
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3502:52: warning: too many arguments for format [-Wformat-extra-args]
:info:build  3502 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                                    ^~~~~~~~~~~~~~~~~~~~~~
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp: In function 'void alSourcePlayAtTimevSOFT(ALsizei, const ALuint*, ALint64SOFT)':
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3558:74: error: expected ')' before 'PRId64'
:info:build  3558 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                 ~                                        ^~~~~~~
:info:build       |                                                                          )
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3558:75: note: 'PRId64' is defined in header '<cinttypes>'; did you forget to '#include <cinttypes>'?
:info:build  3558 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                                                           ^~~~~~
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3558:72: warning: spurious trailing '%' in format [-Wformat=]
:info:build  3558 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                                                        ^
:info:build /opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_macports_release_tarballs_ports_audio_openal-soft/openal-soft/work/openal-soft-1.23.1/al/source.cpp:3558:52: warning: too many arguments for format [-Wformat-extra-args]
:info:build  3558 |         return context->setError(AL_INVALID_VALUE, "Invalid time point %" PRId64, start_time);
:info:build       |                                                    ^~~~~~~~~~~~~~~~~~~~~~

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.

3 participants