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

sfml: add version 3.0.0 #26276

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

sfml: add version 3.0.0 #26276

wants to merge 5 commits into from

Conversation

OMGtechy
Copy link
Contributor

@OMGtechy OMGtechy commented Dec 31, 2024

Summary

Added recipe: sfml/3.0.0

Motivation

SFML 3.0 was released.

Details

Notable changes:

  • cmake required version 3.1 -> 3.24
  • added SFML 3.0.0 source zip + sha256
  • updated test_package.cpp to work with both SFML 3.0.02.0 days.

The cmake minimum version was changed to 3.24 because:

  • Linux SFML 3.0.0 requires 3.22
  • Windows SFML 3.0.0 requires 3.24

It was deemed simpler to just have a single version required for the sake of the test package.

More info: https://github.com/SFML/SFML/blob/7f1162dfea4969bc17417563ac55d93b72e84c1e/CMakeLists.txt#L1-L9


  • [ ✔️ ] Read the contributing guidelines
  • [ 💬 ] Checked that this PR is not a duplicate: list of PRs by recipe
    • I saw some other attempts that looked out-of-date or were more complex in implementation:
    • They appear to remove the check for the sfml-main library rather than fixing it the suffix change post-3.0.0.
    • This PR aims to be:
      • simpler
      • avoid patches
      • keep the check for the sfml-main build artifact
  • [ ✔️ ] Tested locally with at least one configuration using a recent version of Conan
    • Tested on Linux 6.11.11-1-MANJARO SMP PREEMPT_DYNAMIC
      • ✔️ conan create all/conanfile.py --version 3.0.0 --build=missing
      • ✔️ conan create all/conanfile.py --version 2.6.1 --build=missing

changes:
- cmake required version 3.1 -> 3.24
- added SFML 3.0.0 source zip + sha256
- updated test_package.cpp to work with both SFML 3.0.0

The cmake minimum version was changed to 3.24 because:
- Linux SFML 3.0.0 requires 3.22
- Windows SFML 3.0.0 requires 3.24

It was deemed simpler to just have a single version required for the sake of the test package.

More info: https://github.com/SFML/SFML/blob/7f1162dfea4969bc17417563ac55d93b72e84c1e/CMakeLists.txt#L1-L9
@OMGtechy OMGtechy force-pushed the master branch 2 times, most recently from 4598181 to 8fc6848 Compare January 5, 2025 14:16
@OMGtechy OMGtechy marked this pull request as ready for review January 5, 2025 15:05
@OMGtechy
Copy link
Contributor Author

OMGtechy commented Jan 5, 2025

Relevant folks I think would be interested / have useful things to say:

[Hope that's not too spammy / OK]

@OMGtechy
Copy link
Contributor Author

OMGtechy commented Jan 5, 2025

RE multiple being developed at the same time (see #26266) - happy to discuss the merits of each / take the judgement of maintainers or whatever.

They each appear to have their own merits. Perhaps they could be combined?

Pros of this one:

  • doesn't have a separate 3.0.0 directory
  • doesn't use any patches
  • less code duplication (due to lack of 3.0.0 directory)
  • still checks for sfml-main

Pros of alternative:

  • doesn't change default for SFML_USE_SYSTEM_DEPS

[ feel free to add anything I missed here - I am almost certainly bias as author ]

@Sil3ntStorm
Copy link
Contributor

The sole reason mine exists was that I require SFML 3 for a project and it wasn't available on conan, nor did I see a PR for it. So I adjusted and added SFML 3 and figured I might as well upstream it for the benefits of others after some preliminary testing.

I didn't knowingly remove anything that didn't produce errors for me (or later anything that created linter warnings in CCI). So if I removed sfml-main then it caused errors and didn't appear to be required.
I also didn't look into the USE_SYSTEM_DEPS variable much, but it sounds like something CCI likes to do to ensure things are consumed from conan rather than something else. Which is why I kept it and went the patch approach, which is only a minor patch.

@@ -51,6 +52,10 @@ def configure(self):
def layout(self):
cmake_layout(self, src_folder="src")

def build_requirements(self):
if Version(self.version) >= "3.0.0":
self.tool_requires("cmake/[>=3.24]")

Choose a reason for hiding this comment

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

3.24 is only required on Windows. All other OSes only require 3.22.

if Version(self.version) >= "3.0.0":
return 17
elif Version(self.version) >= "2.6.0":
return 11

Choose a reason for hiding this comment

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

Version 2.6 and prior requires C++03, not C++11.

@@ -91,10 +107,12 @@ def generate(self):
tc.variables["SFML_BUILD_AUDIO"] = self.options.audio
tc.variables["SFML_INSTALL_PKGCONFIG_FILES"] = False
tc.variables["SFML_GENERATE_PDB"] = False
tc.variables["SFML_USE_SYSTEM_DEPS"] = True
tc.variables["SFML_USE_SYSTEM_DEPS"] = Version(self.version) < "3.0.0"

Choose a reason for hiding this comment

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

Why change this? A package manager probably wants to use its own dependencies instead of the dependencies bundled with the library.

tc.variables["CMAKE_CXX_STANDARD"] = 11

if self.settings.compiler.cppstd is not None:
tc.cache_variables["CMAKE_CXX_STANDARD"] = self._min_cppstd

Choose a reason for hiding this comment

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

There is no need to set this. The CMake script already uses target_compile_features to set the proper C++ standard version.

@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.1)
cmake_minimum_required(VERSION 3.24)

Choose a reason for hiding this comment

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

Building SFML requires 3.22 (or 3.24 on Windows as mentioned above). Using SFML doesn't require anything particularly new. 3.1 is probably too old but something like 3.10 or 3.16 is probably sufficient. 3.16 or 3.22 are reasonable choices since it's rare you find an OS still in use today that ships with anything older than that.

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.

3 participants