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

Improve HELLOIMGUI_WIN32_NO_CONSOLE implementation #99

Closed
xuboying opened this issue Mar 6, 2024 · 6 comments · Fixed by #103
Closed

Improve HELLOIMGUI_WIN32_NO_CONSOLE implementation #99

xuboying opened this issue Mar 6, 2024 · 6 comments · Fixed by #103

Comments

@xuboying
Copy link

xuboying commented Mar 6, 2024

Version: 0576001
Platform: Win10 + MSVC + clang tool chain
File: hello_imgui_add_app.cmake

Current implantation manually adds link option for MINGW/non-MINGW target which is not necessary, it's also causing a bug for MSVC + clang tool chain as clang could not understand "/subsystem:WINDOWS".

The fix is very simple, CMake supports WIN32 flag in add_executable (https://cmake.org/cmake/help/v3.10/command/add_executable.html)
Just PREPAND the option to app_sources before add_executable.
BTW, it's also possible to APPEND HelloImGui_WinMain.cpp to app_sources before add_excutable so the entire fix could be very clean:

    #############################################################################
    # If windows, and if the user wants to, we can make this an app without console
    # and provide a WinMain entry point
    if (WIN32)
        if (HELLOIMGUI_WIN32_NO_CONSOLE)
            # Make this an app without console, and use HelloImGui_WinMain.cpp
            list(PREPEND app_sources WIN32)
        endif()
        if (HELLOIMGUI_WIN32_AUTO_WINMAIN)
            list(APPEND app_sources ${HELLOIMGUI_CMAKE_PATH}/HelloImGui_WinMain.cpp)
        endif()
    endif()

    if (ANDROID)
        add_library(${app_name} SHARED ${app_sources})
    else()
        add_executable(${app_name} ${app_sources})
    endif()

    hello_imgui_prepare_app(${app_name} ${assets_location})
@pthom
Copy link
Owner

pthom commented Mar 6, 2024

Hello,

Thanks for your suggestion! Would you want to provide a PR for this, so that you are cited in the contributors?

Also, I guess you tried this modification with clang. Can you confirm this, and tell me if you had time to also test it with msvc and/or mingw?

@xuboying
Copy link
Author

xuboying commented Mar 6, 2024

sure, I will try all your suggestions

@xuboying
Copy link
Author

Hi again
I tried the MSVC and MSVC with Clang tool chain on Win10 VS Code, they are fine.

MSVC version:
[proc] Executing command: C:\usr\cli\cmake\bin\cmake.EXE --no-warn-unused-cli -Wno-dev -DCMAKE_BUILD_TYPE:STRING=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE -SC:/repos/hello_imgui -Bc:/repos/hello_imgui/build/Debug -G "Visual Studio 17 2022" -T host=x64 -A x64
[cmake] Not searching for unused variables given on the command line.
[cmake] -- Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.19045.
[cmake] -- The C compiler identification is MSVC 19.38.33130.0
[cmake] -- The CXX compiler identification is MSVC 19.38.33130.0

I would like to try Mingw, however I have no experience on that.
I encounter several new issues like cmake platform unknown issue, Makefile multiple target bug(caused by windows driver) , and finally stopped at opengl32 linking problem. I also realized there are many variants of mingw, making troubleshooting ever harder.

So, is it possible to simply testing the mingw result in your CI pipeline?

I have pushed change to my fork:
xuboying@5dce5cc

@pthom
Copy link
Owner

pthom commented Mar 13, 2024

You are right, testing on the multiple platform / compilers is one of the main difficult things in this project.

So, is it possible to simply testing the mingw result in your CI pipeline?

Yes, please submit a PR, and I'll run the pipeline on it. If it does not work on MingW, I'll diagnose it on my side. You did the test I could not do (i.e. with MSVC + clang).

Thanks!

@xuboying
Copy link
Author

Thanks!

@pthom
Copy link
Owner

pthom commented Mar 13, 2024

The mingw pipeline passed. Many thanks for your contribution!

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 a pull request may close this issue.

2 participants