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

Only link sfml-main for the GUI examples in release mode. #766

Merged
merged 1 commit into from Mar 28, 2015

Conversation

Projects
None yet
4 participants
@eXpl0it3r
Member

eXpl0it3r commented Jan 7, 2015

Fixes #610

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 7, 2015

Member

👎

This won't work for multi-configuration CMake generators (Visual Studio, XCode, KDevelop, ...), where ${CMAKE_BUILD_TYPE} is irrelevant.

There doesn't seem to be a general solution to this problem though. For example, this solution is rather ugly and for Visual Studio only.

Member

LaurentGomila commented Jan 7, 2015

👎

This won't work for multi-configuration CMake generators (Visual Studio, XCode, KDevelop, ...), where ${CMAKE_BUILD_TYPE} is irrelevant.

There doesn't seem to be a general solution to this problem though. For example, this solution is rather ugly and for Visual Studio only.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 7, 2015

Member

True, so should it not link against sfml-main in every case?

Member

eXpl0it3r commented Jan 7, 2015

True, so should it not link against sfml-main in every case?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 7, 2015

Member

That, or we apply this PR anyway:

  • for official SFML releases we only use makefile generators, so this issue doesn't apply for us
  • for user builds, multi-configuration generators will produce examples with a console even in release build, is it a problem?

If we apply the PR as it is, we should at least add a comment such as "won't be applied in multi-configuration generators such as Visual Studio, but we don't care".

Member

LaurentGomila commented Jan 7, 2015

That, or we apply this PR anyway:

  • for official SFML releases we only use makefile generators, so this issue doesn't apply for us
  • for user builds, multi-configuration generators will produce examples with a console even in release build, is it a problem?

If we apply the PR as it is, we should at least add a comment such as "won't be applied in multi-configuration generators such as Visual Studio, but we don't care".

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 7, 2015

Member

for user builds, multi-configuration generators will produce examples with a console even in release build, is it a problem?

Since it's using NOT xyz STREQUAL "Release" it will link against sfml-main with multi-configuration generators.
While thinking about it, I also noticed that in debug we theoretically would had to link sfml-main-d.

Member

eXpl0it3r commented Jan 7, 2015

for user builds, multi-configuration generators will produce examples with a console even in release build, is it a problem?

Since it's using NOT xyz STREQUAL "Release" it will link against sfml-main with multi-configuration generators.
While thinking about it, I also noticed that in debug we theoretically would had to link sfml-main-d.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 7, 2015

Member

I'm quite sure CMake introduced something to fix that long standing issue with CMAKE_BUILD_TYPE, just not sure which version. I think with 3.0...

Member

MarioLiebisch commented Jan 7, 2015

I'm quite sure CMake introduced something to fix that long standing issue with CMAKE_BUILD_TYPE, just not sure which version. I think with 3.0...

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 7, 2015

Member

Are you thinking of Generator expressions? I don't think they can solve this particular issue.

Member

LaurentGomila commented Jan 7, 2015

Are you thinking of Generator expressions? I don't think they can solve this particular issue.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 7, 2015

Member

Not sure, I thought it'd been something different, but can't find anything related. Those might be able to solve this though.

Member

MarioLiebisch commented Jan 7, 2015

Not sure, I thought it'd been something different, but can't find anything related. Those might be able to solve this though.

@eXpl0it3r eXpl0it3r removed the s:unassigned label Jan 8, 2015

@eXpl0it3r eXpl0it3r self-assigned this Jan 9, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 9, 2015

Member

Just noticed that I saw things wrong, because besides CMAKE_BUILD_TYPE there's also the CMAKE_CONFIGURATION_TYPES.

The current issue I have with Visual Studio though is, that the win32 example uses WinMain as entry point, but we don't set the subsystem to Window. How would we go about fixing this?

Member

eXpl0it3r commented Jan 9, 2015

Just noticed that I saw things wrong, because besides CMAKE_BUILD_TYPE there's also the CMAKE_CONFIGURATION_TYPES.

The current issue I have with Visual Studio though is, that the win32 example uses WinMain as entry point, but we don't set the subsystem to Window. How would we go about fixing this?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 9, 2015

Member

The single argument that is not ignored, HINSTANCE instance, can be retrieved with a call to GetModuleHandle(NULL). So we can safely replace WinMain by main.

Member

LaurentGomila commented Jan 9, 2015

The single argument that is not ignored, HINSTANCE instance, can be retrieved with a call to GetModuleHandle(NULL). So we can safely replace WinMain by main.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 9, 2015

Member

Since it's a Windows example, do we really want to go with main() and GetModuleHandle(NULL)?

Member

eXpl0it3r commented Jan 9, 2015

Since it's a Windows example, do we really want to go with main() and GetModuleHandle(NULL)?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 9, 2015

Member

If you don't want to use the WIN32 CMake flag, but yet use WinMain as the entry point, you'll have to set compiler specific linker settings. Do you really want that?

Member

LaurentGomila commented Jan 9, 2015

If you don't want to use the WIN32 CMake flag, but yet use WinMain as the entry point, you'll have to set compiler specific linker settings. Do you really want that?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 7, 2015

Member

Wouldn't just using

target_link_libraries(${target} optimized sfml-main)

work?

Member

binary1248 commented Feb 7, 2015

Wouldn't just using

target_link_libraries(${target} optimized sfml-main)

work?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 11, 2015

Member

@binary1248 The line above the linking would also need to be applied only for the optimized target (i.e. WIN32).

I've updated the PR and it now only removes the console window if there are no multiple configuration types and it's built in release mode.

Additionally the win32 example has been updated.

Member

eXpl0it3r commented Feb 11, 2015

@binary1248 The line above the linking would also need to be applied only for the optimized target (i.e. WIN32).

I've updated the PR and it now only removes the console window if there are no multiple configuration types and it's built in release mode.

Additionally the win32 example has been updated.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 11, 2015

Member

Well... yeah... I didn't say that it would solve the WIN32 issue as well. There will have to be another solution for that.

Member

binary1248 commented Feb 11, 2015

Well... yeah... I didn't say that it would solve the WIN32 issue as well. There will have to be another solution for that.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 23, 2015

Member

An further comments on this?

Member

eXpl0it3r commented Mar 23, 2015

An further comments on this?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 23, 2015

Member

I'd tend to split the examples completely, i.e. provide a standalone CMakeLists.txt for them (would act as another part of the example) and exclude them from building the libraries themselves.

Member

MarioLiebisch commented Mar 23, 2015

I'd tend to split the examples completely, i.e. provide a standalone CMakeLists.txt for them (would act as another part of the example) and exclude them from building the libraries themselves.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 23, 2015

Member

That's what #800 is for or any other PR, this is about linking sfml-main, so let's focus on that.

Member

eXpl0it3r commented Mar 23, 2015

That's what #800 is for or any other PR, this is about linking sfml-main, so let's focus on that.

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Mar 26, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 26, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 26, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit 23cc8cf into master Mar 28, 2015

@eXpl0it3r eXpl0it3r deleted the feature/examples_sfml-main branch Mar 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment