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

servers: fix unicode support for mingw #4926

Merged
merged 1 commit into from May 13, 2020

Conversation

dyfer
Copy link
Member

@dyfer dyfer commented May 12, 2020

Purpose and Motivation

MinGW needs this flag since we are now using wmain() in servers.

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • This PR is ready for review

@dyfer dyfer added comp: build CMake build system os: Windows labels May 12, 2020
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -206,6 +206,7 @@ target_link_libraries(supernova libsupernova)

if(WIN32)
if(NOT MSVC)
SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -municode")
Copy link
Contributor

Choose a reason for hiding this comment

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

using SET in this way is very old-style cmake. i think you want target_link_libraries instead.

also, this will add this flag for MSYS builds too, do you know if that's a problem? i am not familiar enough with these systems to know but just thought i would ask. if you don't know, don't worry about it. @sonoro1234 may know?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont use MSYS just mingw-64 gcc compiler from windows command line!!
I guess @dyfer tested the flag already

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks! in that case don't worry about MSYS support @dyfer

@dyfer
Copy link
Member Author

dyfer commented May 13, 2020

Thanks for the note on target_link_libraries - I understand it better now.

I still need to test this after this change.

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks!!

@mossheim mossheim merged commit 57edf6c into supercollider:develop May 13, 2020
@mossheim
Copy link
Contributor

oh darn, sorry, i just saw "I still need to test this"... please do test this if you have time, we can always open a new PR if it didnt work.

@dyfer
Copy link
Member Author

dyfer commented May 13, 2020

tested - works :)

@mossheim
Copy link
Contributor

thank you!

@patrickdupuis patrickdupuis mentioned this pull request Jul 6, 2020
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
@patrickdupuis patrickdupuis moved this from in progress to DONE in Patch release cherry-picks Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build CMake build system os: Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants