-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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 CMake configuration #3027
base: master
Are you sure you want to change the base?
Add CMake configuration #3027
Conversation
c8bce83
to
3a99ca1
Compare
Any guidance on how to test Marmalade apps would be appreciated. I'd be happy to add the configuration for them. |
81253b2
to
920e7c7
Compare
Hey i like clarity of CMake code in this PR. Great job 👍 I have a few commends:
I think it would be very convenient if we could have all of cmake scripts in one file. I know this is not exactly conventional in CMake projects. Reason i am suggesting it is because Dear ImGui is a multi-buildsystem project and CMake support would be an addon instead of a main build system. Example targets could be created by one-liner if we made a macro for it. Defining most examples is almost same. Third party dependencies could use some touching up. For example if
if (NOT DEFINED IMGUI_SDL_TARGET)
find_package(SDL2 QUIET)
if (TARGET SDL2::SDL2)
# Some platforms (Linux) have SDL target properly exported as CMake target.
set (IMGUI_SDL_TARGET SDL2::SDL2)
elseif (SDL2_FOUND)
# Platforms that do not export target but use old CMake conventions can be handled this way.
add_library(SDL2::SDL2 INTERFACE IMPORTED)
target_link_libraries(SDL2::SDL2 INTERFACE ${SDL2_LIBRARIES})
target_include_directories(SDL2::SDL2 INTERFACE ${SDL2_INCLUDE_DIRS})
set (IMGUI_SDL_TARGET SDL2::SDL2)
elseif (NOT "$ENV{SDL2_DIR}" STREQUAL "")
# On windows we may set SDL2_DIR environment variable and link to binary SDL distribution.
add_library(SDL2::SDL2 INTERFACE IMPORTED)
set_target_properties(SDL2::SDL2 PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES $ENV{SDL2_DIR}/include
INTERFACE_LINK_LIBRARIES "$ENV{SDL2_DIR}/lib/${IMGUI_PLATFORM_ARCH}/SDL2.lib;$ENV{SDL2_DIR}/lib/${IMGUI_PLATFORM_ARCH}/SDL2main.lib"
)
set (IMGUI_SDL_TARGET SDL2::SDL2)
add_custom_target(CopySDL ALL COMMAND ${CMAKE_COMMAND} -E copy
"$ENV{SDL2_DIR}/lib/${IMGUI_PLATFORM_ARCH}/SDL2.dll" "${IMGUI_OUTPUT_DIR}/$<CONFIG>/SDL2.dll")
add_dependencies(${IMGUI_SDL_TARGET} CopySDL)
elseif (PkgConfig_FOUND)
# Rest of the platforms (like MacOS) can consume SDL via pkg-config.
pkg_check_modules(SDL2 QUIET IMPORTED_TARGET sdl2)
if (SDL2_FOUND)
set (IMGUI_SDL_TARGET PkgConfig::sdl2)
endif ()
endif ()
endif () This is not perfect, but could serve as a starting point. My GLFW bit: if (EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/../glfw/CMakeLists.txt)
# When parent directory contains GLFW source code - we can build it directly.
set (GLFW_STANDALONE OFF)
set (GLFW_INSTALL OFF)
set (GLFW_BUILD_DOCS OFF)
add_subdirectory (${CMAKE_CURRENT_SOURCE_DIR}/../glfw ${CMAKE_CURRENT_BINARY_DIR}/glfw)
set (IMGUI_GLFW_TARGET glfw)
elseif (MSVC)
# We ship compiled libraries in our repository for Visual Studio.
add_library(glfw INTERFACE IMPORTED)
target_link_libraries(glfw INTERFACE libs/glfw/lib-vc2010-${IMGUI_PLATFORM_BITS}/glfw3.lib)
target_include_directories(glfw INTERFACE libs/glfw/include)
set (IMGUI_GLFW_TARGET glfw)
elseif (PkgConfig_FOUND)
pkg_check_modules(GLFW QUIET IMPORTED_TARGET glfw3)
if (GLFW_FOUND)
set (IMGUI_GLFW_TARGET PkgConfig::glfw)
endif ()
endif () |
You can ignore this, I am going to scrap that backend and move it elsewhere eventually. |
Hey @rokups, thanks for the feedback!
Yes - using the
I'll defer to @ocornut here, but I will say this: it is indeed very unconventional, and it's going to hurt maintainability in the long-term. I highly recommend against it.
I didn't know
Definitely; this is how I intend to use ImGui in my projects as well. I still need to test, but I've written the configuration with this in mind, and I believe the current configuration allows for this. If not,
I'll give it a shot, thanks :)
Sounds good; what about allegro? I'm not familiar with that either. |
@rokups your glfw pkgconfig code seems to fail for me:
|
Imported library description does not mention this property. I guess invisibility is a byproduct of library not producing any output. And they arent object libraries as object libraries are ones that compile source files but do not link them into libraries/exes. They just inject extra includes/libs/flags to linking target. But we can exploit them to also inject source files to linking targets by using
Could maybe CMake version being rather old? See https://cmake.org/cmake/help/v3.17/module/FindPkgConfig.html?highlight=pkg%20config#module:FindPkgConfig for your version of CMake. |
This seems strange. Targets shouldn't be able to conflict - CMake wouldn't know what to do. I think there's a missing piece of information somewhere here 😅 In your opinion, what are the pros/cons of using import libraries vs. object libraries?
|
@podsvirov I didn't add it since it's not an EDIT: nevermind, I see what you mean. Added :) |
@Qix-, any efforts to export your configuration for others and install examples? |
PS: my acount is @podsvirov :-). Dear @podgorskiy, sorry for noise. |
Whoops, sorry about that. Don't know why github did that. 😅 (EDIT: Oh I see he's also active around here; yes, sorry for the noise indeed!) What do you mean by exporting configuration? As far as installation, I don't know that examples really need to have install targets. |
Yes, we can simply built examples, but then we simply build only examples... |
I don't understand what you mean by "package". Packaging what, and for whom? ImGui examples aren't going to be used by anyone outside the project, so there's no point in having an install target for them. What's the use-case? I'm pretty reluctant on adding anything that doesn't need to be added. |
What is holding back the vulkan stuff? I see the cmake files in the commit. |
@Cazadorro I only have a mac machine and (correct me if I'm wrong) ImGui doesn't have a MoltenVK sample (I've never worked with moltenvk personally). Further, you can't test Vulkan in virtual machines yet (at least, VMWare doesn't support it). Just compilation, but that doesn't tell me if it actually runs or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This CMake implementation looks pretty good and looks much easier to maintain and understand than what was implemented in #1713.
@Qix-, have you tried to build everything with BUILD_SHARED_LIBS=ON? I think that we'd need more CMake code to properly handle shared libs, especially on Windows.
FYI, @ocornut, if this gets merged, I volunteer to maintain and improve this, as having CMake build would be quite beneficial for me and for other users who want a simple multiplatform build of ImGui. (Also would be good to have for ImGui-SFML and possibly other backends)
add_library (imgui | ||
imgui.cpp | ||
imgui_draw.cpp | ||
imgui_widgets.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this also needs imgui_tables.cpp now
cmake_minimum_required (VERSION 3.8.2) | ||
project(imgui VERSION 1.73.0 LANGUAGES CXX C) | ||
|
||
option (IMGUI_EXAMPLES "Build ImGui examples" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and everywhere else the space before brace is not idiomatic. Ideally it would be good to follow the style used in CMake docs and not opinionated style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space before brace is not idiomatic
There is no "idiom" when it comes to whitespace in CMake files - I personally prefer the space as it makes things easier to read in all of the projects I create, hence why they're here. The whitespace makes no difference, and since there isn't a CMake linter it's hard to enforce things either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CMake scripts will be maintained for years to come by many people. I think it's easier to show people CMake docs as an example of how to write scripts than choose an arbitrary, subjective style and allow inconsistencies which will be introduced in future PRs (the spaces are missing already in some lines of the code in your PR, for example).
@@ -0,0 +1,14 @@ | |||
if (NOT TARGET glfw) | |||
find_package (GLFW3 QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is QUIET
specified here? It would be good to fail here loudly instead of failing quietly and user scratching their head because the example is not built for some reason.
Same applies to all find_package calls in this implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look a bit further down. We craft our own error message based on whether or not glfw
was brought in as a CMake submodule or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then I have some further questions.
- Maybe the message shouldn't be a warning, but an error instead? The target fails to build because the dependency is not found, it's an error in my opinion
- Maybe glfw_FOUND variable (set by find_package) should be used instead of the current check for target's existence and GLFW3_DIR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this PR is closed now anyway, but maybe one of you still has an optinion on this that [s]he cares to elaborate, because I use similar mechanisms in other projects:
Shouldn't the order be the opposite? First check, if the target is already available (e.g. through add_subdirectory) and only if not, fall back to find_package. Assuming a library has the ability to use a local version of a dependency at all, I'd be extremely suprised, if if that local version got replaced, just because for some completely unrelated reason someone installed the dependency system wiede via a package manager.
add_subdirectory (examples/example_win32_directx10) | ||
add_subdirectory (examples/example_win32_directx11) | ||
add_subdirectory (examples/example_win32_directx12) | ||
add_subdirectory (examples/example_win32_directx9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of many add_subdirectory calls here, we can move them to "examples/CMakeLists.txt"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall there being a reason I didn't do that initially, but otherwise I agree.
|
||
add_library(imgui::imgui ALIAS imgui) | ||
|
||
target_include_directories (imgui PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You better add variables for add_subdirectory
and install
usage.
target_include_directories(imgui PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> $<INSTALL_INTERFACE:include>)
Furthermore, it is a good practice to store headers in include/lib_name
folder. By including the whole source dir you pollute includes. Using a separate include/imgui
folder for headers will make it clear for library users to understand which library the included header belongs to:
#include <imgui/imgui.h>
https://cliutils.gitlab.io/modern-cmake/chapters/basics/structure.html - a good reference for modern cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware of how good projects are structured. As I've stated elsewhere in this PR, this was modeled to match the existing build system and layout.
To be honest this isn't something I see being merged as there isn't a huge push from the maintainer, which I understand. There are also a lot of less-than-productive remarks made and it's eating a lot of my time. CMake has too many ways to do the same thing and it appears nobody can agree on which way is best (depends on the person, depends on the month) so I encourage those who care enough to implement it how they like instead :) |
That is very sad, since imgui seems to be a very simple and convenient thing to use and deploy. However, it is not easily integrated, which prevents its usage in the first place, since there is nanogui, for example. And it does provide some convenient (not ideal) means for integration to the project. A satisfactory package implementation is the one that provides cmake targets with consistent naming; can be seamlessly integrated both via subdirectory and find_package; can be acquired via a packet manager (apt, pacman, etc) and by default searches for cmake dependencies during configuration (I'm talking mostly about large 3rdparty dependencies which have a chance of being already installed). |
I think this might have more chances if people implement CMakeLists.txt for the main target first (the one which will include imgui.cpp, imgui_widgets.cpp, etc.-etc.) and then implement CMake builds for each backend/impl one by one. Reviewing tons of lines of code in one PR is not great. |
This is not that simple. Core imgui target is useless without backends. And the project itself requires a major restructuring, thus a thorough understanding how the things work... I doubt it is possible to introduce changes gradually when writing CMake support in this case. |
It has been done. Using best ideas from this PR, in a least intrusive way possible, not stepping on toes of people who have no interest in using CMake. https://gist.github.com/rokups/f771217b2d530d170db5cb1e08e9a8f4 |
@Qix- FYI bluntly speaking on my end I haven't had time to investigate this at all, past quick skimming. But must say I appreciate the work done here and the simplicity of the cmakefile itself. So first of all, thank you. Lack of interaction on my part reflects a lack of priority and generally being overwhelmed with six hundred other tasks, but it doesn't mean I wouldn't want this solved. Intuitively my conclusion was similar to yours, when you had just opened the PR I was very optimistic about it and the more debate and comments happening, the less optimistic I become about it, since it was a sort of give away of the "CMake has too many ways to do the same thing and it appears nobody can agree on which way is best (depends on the person, depends on the month)" feeling you expressed. If it was up to me I would prefer to keep this PR open, even if the PR state is going to last probably longer, it's a healthy reminder this is an open problem. From my POV we have not yet solved this problem, and this PR along with #1713 are both:
May I ask you to not delete your branch? When a branch gets deleted Github awkwardly replace with a patch which contains the same stuff but also somehow its harder to browse and work with. Thank you again. |
c++ in general has the exact same problem. The solution is usually to not let the perfect become the enemy of the good and work iteratively - especially in the early phase. Start with a simple version, that covers just a few core scenarious and make sure you are reasonably happy with the way it can be used (a.k.a. the interface). Then iteratively improve it based on user feedback. Also consider that breaking changes to the build system interface are usually much less disruptive than changes to the library itself, because there is "usually" only 1 or at most a handful of locations in the user project, where that interface is used. So there is less reason to be affraid to get stuck with a "bad" interface if the initial design is not ideal. I guess what I'm trying to say: Assuming permission from @qix, you should probably just merge these changes and see what further issues / PRs pop up instead of delaying this indefinetly until no one feels there is even a point in working on cmake support at all. |
0c1e5bd
to
bb6a60b
Compare
8b83e0a
to
d735066
Compare
b3b85d8
to
0755767
Compare
c817acb
to
8d39063
Compare
Supersedes #1713. This is a much simpler configuration for use with CMake projects.
Provided here as suggested in #1713 (comment).
Platforms
General TODO's
Examples
example_allegro5
example_apple_metal
example_apple_opengl2
example_emscripten
example_glfw_metal
example_glfw_opengl2
example_glfw_opengl3
example_glfw_vulkan
example_glut_opengl2
example_null
example_sdl_directx11
example_sdl_metal
example_sdl_opengl2
example_sdl_opengl3
example_sdl_vulkan
example_win32_directx10
example_win32_directx11
example_win32_directx12
example_win32_directx9
Implementations
Let me know if I'm missing anything.