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.

Sign up for GitHub

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

Jump to bottom

[vcpkg baseline][libao] Disable dlfcn check under windows #23235

Merged
merged 3 commits into from
Feb 23, 2022
Merged

[vcpkg baseline][libao] Disable dlfcn check under windows #23235

merged 3 commits into from
Feb 23, 2022

Conversation

Cheney-W
Copy link
Contributor

@Cheney-W Cheney-W commented Feb 23, 2022 •
edited
Loading

Describe the pull request
In an internal CI test, libao installation failed with following errors:

.././../src/1.2.2-d87489a217.clean/src/audio_out.c(291): error C2065: 'DIR': undeclared identifier
.././../src/1.2.2-d87489a217.clean/src/audio_out.c(291): error C2065: 'plugindir': undeclared identifier
.././../src/1.2.2-d87489a217.clean/src/audio_out.c(301): error C2037: left of 'd_name' specifies undefined struct/union 'dirent'
.././../src/1.2.2-d87489a217.clean/src/audio_out.c(301): error C2198: 'strlen': too few arguments for call
.././../src/1.2.2-d87489a217.clean/src/audio_out.c(301): error C2057: expected constant expression
.././../src/1.2.2-d87489a217.clean/src/audio_out.c(301): error C2466: cannot allocate an array of constant size 0

This issue only occurs when dlfcn-win32 installed before libao in Windows. Libao always check dlfcn.h and enable the macro HAVE_DLOPEN if dlfcn.h is exists, then the DIR struct will be compiled, in windows environment this will cause the above errors to appear.

For fixing this issue, I passed a cache variable to disable the search of dlfcn under windows

No feature need to be tested.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Feb 23, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Feb 23, 2022

Does it need a patch, or could you just pass a corresponding cache vale (ac_cv_...=no or similar) to configure? (See config.log... for ac_cv_... variables resulting from detection.)

@Cheney-W Cheney-W changed the title [libao] Add parameter to control whether to detect dlfcn [vcpkg baseline][libao] Add parameter to control whether to detect dlfcn Feb 23, 2022
@JackBoosY JackBoosY mentioned this pull request Feb 23, 2022
Closed
@JackBoosY JackBoosY self-assigned this Feb 23, 2022
@Cheney-W
Copy link
Contributor Author

@dg0yt Thanks for your guidance!

@Cheney-W Cheney-W changed the title [vcpkg baseline][libao] Add parameter to control whether to detect dlfcn [vcpkg baseline][libao] Disable dlfcn check under windows Feb 23, 2022
JackBoosY
JackBoosY approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

LGTM if the tests passed.

@JackBoosY JackBoosY marked this pull request as ready for review February 23, 2022 09:38
@JackBoosY
Copy link
Contributor

@vicroms Please consider to merge this PR first.

@BillyONeal BillyONeal merged commit b23242b into microsoft:master Feb 23, 2022
dg0yt
dg0yt reviewed Feb 24, 2022
View reviewed changes
ports/libao/portfile.cmake
@@ -1,5 +1,7 @@
set(NO_DLFCN )
Copy link
Contributor

@dg0yt dg0yt Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is bad CMake style. It unsets the variable in the current scope, unhiding a possible value from a parent scope. For a clean variable, always use:

set(VAR "")

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/cmake-guidelines.md

ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Feb 27, 2022
@ekilmer
Merge branch 'master' into cpplocate
6be6aa5 
* master: (57 commits)
  [vcpkg-tools] update cmake and git (windows only) (microsoft#22985)
  Update vcpkg tool to 2022-02-24. (microsoft#23162)
  [vcpkg baseline] Move cspice headers (microsoft#23272)
  Fixed inaccurate Chinese words (microsoft#23179)
  [vcpkg] Add fixed changelog generator. (microsoft#23255)
  [authentication.md] Add Jenkins section (microsoft#23226)
  [vcpkg] Meson osx sysroot (microsoft#21772)
  [pkgconf] enable search for system libs on linux (microsoft#23010)
  [yasm/yasm-tool] Incorporate yasm-tool into yasm (microsoft#23218)
  [lapack-reference] Update to 3.10 (microsoft#23228)
  [skia] Arm64 for skia on osx (microsoft#23222)
  [libfido2] Update to 1.10.0 (microsoft#23241)
  [Tracy] Fixing issue where version 0.7.8 was pulling the wrong version (microsoft#23061)
  [libgpiod] Add new port. (microsoft#23221)
  [drogon] Update to 1.7.5 (microsoft#23227)
  [tinyexif] Remove from fail list. (microsoft#23163)
  [vcpkg docs][ES] Sync with English readme (microsoft#19834) (microsoft#22618)
  [vcpkg baseline][libao] Disable dlfcn check under windows (microsoft#23235)
  [OpenCV] upgrade to v4.5.5 (microsoft#22801)
  [libcurl-simple-https] New port (microsoft#22917)
  ...
@Cheney-W Cheney-W deleted the Dev/Cheney/libao-C2065 branch August 30, 2022 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@github-actions GitHub Actions github-actions approved these changes

@JackBoosY JackBoosY JackBoosY approved these changes

@BillyONeal BillyONeal Awaiting requested review from BillyONeal

Assignees

@JackBoosY JackBoosY

@Adela0814 Adela0814

Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Cheney-W @dg0yt @JackBoosY @BillyONeal @Adela0814