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

sdl >= 2.0.18: fix CMake options #8609

Merged

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Jan 2, 2022

Specify library name and version: lib/1.0

closes #8607


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

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

We can check if the new model with self.dependencies is able to retrieve information from the build context, but we cannot use deps_cpp_info.

Probably we can just assume WAYLAND_SCANNER_1_15_FOUND=1 because the build-requires (wayland/1.19.0) is not going to be overriden.

self._cmake.definitions["CONAN_INSTALL_FOLDER"] = self.install_folder
if self.settings.os != "Windows" and not self.options.shared:
self._cmake.definitions["SDL_STATIC_PIC"] = self.options.fPIC
if self.settings.compiler == "Visual Studio" and not self.options.shared:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.settings.compiler == "Visual Studio" and not self.options.shared:
if self.settings.compiler in ["Visual Studio", "msvc"] and not self.options.shared:

wdyt? Should we start considering msvc or maybe it is better to create a tool to check for VS compiler?

Copy link
Contributor Author

@SpaceIm SpaceIm Jan 2, 2022

Choose a reason for hiding this comment

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

yes, there are 2 occurrences of Visual Studio, so it shouldn't be a big change. Moreover, this recipe already requires conan 1.43.0 at least.
#6155

@@ -247,18 +249,84 @@ def _configure_cmake(self):
# FIXME: Otherwise 2.0.16 links with system wayland (from egl/system requirement)
cmake_extra_ldflags += ["-L{}".format(os.path.join(self.deps_cpp_info["wayland"].rootpath, it)) for it in self.deps_cpp_info["wayland"].libdirs]
self._cmake.definitions["WAYLAND_SHARED"] = self.options["wayland"].shared
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = 1 # FIXME: Check actual build-requires version
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = tools.Version(self.deps_cpp_info["wayland"].version) >= "1.15.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = tools.Version(self.deps_cpp_info["wayland"].version) >= "1.15.0"
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = 1 # FIXME: Check actual build-requires version

using deps_cpp_info we get the information from the host context, not the build one. Here we need to check the version of the build-requires, not the requires

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'll revert this change, didn't see that wayland was also in build requirements.

# FIXME: Otherwise 2.0.16 links with system wayland (from egl/system requirement)
cmake_extra_ldflags += ["-L{}".format(os.path.join(self.deps_cpp_info["wayland"].rootpath, it)) for it in self.deps_cpp_info["wayland"].libdirs]
self._cmake.definitions["SDL_WAYLAND_SHARED"] = self.options["wayland"].shared
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = tools.Version(self.deps_cpp_info["wayland"].version) >= "1.15.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = tools.Version(self.deps_cpp_info["wayland"].version) >= "1.15.0"
self._cmake.definitions["WAYLAND_SCANNER_1_15_FOUND"] = 1 # FIXME: Check actual build-requires version

Probably here it also want to check the version from the build context.

@conan-center-bot
Copy link
Collaborator

All green in build 3 (a88329b90fb49ac4ee5e9e679c994e7cf11b95ce):

  • sdl/2.0.18@:
    All packages built successfully! (All logs)

  • sdl/2.0.16@:
    All packages built successfully! (All logs)

  • sdl/2.0.14@:
    All packages built successfully! (All logs)

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

@conan-center-bot conan-center-bot merged commit 52460a9 into conan-io:master Jan 3, 2022
@SpaceIm SpaceIm deleted the fix/sdl-2.0.18-cmake-options branch January 3, 2022 12:02
miklelappo pushed a commit to miklelappo/conan-center-index that referenced this pull request Feb 9, 2022
* fix CMake options if version >= 2.0.18

* properly check wayland version

* Revert "properly check wayland version"

This reverts commit daa7008.

* add compiler=msvc support
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.

[package] sdl/2.0.18: cmake defines changed upstream
5 participants