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

[new port] Add rubberband #24549

Merged
merged 5 commits into from
Sep 22, 2022
Merged

[new port] Add rubberband #24549

merged 5 commits into from
Sep 22, 2022

Conversation

daschuer
Copy link
Contributor

@daschuer daschuer commented May 5, 2022

rubberband is a small library for stretching sounds.
This picks up #17800

  • What does your PR fix?

Fixes [New Port Request] rubberband #17768

  • Which triplets are supported/not supported? Have you updated the CI baseline?

I have so far only tested it on my machine (linux). No I have not updated the CI baseline.

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

@daschuer daschuer force-pushed the add_rubberband branch 2 times, most recently from 6a289db to f98e851 Compare May 5, 2022 11:22
@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 6, 2022
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

Seems this port doesn't support uwp.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for rubberband but no changes to version or port version.
-- Version: 2.0.2
-- Old SHA: 6b503b03aed4028b924d320c5045259a56f05bc9
-- New SHA: db6452933a7581e50c55e3ed1b6861e4f14dad8a
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JackBoosY JackBoosY changed the title Add rubberband [new port] Add rubberband May 6, 2022
JackBoosY
JackBoosY previously approved these changes May 6, 2022
@JackBoosY JackBoosY added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function and removed requires:author-response labels May 6, 2022
@FrankXie05
Copy link
Contributor

Feature test successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

@FrankXie05 FrankXie05 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels May 6, 2022
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.

Want to answer the "can we use the github mirror" question; the other suggestions are nitpicks :)

ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
ports/rubberband/portfile.cmake Outdated Show resolved Hide resolved
@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels May 6, 2022
@daschuer
Copy link
Contributor Author

daschuer commented May 7, 2022

Done

@JackBoosY JackBoosY requested a review from BillyONeal May 9, 2022 06:09
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 9, 2022
github-actions[bot]
github-actions bot previously approved these changes Jun 27, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider replacing the deprecated license identifiers in the following files:

  • ports/libsndfile/vcpkg.json (has deprecated license LGPL-2.1)

Deprecated and non deprecated license identifiers can be found here

github-actions[bot]
github-actions bot previously approved these changes Jun 27, 2022
@daschuer
Copy link
Contributor Author

FAILED: rubberband-program.exe rubberband-program.pdb 
"C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.32.31326/bin/Hostx64/x64/link.exe"  /MACHINE:x64 /OUT:rubberband-program.exe rubberband-program.exe.p/main_main.cpp.obj rubberband-program.exe.p/src_getopt_getopt.c.obj rubberband-program.exe.p/src_getopt_getopt_long.c.obj "/nologo" "/release" "/nologo" "/DEBUG" "/PDB:rubberband-program.pdb" "-machine:x64" "-nologo" "/LIBPATH:D:/installed/x64-windows-static/debug/lib" "librubberband_objlib.a" "D:/installed/x64-windows-static/debug/lib/fftw3.lib" "D:/installed/x64-windows-static/debug/lib/samplerate.lib" "D:/installed/x64-windows-static/debug/lib/sndfile.lib" "D:/installed/x64-windows-static/debug/lib/FLAC.lib" "D:/installed/x64-windows-static/debug/lib/vorbisenc.lib" "D:/installed/x64-windows-static/debug/lib/vorbis.lib" "D:/installed/x64-windows-static/debug/lib/ogg.lib" "D:/installed/x64-windows-static/debug/lib/opus.lib" "D:/installed/x64-windows-static/debug/lib/mpg123.lib" "/LIBPATH:D:/installed/x64-windows-static/debug/lib" "mp3lame.lib" "/SUBSYSTEM:CONSOLE" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
LINK : fatal error LNK1181: cannot open input file 'mp3lame.lib'
ninja: build stopped: subcommand failed.

@daschuer
Copy link
Contributor Author

libsndfile still links against libmp3 even though the feature is now disabled for static builds.
I give up and disable static builds here instead.

@BillyONeal
Copy link
Member

libsndfile still links against libmp3 even though the feature is now disabled for static builds. I give up and disable static builds here instead.

Wait, are you saying that if a feature in a downstream dependency is enabled that this breaks? That's not OK.

@daschuer
Copy link
Contributor Author

Yes, even though this port required only the "default-features": false of libdndfile the mpeg feature is enabled, but broken.
Rubberband does not now emything about mp3, but it is passed to it's linker command because of that. Every executable statically linked to libdndfile suffers this issue, this nothing we can solve in this PR.

@daschuer
Copy link
Contributor Author

Hey, all green finally.

@BillyONeal
Copy link
Member

Yes, even though this port required only the "default-features": false of libdndfile the mpeg feature is enabled, but broken.

Then things need to be patched such that the broken path isn't touched even if someone turns on that feature.

Features are additive. Ports don't get to say "one of my transitive dependencies must be built with a feature off".

What do you mean by "enabled, but broken"?

@daschuer
Copy link
Contributor Author

Then things need to be patched such that the broken path isn't touched even if someone turns on that feature.

This is currently the case. You can freely turn on and off the features nothing is breaks.

Features are additive. Ports don't get to say "one of my transitive dependencies must be built with a feature off".

This does not apply here, because the static build of feature mpeg of libsndfile itself is broken. It is unrelated to rubber-band it is broken for all. libsndfile requires to link a file which is does not exist:
LINK : fatal error LNK1181: cannot open input file 'mp3lame.lib'

What do you mean by "enabled, but broken"?

Currently no target executable is able to link libsndfile statically without explicitly disable the mpeg feature.

@BillyONeal
Copy link
Member

Makes sense.

@daschuer
Copy link
Contributor Author

Is this merge able now?

@JackBoosY
Copy link
Contributor

Still in reviewing.

@JoergAtGithub
Copy link
Contributor

Still in reviewing.

@JackBoosY @FrankXie05 When can we expect an outcome of this review. It would really help, to have Rubberband as VCPKG port!

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 16, 2022
@JackBoosY
Copy link
Contributor

Request further review.

@dan-shaw dan-shaw added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 19, 2022
@dan-shaw dan-shaw merged commit 1d5b2d1 into microsoft:master Sep 22, 2022
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 requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] rubberband
9 participants