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

Add SFML:: namespace to targets #1947

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

ChrisThrasher
Copy link
Member

@ChrisThrasher ChrisThrasher commented Dec 30, 2021

Description

This removes the sfml- prefixed targets from the export set. The sfml- prefixed targets are still available within the build tree but not to downstream users thus making this an API breaking change when compared to the 2.x releases. To keep things consistent, usage of the sfml- targets were replaced with their namespaced counterparts.

This has a number of benefits:

  1. It's more idiomatic. Modern CMake libraries are expected to
    have namespaced targets.

  2. Namespaces targets are less likely to collide with user-defined
    targets. No one will accidentally define a SFML:: target.

This PR is related to the issue #1562

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

How to test this PR?

Build and install SFML. Find and use the package in the following way:

find_package(SFML 3.0.0 REQUIRED COMPONENTS graphics)
...
target_link_libraries(some-target PRIVATE SFML::graphics)

@eXpl0it3r eXpl0it3r linked an issue Dec 30, 2021 that may be closed by this pull request
@eXpl0it3r eXpl0it3r added this to Backlog in SFML 3.0.0 via automation Dec 30, 2021
@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Dec 30, 2021
@eXpl0it3r eXpl0it3r moved this from Backlog to Clean Up Changes in SFML 3.0.0 Dec 30, 2021
@Bromeon
Copy link
Member

Bromeon commented Dec 31, 2021

Should we not use this chance and make module names consistent everywhere, i.e. PascalCase?

We have #include <SFML/Graphics.hpp>, so SFML::Graphics would be standing to reason in my opinion.

@ChrisThrasher
Copy link
Member Author

That's fine by me. I'll make the change as soon as the team comes to a consensus.

@ChrisThrasher
Copy link
Member Author

ChrisThrasher commented Dec 31, 2021

If we move to a SFML::Graphics style do we still want to keep the internal target names like sfml-graphics? That can be done but will require some find-replace string operation in sfml_add_library to automatically capitalize the first letter. sfml-Graphics is a bit unsightly so figuring out that string manipulation is probably worth it.

We also should tweak things such that the following is how clients find SFML package components

find_package(SFML 3.0.0 REQUIRED COMPONENTS Graphics)

with a capital "G" to further keep things consistent.

@Bromeon
Copy link
Member

Bromeon commented Jan 1, 2022

If the user-facing names are consistent (i.e. Graphics), that's already a good start.

What came to my mind now, are the output libraries/binaries, which are probably still sfml-graphics.{dll|so}. I guess that could stay; other namings like SFML-Graphics or SFML_Graphics are rather weird for those.

@ChrisThrasher ChrisThrasher force-pushed the namespaced-targets branch 4 times, most recently from a7f98c6 to 4736f03 Compare January 7, 2022 20:16
@ChrisThrasher
Copy link
Member Author

I think I figured out a good way to keep the existing sfml-graphics style targets while adding SFML::Graphics style targets for export and use by downstream targets. I feel good about this implementation.

@ChrisThrasher ChrisThrasher force-pushed the namespaced-targets branch 2 times, most recently from 3ff0f33 to 9caf8bc Compare January 8, 2022 00:09
@ChrisThrasher
Copy link
Member Author

I updated the component names so instead of writing

find_package(SFML 3.0.0 REQUIRED COMPONENTS graphics audio)

you write

find_package(SFML 3.0.0 REQUIRED COMPONENTS Graphics Audio)

@eXpl0it3r
Copy link
Member

As a German-speaking person I've noticed a major bias towards capitalizing things, so things like that make it hard for me to judge what's better. 😄

I updated the component names so instead of writing

find_package(SFML 3.0.0 REQUIRED COMPONENTS graphics audio)

you write

find_package(SFML 3.0.0 REQUIRED COMPONENTS Graphics Audio)

Does that mean the lower-case variant will no longer be working?

@ChrisThrasher
Copy link
Member Author

Does that mean the lower-case variant will no longer be working?

That’s right. I believe it’s theoretically possible to support both upper and lower case component names but that would be difficult to implement, especially if you wanted graphics and Graphics to be mutually exclusive. Currently it looks like this when users want to depend on SFML

find_package(SFML 3.0.0 REQUIRED COMPONENTS Graphics)
…
target_link_libraries(app PRIVATE SFML::Graphics)
#include <SFML/Graphics.hpp>

While I don’t feel strong about what is or isn’t capitalized, it does look nice that everything follows this “SFML” + “Graphics” pattern in all places.

cmake/Macros.cmake Outdated Show resolved Hide resolved
@ChrisThrasher ChrisThrasher force-pushed the namespaced-targets branch 2 times, most recently from ac21c8d to 33e556c Compare January 11, 2022 18:55
This removes the sfml- prefixed targets from the export set. The sfml-
prefixed targets are still available within the build tree but not to
downstream users thus making this an API breaking change when compared
to the 2.x releases. To keep things consistent, usage of the sfml-
targets were replaced with their namespaced counterparts.

This has a number of benefits:

  1. It's more idiomatic. Modern CMake libraries are expected to
     have namespaced targets.

  2. Namespaced targets are less likely to collide with user-defined
     targets. No one will accidentally define a SFML:: target.

  3. If a namespaced target is not found by CMake, configuration
     will immediately stop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
SFML 3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Add aliases for CMake targets
3 participants