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

[vst3sdk] New port for Steinberg Virtual Studio Technology 3 SDK #41903

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

JoergAtGithub
Copy link
Contributor

  • 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.
  • [n.a.] 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.
  • [n.a.] 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.

@JoergAtGithub
Copy link
Contributor Author

The optional GUI module https://github.com/steinbergmedia/vstgui is not added to this port, as it has own version numbering and release cycle.

I was not able to get the macOS builds working on CI and therefore disabled them. It seems that the projects CMake files require the GUI version of Xcode and do not work with the command line version.

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.

Regarding osx:

ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Contributor Author

I already tried to set ENV{XCODE_VERSION}, but this alone was not sufficient.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 2, 2024

Taking another look, that cmake script doesn't look convincing at all.
They determine (cmake) XCODE_VERSION when ENV{XCODE_VERSION} isn't set.
They determine (cmake) CMAKE_OSX_SYSROOT when ENV{SDKROOT} isn't set.

The key question is if they really need the XCode generator or GUI at some point, or if they just didn't know how to to make their cmake build more flexible.

ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 4, 2024
versions/v-/vst3sdk.json Outdated Show resolved Hide resolved
Removed accidental added version entry "3.7.12" from vst3sdk.json.
Comment on lines 133 to 134
# Suppress CRT linkage check
set(VCPKG_POLICY_SKIP_CRT_LINKAGE_CHECK enabled)
Copy link
Contributor

@dg0yt dg0yt Nov 5, 2024

Choose a reason for hiding this comment

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

Why is it okay to suppress this check?

(Why doesn't it use vcpkg_check_linkage()?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not get it to work with vcpkg_check_linkage so I removed these triplets from the list of supported triplets by !staticcrt

@JonLiu1993
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JonLiu1993
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Please format if/else like

if(...)
    if(...)
        a()
    else()
        b()
    endif()
endif()

ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
ports/vst3sdk/portfile.cmake Outdated Show resolved Hide resolved
Use executables from release build, to support release-only community triplets
Fixed if/else/endif indention
@JonLiu1993
Copy link
Member

@JoergAtGithub, could you please reply @dg0yt's review?

@JonLiu1993
Copy link
Member

Tested features [hosting-examples,hosting-examples] successfully in the followng triplet:

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

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 13, 2024
Comment on lines +91 to +97
if (VCPKG_TARGET_IS_LINUX)
file(INSTALL "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/bin/Release/moduleinfotool" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
file(INSTALL "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/bin/Release/validator" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
else()
file(INSTALL "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/bin/moduleinfotool${VCPKG_TARGET_EXECUTABLE_SUFFIX}" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
file(INSTALL "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/bin/validator${VCPKG_TARGET_EXECUTABLE_SUFFIX}" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Are these executables required for the proper function of the library? Executable files should be installed in tools\${PORT} rather than share\${PORT}

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.

4 participants