cmake cleanup

[cannedbeef]

some cleanup of the CMakeLists.txt file, still a work in progress so don't merge yet. opening this pr now so you can see my progress
more info of the changes can be found in the commit descriptions.
my goal with this is to keep it (nearly) functionally identical to the current cmake

[Darky-Lucera]

Hello!

I see the intention behind these changes (especially around making options more declarative and consistent), but I have a few concerns about the trade-offs introduced here.

1. Minimum CMake version (3.16 → 3.22)

The main driver seems to be cmake_dependent_option with full condition expressions. While that does simplify some option definitions, I’m not convinced it justifies raising the minimum required version for the whole project. The previous approach using explicit if() blocks is more verbose, but also simpler and keeps compatibility wider.

2. Use of CMAKE_SYSTEM_NAME vs standard variables

Replacing checks like if(IOS) or if(APPLE) with CMAKE_SYSTEM_NAME STREQUAL ... makes the code more verbose and, in my opinion, less readable. The built-in variables are idiomatic CMake and already cover the common cases well.

3. Increased indirection

The new structure introduces more intermediate variables (e.g. MINIFB_USE_X11_API) and more declarative logic. While this can be beneficial in large or highly configurable projects, here it makes the flow harder to follow compared to the more direct conditional structure we had before.

Overall, I’m not against improving structure, but I think these changes increase complexity without a clear payoff in this case. I’d prefer to keep a simpler approach unless we have a strong reason (e.g. new platform requirements, real maintenance issues, etc.).

[cannedbeef]

Hi!

You can see in the code, lines 287-308, an example of the concept I was trying to implement with these changes, having all the possible build targets be seperated, yet not reliant on each other, so the code can test for them without nesting.

While personally I think this is clearer and more readable, I can understand the concerns, and given that the change was only really an experiment to begin with, I could always revert it and go back to the previous structure if you feel that's neccecary.

I would ideally like to keep using cmake_dependant_option though, even if we end up using the older 3.16 syntax (as per point 1), as it's a built-in feature designed for our specific use case.

On point 2, these variables (UNIX, APPLE, WIN32, etc) are discouraged in modern CMake, and (in the words of the documentation) are "soft-deprecated". Avoiding their use, although more verbose, lets the code be more descriptive of what's actually happening. (and can also avoid some oddities in their definition, such as APPLE overlapping with UNIX)

e.g line 6, although it's much longer than the alternative APPLE, the updated version very specifically describes the two build targets that use ObjC, instead of the blanket "apple=objc", which (in my opinion), is a change for the better.

Overall, these changes all attempt to move the code toward a different structure that separates all the backends further, and I can understand if that's not the direction you want to take the code, if so, I can close this PR.

[Darky-Lucera]

Hi,

Thanks for the detailed explanation, I understand much better what you were aiming for.

On the general idea of separating build targets so they can be reasoned about independently: I’m actually OK with that direction. Having less nesting and clearer separation between backends can be a good thing if it doesn’t make the file harder to follow.

Regarding cmake_dependent_option, I’m not against using it, but I’d prefer not to raise the minimum required CMake version just for that. One possible compromise would be to use it conditionally (e.g. based on CMAKE_VERSION), and fall back to the current approach otherwise. That way we keep compatibility with older environments where people rely on the system CMake.

On the platform checks, I don’t think it’s accurate to say that variables like APPLE, UNIX or WIN32 are discouraged in any strong sense. They’re still widely used and idiomatic in most CMake projects.

Using CMAKE_SYSTEM_NAME is more explicit, but it also introduces string comparisons, which are more verbose and easier to get wrong (typos, exact string matching, etc.). In practice, I don’t see a clear benefit for this project.

About the specific example (the ObjC check): I understand the intention of being explicit about Darwin/iOS, but I find the longer condition harder to read. If the goal is clarity of intent, something like APPLE OR IOS would already make that explicit while staying concise.

Finally, on the overall structure: I agree that separating the backends more cleanly is a good direction, but I’d like to avoid significantly increasing the cognitive load of the CMakeLists. Right now I feel some of the changes move us in that direction without a clear payoff.

So in short: I’m open to some of the structural ideas, but I’d prefer to keep the simpler, more idiomatic approach where the benefits of the new structure aren’t clearly outweighing the added complexity.

[cannedbeef]

This could probably be merged now; I've tested it's functionally identical on X11 and Emscripten, but can't test other platforms right now.

[Darky-Lucera]

Sorry for the long delay, I have been busy for a long time. I will review it this week

[Darky-Lucera]

Thanks for working on this cleanup. The target-based organization and the example helper make the file easier to follow. I found a few compatibility and consumer-facing regressions that I think we should address before merging, particularly around exported target properties, package dependencies, and Emscripten linking.

[Darky-Lucera]

Could we keep MiniFB's internal warning, optimization, and debug flags PRIVATE? PUBLIC is appropriate for usage requirements that consumers need, but these options export settings such as -O2, /O2, /RTC1, and DEBUG, which may unexpectedly alter downstream builds. The examples could receive the same private options explicitly through minifb_add_example(). The same concern applies to the PUBLIC compile definitions below.

[Darky-Lucera]

Could we apply these link options to the example executables, perhaps inside minifb_add_example()? Since minifb is a static library, it has no link step of its own, and PRIVATE link options are not propagated to the final executables. As written, the Emscripten examples will not receive these options.

[Darky-Lucera]

Was removing the web_assets target intentional? The documented workflow expects files such as build-web/index.html, but the HTML assets will no longer be copied into the build directory. Could we preserve the copy step, or update the documented workflow if the assets are now handled elsewhere?

[Darky-Lucera]

Could we keep this dependency-discovery logic, or update minifb-config.cmake.in as part of this change? The exported static target still references OpenGL::GL and X11::X11. With these variables unset, the generated package config contains empty if() conditions and does not call find_dependency(), so consumers of minifb::minifb may fail during configuration or generation.

[Darky-Lucera]

Could we normalize the deprecated options before deriving MINIFB_USE_X11_API and MINIFB_USE_OPENGL_API? With -DUSE_WAYLAND_API=ON, the X11/OpenGL state has already been calculated before this assignment, which may leave conflicting backends enabled. Applying the compatibility mapping first would preserve the expected legacy behavior.

[Darky-Lucera]

Since the project still supports CMake 3.16, would it be worth setting policy CMP0127 explicitly in the 3.22+ branch? The full condition syntax used by cmake_dependent_option() is controlled by that policy, so setting it to NEW would make the intended behavior explicit.

[Darky-Lucera]

Hello!
How's it going?
Is it finished?

[cannedbeef]

Apologies for the delay.
Just wondering if you could take a look at the TODOs i've put in the cmake, they revolve around macos/ios stuff I can't test and don't know about.
Regarding the last point of your review, is this really necessary? I feel having the new syntax in an if-block with CMAKE_VERSION VERSION_GREATER_EQUAL 3.22, as well as an extensive comment explaining it, is clear enough.