[vcpkg baseline][openal-soft/libmikmod] Fix OSX build#23683
[vcpkg baseline][openal-soft/libmikmod] Fix OSX build#23683JackBoosY wants to merge 7 commits intomicrosoft:masterfrom
Conversation
JackBoosY
added
the
category:port-bug
| int main() {return 0;}" HAVE_AL_ALEXT_H) | ||
| ENDIF() | ||
| - LIST (APPEND EXTRA_LIBS ${OPENAL_LIBRARY}) | ||
| + LIST (APPEND EXTRA_LIBS OpenAL::OpenAL stdc++) |
There was a problem hiding this comment.
openal-soft is a c++ library but libmikmod is c only, so there should be added a libstc++ library.
LilyWangLL
added
the
info:needs-maintainer-attention
LilyWangLL
added
requires:author-response
and removed
info:needs-maintainer-attention
|
@dg0yt Please review again? |
dg0yt
left a comment
dg0yt
left a comment
There was a problem hiding this comment.
If I'm really asked to review, I have a lot of comments...
- Both ports still use deprecated maintainer functions.
- The pc files need to be verified.
openal.pclacksRequires.private: libmikmod. Andlibmikmod.pcprobably really needs the C++ standard library to be usable in C projects. (Problem known from other ports.)
| - include(FindOpenAL) | ||
| - IF (OPENAL_FOUND) | ||
| + find_package(OpenAL CONFIG REQUIRED) | ||
| + IF (1) |
There was a problem hiding this comment.
My preferred form of minimal patching would be:
find_package(OPENAL NAMES OpenAL REQUIRED)
set(OPENAL_LIBRARY OpenAL::OpenAL)
This would be all which is needed to get the needed OPENAL_<...> variables.
| - LIST (APPEND EXTRA_LIBS ${OPENAL_LIBRARY}) | ||
| + LIST (APPEND EXTRA_LIBS OpenAL::OpenAL) | ||
| + if (APPLE) | ||
| + LIST (APPEND EXTRA_LIBS c++) |
There was a problem hiding this comment.
I still think this shouldn't be done here.
I checked OpenALConfig.cmake: OpenAL::OpenAL lacks the link language property. This should be fixed in openal-soft, and libmikmod should be modified to enable CXX in addition to C.
There was a problem hiding this comment.
No that doesn't help.
Openal contains both c and cxx sources, and cmake will not export the cxx standard library.
There was a problem hiding this comment.
It won't export the library. The consuming CMake project (libmikmod) shall use the C++ linker. To do that, the exporting project (openal-soft) shall export CXX as the linking language, and the consuming project (libmikmod) shall enable CXX language to know the linker binary.
But in the end, you have to hard code the c++ lib into the pc file.
Interesting that Linux didn't complain about missing symbols.
| HEAD_REF master | ||
| PATCHES | ||
| dont-export-symbols-in-static-build.patch | ||
| fix-export-libs.patch |
There was a problem hiding this comment.
IMO this patch is not needed at all. ${EXTRA_LIBS} ${MATH_LIB} are really private. CMake stills exports them for static linkages as needed (verified on x64-linux).
| "port-version": 5, | ||
| "description": "OpenAL Soft is an LGPL-licensed, cross-platform, software implementation of the OpenAL 3D audio API.", | ||
| "homepage": "https://github.com/kcat/openal-soft", | ||
| "license": "GPL-2.0-only", |
| "port-version": 11, | ||
| "description": "Mikmod is a module player and library supporting many formats, including mod, s3m, it, and xm.", | ||
| "homepage": "https://sourceforge.net/projects/mikmod/", | ||
| "license": "GPL-2.0-only", |
There was a problem hiding this comment.
README: LGPL 2.0 or later (plus some extra attribution for parts of the code).
|
This PR is pending since one month. @JackBoosY, do you plan to fix the review comments in anytime soon? |
|
Ah I almost forgot this PR, will continue in these days. |
|
@daschuer You can disable it in your PR now. |
|
It has been already disabled by #23757 so I have just rebased my PR on master. |
|
Closing this PR since it seems that no progress is being made. Please ping me to reopen if work is still being done. |
4 participants
Fix openal-soft export and usage in libmikmod:
Related: #21790