feat: Make use of CMake presets to enable easy switching between debug and release configurations on all platforms

[jadebenn]

Enable some optimization options for MSVC builds and altered the CMakePresets.json to make it easier to switch between debug and release configurations on all platforms.

[EmosewaMC]

MSVC build optimizations are handled by CMakePresets.json with build types there. I dont believe we should meddle with the optimizer in this case. MSVC by default with build.sh builds in Release and must have a build type specified if building through CLI. As such I dont believe this change is necessary.

If there are further details I am missing in regards to these flags please let me know, but as far as I know all this is handled by Config types in CMake by default.
further sources actually support doing the opposite, removing optimization flags from our cmake and using the command line instead for this. Because of this I believe a more congruent change would be to continue targeting Release we we currently do, and update CMakePresets.json to use Release as well. Then Removing the optimization flags from the CMakeLists should not have any effect since we are basically overriding the flags we may mean to be using (Release uses -O3 for release, but we may be overriding that with -O2). A few points here are worth discussing for sure.

[jadebenn]

I'm not sure I understand your point. While it's possible the O2 flag is being set on MSVC currently (haven't checked), based in the arifact size changing between enabling some of the other flags vs. not I'm fairly certain they aren't being applied currently.

[EmosewaMC]

That's because you are manually overriding them via the cmake lists. If you override an optimization flag, the last one takes effect and will override all the rest. Logically if you override the cli flag with one in the script, then the script will take precedence because that one is not only global but appended to the end of the flags.

[jadebenn]

My point being that if I was manually overriding the defaults with the same flags, nothing should change. Therefore some of these options aren't normally being enabled.

[jadebenn]

Moving the actual setting of flags to the .json would make sense though:

https://www.reddit.com/r/cpp_questions/comments/taxyut/what_is_the_best_way_to_set_compiler_flags_in_a/i04e1hk/

[jadebenn]

Any way I can get workflow approval in the future somehow, btw? Would be really nice when testing these multi-platform changes.

[EmosewaMC]

Ideally you should test this locally for function instead of relying on CI for multi platform changes

[EmosewaMC]

I have done a bunch of research and the only thing that makes a difference is /Zc:inline in the add_compile_options for MSVC. /O2 breaks Debug builds.

[jadebenn]

Possibly widening the scope of this PR to make better use of the JSON configurations for all platforms. We can have separate debug and release configurations that define the relevant flags to pass to CMake, and only keep the common flags for each platform defined in CMakeLists.txt. Thoughts?

[EmosewaMC]

I'd say play with it and see how it changes compilation sizes and builds. We haven't really touched this file since release since everything just works right now, but it may be worth experimenting and fixing that todo in the cmake file

[jadebenn]

This PR feels like a weird mix of a bit of everthing. Is this forward compat because our libraries are old? Is this nice to have?

It seems to be more standard in some places (build dir) and less standard in others (clang-16)

I'm specifically targeting clang-16 because the build fails on clang-14, which is the default install for Ubuntu via a package manager. If it can't find 16 in particular, it falls back to just searching for a clang install, which may or may not be able to compile DLU successfully.

This is mostly a nice to have so we can set up multiple build configurations in subdirectories of the build folder, but it's opt-in so as to not break the reams of tutorials that presume otherwise.

[Xiphoseer]

This PR feels like a weird mix of a bit of everthing. Is this forward compat because our libraries are old? Is this nice to have?

It seems to be more standard in some places (build dir) and less standard in others (clang-16)

I'm specifically targeting clang-16 because the build fails on clang-14, which is the default install for Ubuntu via a package manager. If it can't find 16 in particular, it falls back to just searching for a clang install, which may or may not be able to compile DLU successfully.

This is mostly a nice to have so we can set up multiple build configurations in subdirectories of the build folder, but it's opt-in so as to not break the reams of tutorials that presume otherwise.

Nice to have yes, but I'd presume few people install ahead of default clang packages in their distro and this logic will need patching as time passes and clang-17 becomes default.

Also: which version of ubuntu? The GH CI? LTS?

[jadebenn]

Nice to have yes, but I'd presume few people install ahead of default clang packages in their distro and this logic will need patching as time passes and clang-17 becomes default.

Also: which version of ubuntu? The GH CI? LTS?

Sorry, should have clarified: The Ubuntu distribution packaged with WSL has Clang-14. You have to install more recent versions manually.

If Cmake can't find clang-16, it defaults to just grabbing the path to a generic "clang" executable. This should prevent any issues in the future if this isn't touched. The clang-16 lookup would fail, but cmake would just invoke clang without a version suffix, which would presumably be a version > 16 at that point.

[jadebenn]

For clarification, I moved the -Werror flag into the presets.json so it doesn't trigger on builds from the command line of build.sh. This is so that if DLU is ever abandoned or idle for a prolonged period of time, users won't have to figure out how to disable it to make new builds. It's still applied to CI runs and runs from presets, though.

[jadebenn]

Hey, I haven't heard from anyone on this in a while and I don't have further changes I want to make. Can we look at merging this?

[aronwk-aaron]

looks good, will let Xipho have the final say

[jadebenn]

@Xiphoseer Are you still looking at this?

[EmosewaMC]

I will trust that this works and is well tested

[EmosewaMC]

thanks so much for taking on this task. I know this took a lot of your time and effort to get done :) Only one note for the future