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

[steam-audio] Adding port #40942

Merged
merged 95 commits into from
Feb 4, 2025
Merged

[steam-audio] Adding port #40942

merged 95 commits into from
Feb 4, 2025

Conversation

Honeybunch
Copy link
Contributor

@Honeybunch Honeybunch commented Sep 12, 2024

Adding this port for those of us who crave spatial audio

  • 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.

@Honeybunch Honeybunch marked this pull request as ready for review September 26, 2024 19:08
@Honeybunch
Copy link
Contributor Author

Friendly bump that this is ready for review 😃

ports/steam-audio/usage Outdated Show resolved Hide resolved
@JavierMatosD
Copy link
Contributor

@Honeybunch, please mark "ready for review" once you have responded to @FrankXie05 / @dg0yt review comments. Thanks!

@JavierMatosD JavierMatosD marked this pull request as draft October 8, 2024 21:34
@Honeybunch
Copy link
Contributor Author

I am trying to upstream what parts of this patch I can but the Steam-Audio repo hasn't seen activity for two months so it may take a while

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.

Sorry for the delay on the review, looks like this one fell through the cracks. I pushed some nitpick fixes to clean up some meaningless files in the installed package, and make sure the right third party notices were included as they were being installed to $/root/THIRDPARTY.md rather than being part of the copyright file.

This looks good to me now. Can you answer my question about -DVCPKG_HOST_TRIPLET=${HOST_TRIPLET} ?

Thanks for your contribution to vcpkg!

Comment on lines 51 to 52
# So the patched port can find the vcpkg host flatc compiler
-DVCPKG_HOST_TRIPLET=${HOST_TRIPLET}
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see nothing looks for VCPKG_HOST_TRIPLET or HOST_TRIPLET, is this leftover from an earlier revision and no longer used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm pretty sure that's cruft and can be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

It changes how vcpkg.cmake configures the search path for programs. Cf.

option(VCPKG_SETUP_CMAKE_PROGRAM_PATH "Enable the setup of CMAKE_PROGRAM_PATH to vcpkg paths" ON)
set(VCPKG_CAN_USE_HOST_TOOLS OFF)
if(DEFINED VCPKG_HOST_TRIPLET AND NOT VCPKG_HOST_TRIPLET STREQUAL "")
set(VCPKG_CAN_USE_HOST_TOOLS ON)
endif()
cmake_dependent_option(VCPKG_USE_HOST_TOOLS "Setup CMAKE_PROGRAM_PATH to use host tools" ON "VCPKG_CAN_USE_HOST_TOOLS" OFF)
unset(VCPKG_CAN_USE_HOST_TOOLS)
if(VCPKG_SETUP_CMAKE_PROGRAM_PATH)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/tools")
if(VCPKG_USE_HOST_TOOLS)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_HOST_TRIPLET}/tools")
endif()

Also #25529.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh damn yeah good catch. It's been a hot minute since I've last looked at this

Copy link
Member

@BillyONeal BillyONeal Feb 4, 2025

Choose a reason for hiding this comment

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

Hmmm... that seems extremely cursed @dg0yt but not looking to un-curse it right now.

find_program calls

We're only building core.

Flatbuffers: Explicitly passed in
ISPC: Guarded by STEAMAUDIO_ENABLE_EMBREE, which is disabled
Sphinx: Guarded by STEAMAUDIO_BUILD_DOCS, which is disabled

I'll remove it.

@BillyONeal BillyONeal dismissed FrankXie05’s stale review February 4, 2025 05:19

FrankXie is out of office and their comments appear to have been addressed.

@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Feb 4, 2025
@BillyONeal BillyONeal enabled auto-merge (squash) February 4, 2025 05:40
@BillyONeal BillyONeal merged commit d80311e into microsoft:master Feb 4, 2025
17 checks passed
@ethindp
Copy link

ethindp commented Feb 7, 2025

Steam Audio has a new version (4.6.0). In theory it should be trivial to upgrade (I think?)

@FrankXie05
Copy link
Contributor

@ethindp Maybe we can wait for PR ValveSoftware/steam-audio#399 to be merged into the next version before updating. :)

autoantwort pushed a commit to autoantwort/vcpkg that referenced this pull request Feb 8, 2025
Co-authored-by: Kai Pastor <[email protected]>
Co-authored-by: Billy Robert O'Neal III <[email protected]>
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.

6 participants