-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[fluidsynth] Update to 2.3.2 #30625
[fluidsynth] Update to 2.3.2 #30625
Conversation
The usage requirements patch is no longer required as a similar patch has been merged upstream.
- List out the options per-platform. - Set the proper options for Android.
The linux CI errors seem to be related to ALSA:
It should be noted that upstream has switched from I can't seem to be able to reproduce the failure locally on the |
I cloned your branch code locally, and then compiled it on Ubuntu and had the same problem as CI.
|
Thank you for the confirmation. It sorts of look like an ALSA issue, where ALSA depends on For the time being, what do you think about adding the following patch: diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 114fa163..2ea55ddc 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -365,7 +365,7 @@ if ( PULSE_SUPPORT )
endif()
if ( TARGET ALSA::ALSA AND ALSA_SUPPORT )
- target_link_libraries ( libfluidsynth-OBJ PUBLIC ALSA::ALSA )
+ target_link_libraries ( libfluidsynth-OBJ PUBLIC ALSA::ALSA ${CMAKE_DL_LIBS} )
endif()
if ( TARGET PortAudio::PortAudio AND PORTAUDIO_SUPPORT ) While I still cannot reproduce the issue locally, the output looks sane to me:
|
8f09cf1
to
71356cb
Compare
The features and usage have been tested successfully locally. |
ports/fluidsynth/portfile.cmake
Outdated
|
||
vcpkg_find_acquire_program(PKGCONFIG) | ||
vcpkg_cmake_configure( | ||
SOURCE_PATH "${SOURCE_PATH}" | ||
OPTIONS | ||
"-DVCPKG_HOST_TRIPLET=${HOST_TRIPLET}" | ||
-DVCPKG_HOST_TRIPLET="${HOST_TRIPLET}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this transform is correct. At least, https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#command-arguments seems to indicate that this will cause the argument to be broken into 2 arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking and while the resulting configure command looks odd "-DVCPKG_HOST_TRIPLET="arm64-osx""
, the value in the cache is properly set:
//No help, variable specified on the command line.
VCPKG_HOST_TRIPLET:UNINITIALIZED=arm64-osx
But it's probably safer to keep it fully quoted as it used to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inner quotes are part of the CMake value and work with some build systems and shells. It is not portable. That's why we want quotes around the CMake value so that CMake can treat the value as needed when forwarding it to specific build systems and shells.
|
||
if ( TARGET ALSA::ALSA AND ALSA_SUPPORT ) | ||
- target_link_libraries ( libfluidsynth-OBJ PUBLIC ALSA::ALSA ) | ||
+ target_link_libraries ( libfluidsynth-OBJ PUBLIC ALSA::ALSA ${CMAKE_DL_LIBS} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should ALSA::ALSA already be including CMAKE_DL_LIBS if necessary? (Should that port be fixed rather than this one?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am having a hard time reproducing the error in question locally, but I agree that it seems a lot like an ALSA issue. Its pkg-config file does include dl
, but the upstream FindALSA module from Kitlab does not seem to search for any potential transitive usage requirements.
I will see about making a vcpkg-cmake-wrapper
for ALSA.
Thanks for the update! |
./vcpkg x-add-version --all
and committing the result.The
add-usage-requirements
patch is no longer necessary as upstream now handles it.Update the logic for setting the CMake options:
MAYBE_UNUSED_VARIABLES
more easily.