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

[Spout2] New Port #30119

Merged
merged 17 commits into from
Mar 15, 2023
Merged

[Spout2] New Port #30119

merged 17 commits into from
Mar 15, 2023

Conversation

reitowo
Copy link
Contributor

@reitowo reitowo commented Mar 10, 2023

Fixes #30117

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

ports/spout2/usage Outdated Show resolved Hide resolved
ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 13, 2023
@jimwang118
Copy link
Contributor

All feature passed with following triplets:

x64-windows
x64-windows-static

@jimwang118 jimwang118 requested a review from LilyWangLL March 13, 2023 07:06
@dg0yt
Copy link
Contributor

dg0yt commented Mar 13, 2023

This comment was marked as resolved, but isn't IMO:

Vcpkg doesn't want to build both Spout2::Spout and Spout2::Spout_static in the same triplet. Only one will match the triplet's VCPKG_LIBRARY_LINKAGE.

@reitowo
Copy link
Contributor Author

reitowo commented Mar 13, 2023

Only one will match the triplet's VCPKG_LIBRARY_LINKAGE.

Sure, but SpoutDX and SpoutLibrary use Spout_static as link dependency, so this target is got to be built. What can I do is prevent Spout_static being installed to vcpkg buildtree.

But after all, user still needs to import cmake target manually. I think it is ok for user to use Spout / Spout_static occasionally if they want static linkage according to the usage file says. In this situation, I think it makes no difference to prevent installing libs. What's more, it can keep upstream files untouched (which I remember is an preference of vcpkg).

ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
ports/spout2/portfile.cmake Outdated Show resolved Hide resolved
LilyWangLL
LilyWangLL previously approved these changes Mar 13, 2023
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Mar 13, 2023
@reitowo
Copy link
Contributor Author

reitowo commented Mar 15, 2023

Why this still not merged?👀

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.

Does this port need OpenGL-related dependencies?
It has

#include <gl/gl.h> // For OpenGL definitions

Comment on lines 18 to 20
"c": {
"description": "The Spout SDK as a C-compatible library."
},
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I wonder if this makes sense to be a separate feature.

Copy link
Contributor Author

@reitowo reitowo Mar 15, 2023

Choose a reason for hiding this comment

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

This C-compatible library can also be switched on/off by SPOUT_BUILD_LIBRARY. It really doesn't make any significant difference (to me) about whether separating this option as a feature or making it default on. So I'll just leave it there, I think making this an option is better than making this default on anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add features at any time, but it is disruptive to remove them.

AFAICS this library has C++ API, so c is a disnomer.
https://github.com/leadedge/Spout2/blob/master/SPOUTSDK/SpoutLibrary/SpoutLibrary.h

AFAIU it is just a tiny alternative DLL surface on top of Spout_static.
https://github.com/leadedge/Spout2/blob/master/SPOUTSDK/SpoutLibrary/CMakeLists.txt

That's why I'm reluctant to accept a feature c here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound reasonable, I'll remove that.

@reitowo
Copy link
Contributor Author

reitowo commented Mar 15, 2023

Does this port need OpenGL-related dependencies?

Yes, but it seems included in Windows SDK and doesn't makes a dependency error on both my computer and CI.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 15, 2023

Does this port need OpenGL-related dependencies?

Yes, but it seems included in Windows SDK and doesn't makes a dependency error on both my computer and CI.

It doesn't make errors, but vcpgk owners want port opengl to be used to make the dependency explicit.
(This has been discussed elsewhere. I just tell you. My POV: "I fail to see a robust benefit from the copying of SDK headers.")

@reitowo
Copy link
Contributor Author

reitowo commented Mar 15, 2023

Thank for telling that, I'll add it.

@dan-shaw dan-shaw merged commit b12243b into microsoft:master Mar 15, 2023
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.

[New Port Request] Spout2
5 participants