-
Notifications
You must be signed in to change notification settings - Fork 557
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
CMake and CPack integration (removing Python dependency in the process) #462
base: master
Are you sure you want to change the base?
Conversation
…r-only and 'regular' library variants; fix (most) whitespace discrepancies betwixt Python and CMake amalgamation generation routines; begin implementing `fix_comments`
…Use same compiler flags as example Makefile (sans C99); [example/CMakeLists.txt] WiP implementation; [cmake/modules/CMakeFindM.cmake] Find math library
…package; [src/CMakeLists.txt] Fix `fix_comments` implementation; improve whitespace consistency with Python implementation
…ption; [demo/CMakeLists.txt] `add_subdirectory` for each demo (comment out all but `allegro5` for now); [demo/allegro5/CMakeLists.txt] Begin implementing allegro5 demo
This project focus on portability, efficiency and simplicity. There's no space for CMake |
@HelloWorldlsla My interest in nuklear is keenly cross-platform. Nuklear, IMHO, should be easy to build, test, and change. Its testing should be integrated with a CI/CD platform with status shown on GitHub (e.g, GitHub Actions, Cirrus CI, Travis CI, Circle CI, or other). Platforms that I'm keen to test [and if needed, implement] support for include:
Stretch goals:
CMake is merely one open-source cross-platform build system that works for all these platforms (or soon will, in the case of iOS). GNU Makefiles unfortunately are not portable. With a native first approach, which build system would you prefer using? - Maybe I can assist in contributing support for that one. |
"port-version": 1, | ||
"homepage": "https://github.com/Immediate-Mode-UI/Nuklear", | ||
"description": "Minimal state immediate mode graphical user interface toolkit written in ANSI C and licensed under public domain.", | ||
"dependencies": [ |
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.
Before going into the discussion about the general idea of getting rid of Python in favour of anything else (here cmake), I would like to point out that Nuklear has literally zero dependencies. It has only optional dependencies. So this is wrong. If vcpkg does not support optional dependencies, then we must not put any dependencies into the package description.
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.
Agreed. These dependencies should be defined in the demos rather than here 👍
@SamuelMarks cross-platform in case of single header libs (incl. Nuklear) means for me to have CI running on all imaginable platforms. Nothing more, nothing less. I would like to understand a little more your point of view on this. Could you elaborate a bit more why would you (pretty radically 😉) change the current state of things instead of just adding a few yaml/... file for CI to cover more cases? What was the motivation? Also what are the pros & cons of the approach you have chosen (compared to other approaches - at least to the current approach in this repo)? Thanks! |
@dumblob In terms of my [proposed] approach: Pros
Cons
|
Any progress? |
Thanks @SamuelMarks for the pointers! Let us discuss this to set a deeper mutual understanding.
Any real use cases? After those years with header-only libraries I could not find any real use case for making them a shared/static library.
Do I understand this correctly that this point solves only new problems originating from supporting the first point - namely building as shared/static lib?
But cmake is 😉. IMHO both (Python and CMake) are fine so I do not consider this point a pro nor a con.
This sounds interesting as I am not much experienced with modern IDEs. Do modern IDEs have issues with header-only libraries? Could you list those isusses? Any other approaches to solve them (and their pros & cons)? Or could you generally elaborate? Thanks!
If you compare the modularity you envision with the current modularity (i.e. source split into logical blocks mostly individually reusable as seen in https://github.com/Immediate-Mode-UI/Nuklear/tree/master/src ), could you list all the differences you see and why they are important (or alternatively the pros & cons of the current modularity)? Thanks!
Ok, but not a big issue IMHO 😉 (definitely not a reason to not merge the PR). Let me point out one more thing - I am pretty sure we can not maintain multiple build systems for the Thoughts? Btw. could you take a look at the deps in |
"nuklear_font.c" "nuklear_input.c" "nuklear_style.c" | ||
"nuklear_context.c" "nuklear_pool.c" "nuklear_page_element.c" | ||
"nuklear_table.c" "nuklear_panel.c" "nuklear_window.c" | ||
"nuklear_popup.c" "nuklear_contextual.c" "nuklear_menu.c" |
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 glob is frowned upon in CMake, but it may help ease the maintenance of the CMake definition files.
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 reason why glob is not recommended is because it is slower, and when there is a file added or removed you do not trigger a cmake rebuild.
Instead of using glob, I would suggest to format these lists differently (one file per line) so that editing the list results in cleaner diffs.
"allegro5" | ||
# "common" | ||
# "d3d11" | ||
# "d3d12" | ||
# "d3d9" | ||
# "gdi" |
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.
Possibly have these as individual options defaulting to if the platform dependencies are found?
option(BUILD_DEMOS "Build demos" "ON") | ||
option(BUILD_EXAMPLES "Build 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.
Could have the BUILD_DEMOS and BUILD_EXAMPLES options enabled only if the user is using CMake from root. That wasy it'll only build them if you're building locally rather than as a third-party dependdency.
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 can be accomplished with...
# Options
if ("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")
set(NUKLEAR_IS_MAIN TRUE)
else()
set(NUKLEAR_IS_MAIN FALSE)
endif()
option(BUILD_DEMOS "Build demos" ${NUKLEAR_IS_MAIN})
option(BUILD_EXAMPLES "Examples" ${NUKLEAR_IS_MAIN})
@@ -0,0 +1,105 @@ | |||
cmake_minimum_required(VERSION 3.15) | |||
|
|||
project(Nuklear VERSION 0.0.1 LANGUAGES C) |
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.
Since VERSION
won't match, possibly remove it?
project(Nuklear VERSION 0.0.1 LANGUAGES C) | |
project(Nuklear LANGUAGES C) |
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 could be made the primary place for the version?
Then it generates a header with the version in it, so that compile-time you have access to the same version.
Just a note that I'm slowly growing as a CMake fan, and found it does make a few things easier. Particularly when it comes to dependency management.
Building as a shared library does improve compilation performance, as you don't have to compile directly into your application. While the majority of cases are static, it doesn't hurt to have the option. Don't think we really need it right now though. It's definedin that
I think having both in the mean time is a good option. Just grows the potential contributor base.
Generally, IDEs don't have issues header-only libraries. The benefits in the IDEs with CMake comes in providing the build commands directly through the user interface. VSCode, for instance, will find the available build targets, and give you a one-click button to build the target automagically.
One thing we could do is limiting the places where we throw the CMake definition files. Keeping to jsut the root
Perhaps a slow transition? There is still work required to get this to a place where it matches the build processes that exist in there currently. |
If we isolated cmake support in (If someone really needs a shared lib there is nothing easier than to write Admittedly I am reluctant to remove support for That means What do you think? |
@RobLoach @dumblob Thanks for the feedback. Possibly hiding everything behind This migration should also simplify CI/CD and addition of new "backends". As for where to place all the Finally the shared/static and header-only/header-src toggling is an interesting one, which should enable a great number more use-cases and also optimisations (remove support for "backend" that aren't being used). |
@@ -9,3 +9,5 @@ docs/src | |||
*.swo | |||
*.swp | |||
/private/ | |||
*build* | |||
.idea |
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.
Ide specific folders should be in your global git config, not per project (that includes .idea
, .vscode
, etc..)
I am a fervent user of CMake, and even have my method to use single header libraries with it. The only thing is that introducing CMake in a project like Nuklear will be quite complicated if you want to make it powerful.
Plus, managing all possibilities for demos will harden the task, and will be a nightmare to maintain. So my opinion is to not include CMake in Nuklear, but maybe provide an example project aside (another repository) which uses Nuklear and builds demos and all. |
@riri Removing Python dependency and adding CMake dependency does clean things up in terms of configuration (one tool for configure ; same tool for build ; sister tool for packaging [CPack] ; sister tool for testing [CTest]). The shared library option would be useful for reducing build times and enable a plethora of different deployment use cases including:
…in terms of Testing is a nightmare; agreed. CMake helps to centralise all the configuration, and combined with CTest and a CI/CD builtin to this repo one can do most of the testing. I've seen some GUI framework developers add in some ML and do object recognition to headless test UI drawing and interaction on "screen". Naturally we would run this on every supported OS and backend combination. |
@SamuelMarks I totall agree with all of your arguments, and CMake is great for managing projects industrially. But I'm still thinking it's better to define a CMakeNuklear repository which provides all those features, and keep the Nuklear one lean and mean, in the kiss philosophy. In this regard, I'm more on the side of @dumblob. We would have then:
Remains the question of the algamatron. Maybe having a simple CMake setup in Nuklear just to provide this tool and and simple definition of an interface library could suit, keeping more advanced stuff on the aside project I mentionned ? |
I am not experienced with cmake but this sounds a bit like a diversion from the current course of the Nuklear repository 😉. But I understand that this might be what IDE users expect. In which case - considering the "intrusiveness" (see below) cmake demands I would lean towards creating a separate repository (perhaps
So far I can not remember any
Yep, this is in line with my limited cmake experience. One word: intrusive. Cmake was and still appears to be intrusive. I think a separate repo might be the best of both worlds - easy to use in line with cmake practices and easy to maintain independently but still as part of the Nuklear project.
Well, there is no such thing as "support for unused backends" - there is always only one backend without any need to remove anything. Could you explain a bit more about this "removal of unused backends"? Thanks! |
Btw. thanks @riri for the link https://github.com/riri/using-shl/blob/main/src/shl/CMakeLists.txt - that is how I originally envisioned cmake users use single header libs. But as I said, I have limited experience with cmake projects in the wild so I depend on judgements of you all here 😉. |
I forgot to mention that I would prefer to have amalgamation to be as portable as possible - as I outlined above in #462 (comment) I think the best way would be to rewrite it to POSIX We can then even decide whether we shall embed the amalgamation script into Thoughts? |
@dumblob POSIX My PR isn't necessarily the most concise solution, and yes having I am using the word "backend" to refer to the directory names in https://github.com/Immediate-Mode-UI/Nuklear/tree/master/demo One of the major reasons I wanted to integrate CMake is to simplify the inclusion of different "backends" integrated into vcpkg. The others are:
I still think having all the CMake files in this repos to be a good idea. The root CMakeLists.txt and everything else under a root |
I'm quite confident with the POSIX sh solution, even if that means some minor modifications in source files. For windows (when not using mingw ), I'm sure a powershell expert could do it. I don't think we need more for rebuilding nuklear.h, once the correct script is written. edit: it's hard commenting from the phone with autocorrection :) |
@SamuelMarks for ci/di you mark a point. The most important is to not impose cmake to every user, but provide it as an alternative. So I'm splitted 😃 |
Random 2 cents here...as a possible new user to Nuklear, was a bit surprised the demos didn't have any popular build system support. The Windows based samples have the
...to generate build files for Windows and just proceed to checkout what Nuklear has to offer. Having read through this thread, I can kinda see where everyone is coming from. But honestly, not having CMake support (or some build system) just makes the getting start unnecessarily cumbersome. As random dude on the interwebs that uses CMake, I find a lot of what @SamuelMarks said is very sound and reasonable. Having CMake doesn't necessarily create dependencies unless you want it it to. A lot of projects have And lastly, SAY NO TO GLOB! |
I'm in favour of moving from our custom Python and build scripts to something a bit more standard. There are a few improvements to this PR that could be made, but I don't think that should stop us from moving in that direction. @SamuelMarks Do you have any plans to make updates here? If not I could help pick up a few of the pieces. |
Sure I can finish up here. In a few days one of my paying clients is paused until January 10 so this is a good period to work on this (and other things!). Just start replying with wishlist & fixlist and I'll incorporate and also finish up the whole |
Hi everybody! Sorry for not getting involved earlier. It seems my time capacity did not change for the better and I anticipate this to continue. So as a dinosaur among modern devs here, let me summarize my vision regarding CMake:
Nice to have:
|
add_subdirectory("example") | ||
endif (BUILD_EXAMPLES) | ||
|
||
include(CTest) |
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.
Would it make sense to conditionally include CTest
only if testing is enabled?
) | ||
|
||
foreach (demo ${demos}) | ||
add_subdirectory("${demo}") |
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 CMakeLists.txt for all these demos are possibly going to be 90% the same, would it make sense to add a cmake function/macro like add_nuklear_demo(SOURCES xxx LIBS xxx)
for convenience?
(And so that when projects are grouped with something like set_property(TARGET gdip PROPERTY FOLDER "Nuklear/examples")
this only has to be done in one place?
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.
Contributions welcome
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.
How do you propose to do this, send a PR to your fork?
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 happy to bring the branch into the repository too, if that makes contributions easier.
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 happy either way
Co-authored-by: Rob Loach <robloach@gmail.com>
Rewrote your Python code in CMake also.
[WiP: still some minor discrepancies in amalgamated
nuklear.h
to resolve]Addresses (in part): #101 #103