Skip to content

Conversation

sylveon
Copy link
Contributor

@sylveon sylveon commented Apr 10, 2020

This adds the Discord Game SDK as installable library in vcpkg. It supports Windows x86, Windows x64, macOS x64 and Linux x64.

It's a bit of an oddball because it has a C-compatible DLL component and source code that needs to be built in the consumer's C++ project to provide a C++ API. This is worked around in vcpkg by compiling the source code as static library, so the final package contains both static and dynamic contents. The DLL is always required, so the package ensures ONLY_DYNAMIC_LIBRARY.

This is opened as draft because I'm unsure about a couple of things.

  • I do not use macOS, and a web search was not of big help about the utility of the .bundle file in the SDK download. What should I do with it?
  • I want to build pdbs for both release and debug static libraries but for some reason none seem to be generated, even in debug.

Any assistance about those two details would be appreciated.

@sylveon
Copy link
Contributor Author

sylveon commented Apr 10, 2020

Seems like CI ignores !static. I specified it but the Windows CI fails because it tried to build it in static, but Supports should make it ignore that.

@NancyLi1013
Copy link
Contributor

Could you please look into the regressions on Windows and osx?
x64-windows-static:

Could not locate cached archive: C:\vsts\_work\3\s\archives\ed\edb57939d5da949ebe8b884e77c0b876ccba53cb.zip
-- Note: discord-game-sdk only supports dynamic library linkage. Building dynamic library.
CMake Error at scripts/cmake/vcpkg_check_linkage.cmake:42 (message):
  Refusing to build unexpected dynamic library against the static CRT.  If
  this is desired, please configure your triplet to directly request this
  configuration.
Call Stack (most recent call first):
  ports/discord-game-sdk/portfile.cmake:6 (vcpkg_check_linkage)
  scripts/ports.cmake:90 (include)

x64-osx:

CMake Warning at /Volumes/data/work/1/s/scripts/buildsystems/vcpkg.cmake:124 (message):
  There are no libraries installed for the Vcpkg triplet x64-osx.
Call Stack (most recent call first):
  /Volumes/data/downloads/tools/cmake-3.14.0-osx/cmake-3.14.0-Darwin-x86_64/CMake.app/Contents/share/cmake-3.14/Modules/CMakeDetermineSystem.cmake:93 (include)
  CMakeLists.txt:3 (project)


CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
SDK_LIB
    linked by target "discord_game_sdk_cpp" in directory /Volumes/data/work/1/s/buildtrees/discord-game-sdk/src/d_game_sdk-4c2a1762b7

@sylveon
Copy link
Contributor Author

sylveon commented Apr 14, 2020

@NancyLi1013 static is expected to fail, and I added && !static to my Supports. Shouldn't the CI skip the static configuration?

As for macOS, I'm still unsure what needs to be done with the .bundle file that is part of the SDK. Does vcpkg have any handling for that?

@JackBoosY
Copy link
Contributor

vcpkg is an open source package manager. We hope that the integrated ports are all open source to ensure the security and reliability of the library.

@sylveon
Copy link
Contributor Author

sylveon commented Apr 16, 2020

Whether that SDK is open source or not is out of my control, I'm just attempting to use it in my own OSS project, and the recommended way (putting the files in your source project) is incompatible with OSS projects since that is redistribution of the files which is against the license of the SDK (a script locally acquiring them like a vcpkg portfile is fine however). Telling contributors to manually put them somewhere in the project is also a very poor option for a plethora of reasons. I could use a local ports overlay (and I already do for some packages that have custom patches or custom libraries I don't feel are in a good enough state to release them to everyone) but I'd rather see this merged within vcpkg itself so that its both kept up to date with changes in vcpkg and reusable by others.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@JackBoosY
Copy link
Contributor

After discussion, we think that this port can be added to vcpkg.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please add the following code to VCPKG_PATH/scripts/ci.baseline.txt:

discord-game-sdk:x64-osx=fail
discord-game-sdk:x64-windows-static=fail

@sylveon
Copy link
Contributor Author

sylveon commented Apr 21, 2020

macOS shouldn't fail, I can fix it, but I am still unsure about what to do with the .bundle file that the Game SDK zip includes for macOS. Should I just ignore it?

@JackBoosY
Copy link
Contributor

JackBoosY commented Apr 22, 2020

@sylveon Since we have archived the binary, if the bundle file has nothing to do with the archived binaries, we can install it to share/${PORT} .

@sylveon
Copy link
Contributor Author

sylveon commented Apr 23, 2020

Has been clarified to me that the .bundle file is for the Unity game engine, so we don't need it in the vcpkg package.

@sylveon sylveon marked this pull request as ready for review April 23, 2020 21:56
@sylveon
Copy link
Contributor Author

sylveon commented Apr 23, 2020

Alright this PR is now in a state that I consider acceptable so I'll mark it ready for review.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Apr 24, 2020
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Hey, could you change the license file to a link to https://discordapp.com/developers/docs/legal? We want to make sure it stays up to date :)

@sylveon
Copy link
Contributor Author

sylveon commented May 2, 2020

@strega-nil done!

@JackBoosY JackBoosY requested a review from strega-nil May 6, 2020 05:45
@strega-nil
Copy link
Contributor

Cool, thanks @sylveon :)

@strega-nil strega-nil merged commit 411929e into microsoft:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants