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

[rubberband] Update to 3.2.1 and use recommended sleef dependecy where supported #31732

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

JoergAtGithub
Copy link
Contributor

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

This PR:

@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label May 31, 2023
@JoergAtGithub
Copy link
Contributor Author

I don't understand what happens on arm64_windows.
Both vcpkg.json and the portfile contain the same architecture conditions,

    {
      "name": "fftw3",
      "platform": "(arm & windows) | (arm64 & osx)"
    },
    {
      "name": "sleef",
      "platform": "!(arm & windows) & !(arm64 & osx)"
    },
if(VCPKG_TARGET_IS_WINDOWS AND VCPKG_TARGET_ARCHITECTURE STREQUAL "arm")
    set(FFT_LIB "fftw")

but on arm64_windows it installs the dependency
fftw3:arm64-windows: SUCCEEDED: 30.9 ms
instead of sleef, while the compile parameter from the portfile is set to -Dfft=sleef :
Command failed: D:/downloads/tools/python/python-3.10.7-x64/python.exe D:/installed/x64-windows/tools/meson/meson.py -Dfft=sleef -Dresampler=libsamplerate -Dipp_path= -Dextra_include_dirs= -Dextra_lib_dirs= -Djni=disabled -Dladspa=disabled -Dlv2=disabled -Dvamp=disabled -Dcmdline=disabled -Dtests=disabled --buildtype plain --backend ninja --wrap-mode nodownload --native C:/a/1/s/scripts/buildsystems/meson/none.txt --libdir lib --cross D:/buildtrees/rubberband/meson-arm64-windows-dbg.log -Ddebug=true --prefix D:/packages/rubberband_arm64-windows/debug --includedir ../include -Dcmake_prefix_path=['D:/installed/arm64-windows/debug','D:/installed/arm64-windows','D:/installed/arm64-windows/share'] D:/buildtrees/rubberband/src/v3.2.1-b2bb56759a.clean

For me it seems that arm is different during evaluation of the vcpkg.json file and during execution of the portfile.

Question: Should arm be set in case of arm64 or only for the 32 bit arm arcitecture?

@dg0yt
Copy link
Contributor

dg0yt commented Jun 3, 2023

In platform expressions, arm includes arm64. That's why you will find cases of, or need, (arm & !arm64).

@dg0yt
Copy link
Contributor

dg0yt commented Jun 3, 2023

Cf. https://learn.microsoft.com/en-us/vcpkg/reference/vcpkg-json#platform-expression

@dg0yt
Copy link
Contributor

dg0yt commented Jun 3, 2023

For discussion/consideration:

Reading https://github.com/breakfastquay/rubberband/blob/default/COMPILING.md#fft-libraries-supported, fftw3 seems to have two disadvantages for Apple platforms: slower than the default vDSP and using a restrictive license.

I wonder if we should add minimal feature control here. AFAIU there are different FFT backends, but not a different surface. We must not model "alternatives", but we can model the fast FFT "behaviour":

  • Default(core): Use the system vDSP lib on Apple targets, or the builtin FFT implementation otherwise.
    This represents upstream's defaults.
  • fast-fft: The fastest FFT backend.
    • no effect for Apple targets.
    • otherwise using sleef where supported
    • otherwise using fftw3.

@JoergAtGithub
Copy link
Contributor Author

Rubberband consumes very significiant CPU ressources, for practical use, you always need fast FFT. The default is therefore easy to build but unusable in real applications.

The problem with Apple platforms is, that the fast Apple implemention requires this framework to be installed: https://developer.apple.com/documentation/accelerate
I guess VCPKG can't detect if it's installed. An I personal have no Mac to develop such a detection script. But the upstream manual doesn't state that the Apple implementation is faster than the sleef implementation.

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft June 5, 2023 09:50
@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Jun 5, 2023

When installing sleef on x86-windows, the following error occurs:

E:\rubberband\buildtrees\sleef\src\3.5.1-7f3a2e645a.clean\src\libm\sleefsimddp.c(2104) : fatal error C1001: Internal compiler error.
(compiler file 'D:\a\_work\1\s\src\vctools\Compiler\Utc\src\p2\main.c', line 225)
 To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com 
Please choose the Technical Support command on the Visual C++ 
 Help menu, or open the Technical Support help file for more information
  cl!RaiseException()+0x69
  cl!RaiseException()+0x69
  cl!CloseTypeServerPDB()+0x20d78f
  cl!CloseTypeServerPDB()+0xd2f1d

This is a VS bug, see: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1334513/ , it has not been fixed yet.
Users of both VS 2019 and VS 2022 will face this error if you replace feature fftw with sleef on x86-windows.

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review"

@JoergAtGithub JoergAtGithub marked this pull request as ready for review June 6, 2023 19:00
@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Jun 7, 2023

All features are tested successfully in the following triplet:

x86-windows
x64-windows
arm64-windows
x64-osx

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jun 7, 2023
@dan-shaw dan-shaw merged commit 037ef82 into microsoft:master Jun 7, 2023
@JoergAtGithub
Copy link
Contributor Author

Thank you!

@JoergAtGithub JoergAtGithub deleted the rubberband_321 branch June 7, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants