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

Try GameNetworkingSockets #7040

Merged
merged 35 commits into from
Oct 15, 2021

Conversation

TheClonerx
Copy link
Contributor

@TheClonerx TheClonerx commented Aug 24, 2021

Specify library name and version: game-networking-sockets/1.3.0

I saw this listed on #621 and thought it would be a good addition to CCI.

Closes #7589


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@Croydon Croydon left a comment

Choose a reason for hiding this comment

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

package_info() and a test_package are missing

@TheClonerx
Copy link
Contributor Author

So, GameNetworkingSockets unconditionally builds both the static and shared libraries here. Should I surround these by an if-else in a patch?

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@TheClonerx
Copy link
Contributor Author

TheClonerx commented Aug 30, 2021

  • The protobuf:shared option has to be somehow reflected on this package's ID.
  • Some tests fail due to old compilers not supporting static_assert, while on Windows there's some weird permission shenanigans going on.
  • Cross-compiling doesn't work (Linux -> Windows) due to CMake using the Windows protoc binaries.
  • A patch is needed to only build and install either the shared or static library.

@Croydon
Copy link
Contributor

Croydon commented Aug 30, 2021

So, GameNetworkingSockets unconditionally builds both the static and shared libraries here. Should I surround these by an if-else in a patch?

Patches are hard to maintain. Probably the cleanest way, would be to see if upstream would accept an optional build setting to only build one of them. The easier one would be to just delete the respective unwanted library in package()

Cross-compiling doesn't work (Linux -> Windows) due to CMake using the Windows protoc binaries.

You can add a package both as a requirement and as a build_requirement. You also have to use two Conan profiles, both together should accomplish this (but I have almost zero hands-on experience with that setup myself, so I can't really help further)

Some tests fail due to old compilers not supporting static_assert, while on Windows there's some weird permission shenanigans going on.

If that is nothing that can be easily fixed, then exclude these old, unsupported compilers. Something along the lines of:

TheClonerx and others added 2 commits August 29, 2021 22:01
Co-authored-by: Michael Keck <git@cr0ydon.com>
Co-authored-by: Michael Keck <git@cr0ydon.com>
@conan-center-bot

This comment has been minimized.

TheClonerx and others added 4 commits October 11, 2021 17:16
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot

This comment has been minimized.

@TheClonerx TheClonerx force-pushed the game-networking-sockets branch from 9802805 to 5d1c88a Compare October 11, 2021 23:02
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@TheClonerx
Copy link
Contributor Author

So, ConanInvalidConfiguration can't be raised from build(), but deps_cpp_info is not available in validate().
I'm guessing that we can't check openssl version at all then.

@prince-chrismc
Copy link
Contributor

That's odd... we have a bunch of recipes that do this... Also I just wrote #7621 to do exactly that 😕

def build(self):
if tools.Version(self.version) <= "2.0.1" and "asio" in self.deps_cpp_info.deps and tools.Version(self.deps_cpp_info["asio"].version) >= "1.18.0":

It's a topic in another PR as well #7510 (comment)

@prince-chrismc
Copy link
Contributor

Please do not force push 🙏 GitHub forces us to restart the review which is not fun!

@conan-center-bot

This comment has been minimized.

@TheClonerx
Copy link
Contributor Author

Oh, i didn't know that.
Thanks for telling me that

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM Thank you for your contribution

@conan-center-bot
Copy link
Collaborator

All green in build 22 (58c76d45d36a92b952ee8094274df67ca0a773c4):

  • gamenetworkingsockets/1.3.0@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit c9ef544 into conan-io:master Oct 15, 2021
@TheClonerx TheClonerx deleted the game-networking-sockets branch October 24, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[request] ValveSoftware/GameNetworkingSockets
7 participants