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

[mp3lame] decouple executable as 'frontend' non-default feature #39969

Merged

Conversation

derekcyruschow-catapult
Copy link
Contributor

Main reason for this is that I was seeing a weird issue when trying to cross-compile on my mac for iOS with the mp3lame port resolving -lcurses but then couldn't find curses.h when compiling frontend/console.c. I couldn't get to the bottom of why this was happening but because I only wanted mp3lame for ffmpeg I realised I could just disable a frontend feature.

  • 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.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • 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.

@derekcyruschow-catapult derekcyruschow-catapult force-pushed the mp3lame_add_default_feature_for_frontend branch from 7cb0e89 to 5b965b8 Compare July 18, 2024 09:15
@derekcyruschow-catapult
Copy link
Contributor Author

derekcyruschow-catapult commented Jul 18, 2024

Reading the maintainer guide I'm unsure to be honest whether frontend should be a default-feature, I just made it so because I didn't want to break backwards compatibility with any custom overlay ports out in the while that might expect it to be on as per before these changes.

If the feature adds additional APIs (or executables, or library binaries) and doesn't modify the behavior of existing APIs, it should be left off by default.

@derekcyruschow-catapult

This comment was marked as resolved.

@derekcyruschow-catapult derekcyruschow-catapult force-pushed the mp3lame_add_default_feature_for_frontend branch from 5b965b8 to d68190e Compare July 18, 2024 09:32
@MonicaLiu0311 MonicaLiu0311 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jul 18, 2024
@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Jul 18, 2024

mp3lame

All features are tested successfully in the following triplet:

x86-windows
x64-windows
x64-windows-static

@dg0yt
Copy link
Contributor

dg0yt commented Jul 18, 2024

IIUC "frontend" means "the lame exeutable", i.e. an executable program.

  • AFAICT it should not be a default feature in vcpkg in this case.
    • vcpkg focus: libraries
    • Default features can only be deselected on the command line or in the top-level manifest.
  • "tool" or "tools" is more widely used for such features.

Dependencies must be controlled, not autodetected. vcpkg has a ncurses port, so there must be either an explicit dependency, or ncurses support must be reliably disabled.

@derekcyruschow-catapult derekcyruschow-catapult changed the title [mp3lame] add default feature for 'frontend' (and disable it for ffmpeg) [mp3lame] decouple executable as 'frontend' non-default feature Jul 18, 2024
@derekcyruschow-catapult
Copy link
Contributor Author

derekcyruschow-catapult commented Jul 18, 2024

Thanks for your comments @dg0yt

AFAICT it should not be a default feature in vcpkg in this case

I've tweaked the PR accordingly.

Just remembered I still need to look into wiring up the feature for non-Windows/MinGW targets which currently use msbuild instead of configure/make. (Done 8041334)

Dependencies must be controlled, not autodetected. vcpkg has a ncurses port, so there must be either an explicit dependency, or ncurses support must be reliably disabled.

For simplicity I'll look at adding the dependency. (Done ce34cba)

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jul 23, 2024
@data-queue data-queue merged commit 04f9031 into microsoft:master Jul 24, 2024
16 checks passed
@derekcyruschow-catapult derekcyruschow-catapult deleted the mp3lame_add_default_feature_for_frontend branch July 24, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants