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

[directxtk] fix wrong dep on arm #33712

Merged
merged 1 commit into from Sep 15, 2023

Conversation

autoantwort
Copy link
Contributor

The dep also exists on arm, otherwise you get:

CMake Error at C:/v/vcpkg4/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
  Could not find a package configuration file provided by "xaudio2redist"
  with any of the following names:

    xaudio2redistConfig.cmake
    xaudio2redist-config.cmake

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

Thank you for submitting this PR. Our CI does not have a testing environment. Please help confirm that there are no problems with your local testing.

@autoantwort
Copy link
Contributor Author

There are no problems because you get an message that the dependency xaudio2redist is not supported on arm and uwp

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Sep 13, 2023
@BillyONeal
Copy link
Member

This seems OK to me; @walbourn as the nominal 'owner' of these ports do the edits look OK to you? Note that that dependency is already blocked so there should be no need to block it here again:

"supports": "windows & !arm & !uwp & !xbox"

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Sep 13, 2023
@walbourn
Copy link
Member

walbourn commented Sep 14, 2023

The xaudio2redist does not exist on arm32 or arm64. You have to use Xaudio2.9. The same is true for all UWP architectures.

What is the specific triplet and feaure being used here that is deemed a bug? Is it perhaps an upstream bug in my Cmake instead?

@walbourn
Copy link
Member

The only time the xaudio2redist dep should be needed is building:

  • -DBUILD_XAUDIO_WIN7=ON for directxtk which by definition is only x86 or x64 Win32 desktop.
  • -DBUILD_XAUDIO_REDIST=ON for directxtk12 which should only ever be done for x86 or x64 scenarios Win32 desktop.
  • When building for MinGW (which doesn't support ARM)

What scenario are you trying to build for ARM32 and/or ARM64?

@BillyONeal
Copy link
Member

@autoantwort Do you have a repro?

@BillyONeal
Copy link
Member

@walbourn I think the bug is that as written, dirextxtk12[xaudio2redist] exists on arm/uwp/xbox, but just does not actually depend on xaudio2redist. And apparently as a result cmake configs are getting minted referring to that dependency without that dependency existing.

After this PR, vcpkg install dirextxtk12[xaudio2redist] will fail with "xaudio2redist is only supported on windows & !arm & !uwp & !xbox"

@autoantwort
Copy link
Contributor Author

@autoantwort Do you have a repro?

vcpkg x-set-installed directxtk[xaudio2redist]:arm64-windows

@autoantwort
Copy link
Contributor Author

autoantwort commented Sep 15, 2023

After this PR, vcpkg install dirextxtk12[xaudio2redist] will fail with "xaudio2redist is only supported on windows & !arm & !uwp & !xbox"

Yeah that is a better error message than a build failure because some cmake configs are not found.

@BillyONeal
Copy link
Member

I'm interpreting @walbourn 's thumbup as "good to merge" :)

@BillyONeal BillyONeal merged commit e8c2a04 into microsoft:master Sep 15, 2023
15 checks passed
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Sep 19, 2023
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.

None yet

4 participants