Skip to content
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

[portaudio] Fix build on windows #13982

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Conversation

Nemirtingas
Copy link
Contributor

Describe the pull request

  • What does your PR fix? I couldn't build portaudio on windows. This fixes this, and now we also only build static when static linking or shared when shared linking.

@NancyLi1013 NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Oct 12, 2020
Copy link
Contributor

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. It would be better, I think, to forgo the patch and instead set -DPA_BUILD_SHARED correctly in the vcpkg_configure_cmake call in portfile.cmake. You could figure out the correct value by doing something like:

string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} dynamic PA_BUILD_SHARED)

@Nemirtingas
Copy link
Contributor Author

This doesn't look right. It would be better, I think, to forgo the patch and instead set -DPA_BUILD_SHARED correctly in the vcpkg_configure_cmake call in portfile.cmake. You could figure out the correct value by doing something like:

string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} dynamic PA_BUILD_SHARED)

But the static build add a -static to the library name, when reading the vcpkg documentation its written that its better to have 1 name for both static and dynamic libs.

@Hoikas
Copy link
Contributor

Hoikas commented Oct 12, 2020

Ok. Looking at the upstream CMakeLists.txt, if PA_LIBNAME_ADD_SUFFIX is set to OFF, the static suffix is not added (source) on all non-MSVC compilers. There is code in the CMakeLists to force it on MSVC, but the fix-library-can-not-be-found.patch patch should fix that issue. So, I'm not really sure where the problem is.

It does look like we need to appropriately set PA_BUILD_STATIC in the portfile, though.

string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} static PA_BUILD_STATIC)

@Nemirtingas
Copy link
Contributor Author

Nemirtingas commented Oct 12, 2020

When I built it on linux, I got 1 static and 1 dynamic libraries even if the LINKAGE was static. I triied to set PA_BUILD_STATIC and PA_BUILD_SHARED but it still exports the library as portaudio_static

The package portaudio:x64-linux provides CMake targets:

    find_package(portaudio CONFIG REQUIRED)
    target_link_libraries(main PRIVATE portaudio_static)

even if the output name is actually portaudio.a

ls -l installed/x64-linux/lib/libportaudio.a
-rw-r--r-- 1 root root 264474 oct.  12 07:33 installed/x64-linux/lib/libportaudio.a

@NancyLi1013 NancyLi1013 changed the title Fix portaudio build on windows [portaudio] Fix build on windows Oct 12, 2020
@Nemirtingas Nemirtingas reopened this Oct 12, 2020
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update version in CONTROL file.

Update
Version: 2020-02-02
as
Version: 2020-02-02
Port-Version: 1
.
Please see https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#update-the-port-version-field-in-the-manifest-file-of-any-modified-ports

@Nemirtingas
Copy link
Contributor Author

Woops, yes sorry, I forgot to push it.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Oct 13, 2020
@dan-shaw dan-shaw merged commit 654adc6 into microsoft:master Oct 13, 2020
traversaro added a commit to robotology/robotology-superbuild-dependencies-vcpkg that referenced this pull request Oct 13, 2020
As the ogre ( microsoft/vcpkg#14006 ) and portaudio ( microsoft/vcpkg#13982 ) problems were fixed.
@Nemirtingas Nemirtingas deleted the portaudio_fix branch October 14, 2020 08:50
traversaro added a commit to robotology/robotology-superbuild-dependencies-vcpkg that referenced this pull request Oct 16, 2020
* Install directly gts as a Gazebo dependency

As microsoft/vcpkg#10422 was fixed.

* Gazebo and Ignition dependencies: use released version

Fix #26 .

* Bump vcpkg to recent commit

As the ogre ( microsoft/vcpkg#14006 ) and portaudio ( microsoft/vcpkg#13982 ) problems were fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants