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

[faudio] new port #37157

Merged
merged 2 commits into from
Mar 12, 2024
Merged

[faudio] new port #37157

merged 2 commits into from
Mar 12, 2024

Conversation

rkitover
Copy link
Contributor

@rkitover rkitover commented Mar 6, 2024

Add new port FAudio, an "accuracy-focused XAudio reimplementation for
open platforms".

Use the PLATFORM_WIN32 cmake flag on Windows, otherwise use the sdl2
dependency.

MSVC build PR is here:

FNA-XNA/FAudio#333

MSYS2 package is here:

https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-faudio/PKGBUILD

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@rkitover
Copy link
Contributor Author

rkitover commented Mar 6, 2024

Please tell me what I'm doing wrong here.

@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 6, 2024
@rkitover rkitover force-pushed the faudio-port branch 2 times, most recently from 9dc0375 to f8f6947 Compare March 6, 2024 17:27
@rkitover
Copy link
Contributor Author

rkitover commented Mar 6, 2024

I see some of the problems now, it will take me a bit more time to sort everything out.

@rkitover rkitover force-pushed the faudio-port branch 4 times, most recently from 102b74c to e30f910 Compare March 6, 2024 20:59
@rkitover rkitover marked this pull request as ready for review March 6, 2024 21:07
@rkitover
Copy link
Contributor Author

rkitover commented Mar 6, 2024

@jimwang118 this is ready for review now, the remaining two CI problems I can do nothing about unless you have ideas. Thank you.

@rkitover rkitover force-pushed the faudio-port branch 3 times, most recently from abd5202 to a518e5f Compare March 7, 2024 17:58
@jimwang118
Copy link
Contributor

Can faudio be compiled on Android or osx? If compilation on these platforms is supported, compilation errors need to be resolved. If it is not supported, you can add "supports": "!osx & !android" to vcpkg.json. In addition, you need to run the command: "./vcpkg x-add-version faudio --overwrite-version", and then submit the generated file.

@jimwang118
Copy link
Contributor

I will first convert your PR to a draft, wait for you to fix all CI errors, and then click "ready for review".

@jimwang118 jimwang118 marked this pull request as draft March 8, 2024 01:56
@rkitover rkitover marked this pull request as ready for review March 8, 2024 02:26
@rkitover
Copy link
Contributor Author

rkitover commented Mar 8, 2024

If you look at the build logs for the CI failures, they are because vcpkg does not like the version string, not actual build failures. I can't do anything about that.

@jimwang118
Copy link
Contributor

If you look at the build logs for the CI failures, they are because vcpkg does not like the version string, not actual build failures. I can't do anything about that.

Yes, version update requires running the command: "./vcpkg x-add-version faudio --overwrite-version".

@rkitover
Copy link
Contributor Author

rkitover commented Mar 8, 2024

I did run that, it is in the checklist. I just ran it again too, but it is the same.

ports/faudio/portfile.cmake Outdated Show resolved Hide resolved
@jimwang118
Copy link
Contributor

When I installed it on local arm64-osx, the same error message appeared. There should be a problem with the code.

/Users/vcpkg/jim/faudio/buildtrees/faudio/src/24.03-583b222848.clean/src/FAudio_internal_simd.c:1080:20: error: expected ';' at end of declaration
        ALIGN(int32_t, 16) data[4] =
                          ^
                          ;
/Users/vcpkg/jim/faudio/buildtrees/faudio/src/24.03-583b222848.clean/src/FAudio_internal_simd.c:1087:25: error: use of undeclared identifier 'data'; did you mean 'atan'?
        adder_frac = vld1q_s32(data);
                               ^~~~
                               atan

@rkitover
Copy link
Contributor Author

rkitover commented Mar 8, 2024

@jimwang118 thank you very much for that info, this means I did not properly implement clang support for my ALIGN() macro which I made to replace a gcc extension.

It would have been nice if the CI build log showed me this error instead of that stuff about an unparseable version.

I will fix the arm64-osx triplet first.

Does arm64-android also use clang?

I will need to do a followup to my upstream PR.

@rkitover rkitover force-pushed the faudio-port branch 2 times, most recently from 2f00739 to 1549489 Compare March 8, 2024 19:07
@rkitover
Copy link
Contributor Author

rkitover commented Mar 8, 2024

@jimwang118 I disabled clang in my ALIGN() macro, now those two CI checks pass, but x86-windows is failing for some reason, can you please find out why? The build for this triplet is succeding on my local machine.

I will check with the upstream developers to check if disabling alignment is safe to do and get back to you.

@rkitover
Copy link
Contributor Author

rkitover commented Mar 8, 2024

Upstream will decide on the clang issue over the next few days, I will implement it and replace the patch here.

If you would like to merge this faster, I can disable the triplets arm64-osx and arm64-android (although I need confirmation that that build environment is clang on Android.)

I believe this is the only outstanding issue.

For reference my folllowup PR and discussion is here:

FNA-XNA/FAudio#334
.

@rkitover rkitover marked this pull request as ready for review March 9, 2024 06:17
@rkitover
Copy link
Contributor Author

rkitover commented Mar 9, 2024

I fixed the alignment patch, this should be ready to go now.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

You must update versions after committing all changes to ports. (But you can amend the last commit.)


Unfortunately, the CMake config and the pc file don't reflect the transitive sdl2 requirement for static linkage.

cmake/config.cmake.in needs

if(NOT "@PLATFORM_WIN32@")
   include(CMakeFindDependencyMacro)
   find_dependency(SDL2 CONFIG)
endif()

after @PACKAGE_INIT@.

cmake/FAudio.pc.in needs

Requires.private: sdl2

but only for non-win32 - maybe sdl2 must be injected via a new variable e.g. @PC_REQUIRES_PRIVATE@.

ports/faudio/portfile.cmake Outdated Show resolved Hide resolved
ports/faudio/usage Outdated Show resolved Hide resolved
ports/faudio/usage Outdated Show resolved Hide resolved
ports/faudio/vcpkg.json Outdated Show resolved Hide resolved
@dg0yt
Copy link
Contributor

dg0yt commented Mar 9, 2024

FTR the usage suggest by vcpkg's heuristic should be good enough:

faudio provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(FAudio CONFIG REQUIRED)
  target_link_libraries(main PRIVATE FAudio::FAudio)

faudio provides pkg-config modules:

    # Accuracy-focused XAudio reimplementation for open platforms
    FAudio

@rkitover rkitover force-pushed the faudio-port branch 2 times, most recently from eecd624 to 9b8ccf9 Compare March 10, 2024 17:36
Add new port FAudio, an "accuracy-focused XAudio reimplementation for
open platforms".

Use the PLATFORM_WIN32 cmake flag on Windows, otherwise use the sdl2
dependency.

MSVC build PR is here:

FNA-XNA/FAudio#333

, followup PR to fix alignment is here:

FNA-XNA/FAudio#334

, and another followup for the SDL2 dependency:

FNA-XNA/FAudio#335

, the MSYS2 package is here:

https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-faudio/PKGBUILD
.

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Contributor Author

@dg0yt thank you very much for your feedback, I have fixed all the issues you flagged as you specified. Your SDL2 dependency patch was merged upstream and is included here.

@jimwang118
Copy link
Contributor

Usage test pass with following triplets:

x86-windows
x64-windows
x64-windows-static

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Mar 11, 2024
@rkitover
Copy link
Contributor Author

Any outstanding issues I need to take care of or can this be merged now?

@jimwang118
Copy link
Contributor

Any outstanding issues I need to take care of or can this be merged now?

You just have to wait for it to merge.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I switched to a commit containing all 3 of the patches and removed them from here so it's clear we are doing exactly as upstream wishes.

Thanks for the new port!

@BillyONeal BillyONeal merged commit 8218590 into microsoft:master Mar 12, 2024
16 checks passed
@rkitover rkitover deleted the faudio-port branch March 12, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants