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

Added CMake variables to select the modules to be built #800

Merged
merged 1 commit into from Apr 25, 2017

Conversation

MarioLiebisch
Copy link
Member

This addresses issue #798.

The only leftover issue I see is the fact that on most platforms we're always installing most third party dependencies, even if the modules aren't built.

CMakeLists.txt Outdated
@@ -44,11 +44,25 @@ else()
set(SFML_BUILD_EXAMPLES FALSE)
endif()

# add options to select which modules to build
sfml_set_option(SFML_BUILD_SYSTEM TRUE BOOL "TRUE to build SFML's System module. This setting is ignored, if any of the other modules is built.")
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of not building sfml-system? I think we can remove this option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe building documentation only? Wasn't sure either.

Copy link
Member

Choose a reason for hiding this comment

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

We already can build only the doc with make doc. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It would still want dependencies and you couldn't install the docs only. But if you think we don't need that option, can cut it as well (same with the main one).

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I forget the issue at hand, sorry.

But wouldn't someone who's building the doc want to build at least one module of SFML?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's quite an edge case.

@LaurentGomila
Copy link
Member

What's the problem with external libs? Your PR seems to handle them correctly.

@MarioLiebisch
Copy link
Member Author

No problem in general, but let's assume you want to install system and network only (server), you'd still install all extlibs (i.e. on Windows). Guess I'll just update that part of the CMake script.

@eXpl0it3r
Copy link
Member

you'd still install all extlibs (i.e. on Windows)

Did you test this? Afaik the extlibs have to be explicitly added to get installed so when you just pick one module it won't added the other ones, but maybe my memory serves me wrong.

@MarioLiebisch
Copy link
Member Author

Right now you install the whole directory since the file names are toolchain dependent.

@MarioLiebisch
Copy link
Member Author

So, any further comments regarding this? Especially related to whether to include SFML_BUILD_SYSTEM or ommit it.

@mantognini
Copy link
Member

I also believe we don't need SFML_BUILD_SYSTEM. Beside that, this PR's changes will conflict with change with no_libsndfile but that's not a big deal.

@binary1248
Copy link
Member

This PR needs rebasing now that no_libsndfile is merged.

@MarioLiebisch
Copy link
Member Author

Completely forgot about this; the branch is rebased and up to date again. Still not 100% sold regarding SFML_BUILD_SYSTEM though.

@Bromeon
Copy link
Member

Bromeon commented Mar 23, 2015

I would remove the option SFML_BUILD_SYSTEM, too. It's not really useful -- you just add confusion and possible error sources with more options. For example, you would then need to do all kinds of checks like "you can't use sfml-audio without sfml-system".

@MarioLiebisch
Copy link
Member Author

For example, you would then need to do all kinds of checks like "you can't use sfml-audio without sfml-system".

To prevent this, dependencies are always built, so even with SFML_BUILD_SYSTEM set to false it's always built in case you try to build any of the other libraries.

But yeah, guess I'll just drop it then, unless there's some other opinion till tomorrow.

@Bromeon
Copy link
Member

Bromeon commented Mar 23, 2015

To prevent this, dependencies are always built, so even with SFML_BUILD_SYSTEM set to false it's always built in case you try to build any of the other libraries.

I personally don't like this way of hiding user errors. If the user ticks sfml-graphics but not sfml-system, he probably has a misconception of how the dependencies work -- and I'd rather tell him than just assume he didn't mean it.

Same with sfml-graphics and sfml-window at the moment, I would probably add an error message...

@MarioLiebisch
Copy link
Member Author

True, there should be a warning, if it's possible to exclude dependencies, which are still built.

@mantognini
Copy link
Member

So what's the status of this?

In any case, those lines need to be updated.

@MarioLiebisch
Copy link
Member Author

Can have a look later this week; caught a cold. :(

@mantognini
Copy link
Member

Before I forget: the framework part for OS X also need some update. See https://github.com/SFML/SFML/pull/933/files#diff-af3b638bc2a3e6c650974192a53c7291R272 (not tested)

@MarioLiebisch
Copy link
Member Author

So we have any current open PR or changes that would conflict this? I'd rebase it, but don't want to do the work twice. :)

@Bromeon
Copy link
Member

Bromeon commented Jan 1, 2016

Are we going to include this in SFML 2.4? If so, it would be nice to perform the adaptations mentioned above.

@eXpl0it3r
Copy link
Member

@MarioLiebisch Can you rebase this and include the possible changes that @mantognini mentioned?

@MarioLiebisch MarioLiebisch force-pushed the feature/selective_building branch 2 times, most recently from fa0678b to 40cb8cd Compare November 5, 2016 12:33
@MarioLiebisch
Copy link
Member Author

Done.

Copy link
Member

@mantognini mantognini left a comment

Choose a reason for hiding this comment

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

This patch chunk should be applied as well.

Also, we could add an "assert" to make sure SFML_BUILD_WINDOW is on when SFML_BUILD_GRAPHICS is true.

CMakeLists.txt Outdated
@@ -329,31 +339,33 @@ if(SFML_OS_WINDOWS)
elseif(SFML_OS_MACOSX)

# install extlibs dependencies only when used
if("${FLAC_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/FLAC.framework")
if(SFML_BUILD_AUDIO AND FLAC_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/FLAC.framework")
Copy link
Member

Choose a reason for hiding this comment

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

Audio dependencies should be moved together in a single if(SFML_BUILD_AUDIO) block. This includes Flac,

CMakeLists.txt Outdated
endif()

if("${OGG_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/ogg.framework")
if(OGG_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/ogg.framework")
Copy link
Member

Choose a reason for hiding this comment

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

ogg,

CMakeLists.txt Outdated
install(DIRECTORY extlibs/libs-osx/Frameworks/ogg.framework DESTINATION ${CMAKE_INSTALL_FRAMEWORK_PREFIX})
endif()

if("${VORBIS_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbis.framework")
if(VORBIS_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbis.framework")
Copy link
Member

Choose a reason for hiding this comment

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

vorbis,

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, although I think these should ultimatively moved to the modules' CMakeLists.txt rather than doing it globally here.

CMakeLists.txt Outdated
install(DIRECTORY extlibs/libs-osx/Frameworks/vorbis.framework DESTINATION ${CMAKE_INSTALL_FRAMEWORK_PREFIX})
endif()

if("${VORBISENC_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbisenc.framework")
if(VORBISENC_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbisenc.framework")
Copy link
Member

Choose a reason for hiding this comment

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

vorbisenc,

CMakeLists.txt Outdated
install(DIRECTORY extlibs/libs-osx/Frameworks/vorbisenc.framework DESTINATION ${CMAKE_INSTALL_FRAMEWORK_PREFIX})
endif()

if("${VORBISFILE_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbisfile.framework")
if(VORBISFILE_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/vorbisfile.framework")
Copy link
Member

Choose a reason for hiding this comment

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

vorbisfile,

CMakeLists.txt Outdated
install(DIRECTORY extlibs/libs-osx/Frameworks/vorbisfile.framework DESTINATION ${CMAKE_INSTALL_FRAMEWORK_PREFIX})
endif()

if("${OPENAL_LIBRARY}" STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/OpenAL.framework")
if(OPENAL_LIBRARY STREQUAL "${SFML_SOURCE_DIR}/extlibs/libs-osx/Frameworks/OpenAL.framework")
Copy link
Member

Choose a reason for hiding this comment

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

and OpenAL.

@mantognini
Copy link
Member

Thanks for the rebase BTW. 👍

@MarioLiebisch MarioLiebisch force-pushed the feature/selective_building branch 2 times, most recently from 4d2c9ef to e83a01d Compare November 8, 2016 09:31
@eXpl0it3r
Copy link
Member

Looks like this PR was a bit forgotten. Can you give this a rebase?

@eXpl0it3r eXpl0it3r moved this from WIP to Review & Testing in SFML 2.5.0 Feb 28, 2017
@MarioLiebisch MarioLiebisch force-pushed the feature/selective_building branch 2 times, most recently from 3a9ee64 to 1938ffc Compare February 28, 2017 08:20
@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Mar 13, 2017
@eXpl0it3r
Copy link
Member

Nearly feel bad for asking again, can you give this one last rebase? 😄

@eXpl0it3r eXpl0it3r merged commit 0b2ac85 into master Apr 25, 2017
@eXpl0it3r eXpl0it3r deleted the feature/selective_building branch April 25, 2017 12:48
@eXpl0it3r eXpl0it3r moved this from Ready to Merged in SFML 2.5.0 Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

6 participants