Feat/cmake presets

[Swiftb0y]

This reduces the copy-pasted build configs across CI by unifying them in a CMakePreset. That way the same options can be used in local builds more easily. Moreover, this reduces the size of our github workflow config files, easing a transition to a different service.

Open Questions:

1. Should we also move the OS-specific "cacheVariables" into presets?
2. Do also want to use the other CMakePreset features (configs for tests, workflows, etc)?
3. Now that this reduced the noise, it highlighted some more config options that seemed inconsistent across related jobs (see -DOPTIMIZE=OFF missing for the clang-tidy, -DLOCALECOMPARE=ON everywhere except macos, etc). Which of those are intentional and which have been missing by error?
4. the cacheVariables objects supports js booleans as values as well which would get converted into "TRUE"/"FALSE" while we use ON/OFF. Should we change that?

[daschuer]

Here some thoughts:

The long list of flags in the workflow files is sourced by the idea to make every possible option visible. On the user machine all of them should be on a reasonable default after performing a plain CMake configuration. The good thing with giving them explicit is that changed defaults due to regressions on the workflow runner are more likely detected.

On the user machine the presets are reasonable if we want to maintain a set of different configuration that require more than a single change of a config option. I am no sure if this is currently the case and if this would be helpful or another source of confusion.

Compared to single source of flags for the CI, individual files/regions have the advantage that they can be easily changed individual.

So there are pros and cons. In general i like the idea. We need to make sure there id more on the pro side finally.

[daschuer]

LOCALECOMPARE should be ON for all.
This should be already the case by the main CMakeList.txt the only case where this is not set is of you build on Apple with dynamic libraries.

[Swiftb0y]

The long list of flags in the workflow files is sourced by the idea to make every possible option visible.

Well thats not really the case currently, we have loads of options that are not defined in there.

The good thing with giving them explicit is that changed defaults due to regressions on the workflow runner are more likely detected.

Thats a good point, the explicitness doesn't vanish with this approach though, it just moves into another file.

On the user machine the presets are reasonable if we want to maintain a set of different configuration that require more than a single change of a config option.

Right, thats already the case for me for example where I have a debug config for development and a release config I use to install betas from my fork. Having the CI options in the CMakePresets.json is also nice because then my CMakeUserPresets.json can just inherit from from the upstream config which ensures my local build more closely matches the official build.

---

I see no cons with the CMakePresets approach (at least I couldn't spot any in your previous message, if there are please repeat them in a more explicit manner so I can understand them). CMakePresets is well supported and encouraged by most developer Tooling (IDE support especially).

[Swiftb0y]

LOCALECOMPARE should be ON for all. This should be already the case by the main CMakeList.txt the only case where this is not set is of you build on Apple with dynamic libraries.

So can we specify it explicitly in the Presets to ON or will that cause a hard fail in the dynamic case for macOS?

[daschuer]

So can we specify it explicitly in the Presets to ON or will that cause a hard fail in the dynamic case for macOS?

I think yes.

[daschuer]

I have a debug config for development and a release config I use to install betas from my fork.

What is special about it in your case? Normally we have the CMAKE_BUILD_TYPE for this.

[daschuer]

The current solution has the disadvantage that it is tempting to use this at home or in the build system of various distros. Even though it should IMHO not be done, because it bypasses the carefully maintained calculation of default values in the CMakeList.txt Using the calculated flags is the recommended way! If we think this is not the case, we should adjust the condition that this is again true.

The use case for our CI is to fix a certain build setting for our CI, just to fail if something unexpected happens. We need to make it very clear that this is the "only" use case for this preset.

Recommend to use the presets with all flags fixed reverts the design pattern of the flags. Today most of them are "Internal and correct by default". If we put them in a preset representing all on a const value, the preset matches only for one single case. We need up 2^n presets to cover all cases and a smart selector for the correct one.

What can we do about it to make it clear?

I think we should give our CI preset a funky name like "workflow_regression_check" or something and than add a "release" with no flags and a debug preset with only "CMAKE_BUILD_TYPE": "Debug"

Dos that make sense?

[Swiftb0y]

What is special about it in your case? Normally we have the CMAKE_BUILD_TYPE for this.

I have some custom options in the debug config, for example I disable some parts of mixxx to speed up fresh compilations, my release build comes with -DWARNINGS_FATAL=OFF, while the dev build is with ON, etc.

The current solution has the disadvantage that it is tempting to use this at home or in the build system of various distros. Even though it should IMHO not be done, because it bypasses the carefully maintained calculation of default values in the CMakeList.txt Using the calculated flags is the recommended way! If we think this is not the case, we should adjust the condition that this is again true.

People will use their own config anyways. I'm pretty sure no 3rd party packager relies on the defaults, not even we do (see the diff of this PR). The purpose of CMakePresets is to make it easier for local compiles resemble our CI setup closer. IMO this would make it easier for people to rely on our defaults (as specified in CMakePresets).

Today most of them are "Internal and correct by default". If we put them in a preset representing all on a const value, the preset matches only for one single case. We need up 2^n presets to cover all cases and a smart selector for the correct one.

If they were internal and correct by default, we shouldn't have had to specify them on CI in the first place. I can go ahead and check which of those match the default already. We also don't need $2^n$ presets thanks to the CMakePresets being relatively smart and expressive in terms of its inheritance functionality. A single base would suffice, a single debug and release mixin would work. The rest of the presets could be built by inheriting from those. Those extra presets are also not new because they're just the artefact of the defines already custom to each CI workflow.

What can we do about it to make it clear?

Make clear what exactly? For people to not use their own config?

I think we should give our CI preset a funky name like "workflow_regression_check" or something and than add a "release" with no flags and a debug preset with only "CMAKE_BUILD_TYPE": "Debug"

Dos that make sense?

Yes, but whats the advantage of that versus the more general config?

---

Also a quick note, and this is more speculation since I'm not super familiar with CMake, but I think the way we currently do it with the option and all the conditional settings is considered pretty bad style nowadays because it hinders us from modularizing the CMakeFile (which is very much needed based on its SLOC count).

[daschuer]

If they were internal and correct by default, we shouldn't have had to specify them on CI in the first place

They have been artificially added. In case of Ubuntu we have only: https://github.com/mixxxdj/mixxx/blob/44a997258d490887eab95874ac9fe05080d34b00/packaging/debian/rules#L7

this reminds me that we still not have a keyfinder solution on Ubuntu

Setting -DCMAKE_BUILD_TYPE=RelWithDebInfo is redundant, because it should be the default -DINSTALL_USER_UDEV_RULES=OFF is required because debian has its own way to install udef rules.

That's it.

[daschuer]

CMakePresets being relatively smart and expressive

I consider this a maintenance burden to have on one hand smart defaults in CMakeList.txt, override it with preset and add another layer of smart.

That is the main reason I prefer to provide an almost empty preset as recommended for local use. This allows us to maintain the smart logic within the CMakeList.txt close to usage.

Make clear what exactly? For people to not use their own config?

To not use our CI config. They are always free to define their CMakeUserPresets.json. This is out of scope of this PR.

Yes, but whats the advantage of that versus the more general config?

This is the more general config IMHO. Or what else do you have in mind?

I think the way we currently do it with the option and all the conditional settings is considered pretty bad style nowadays because it hinders us from modularizing the CMakeFile (which is very much needed based on its SLOC count).

I don't care about SLOC count as long at is works out of the box. The alternative to move all this to a "smart" preset is worse, because you don't now have the access to the truth an a turing complete scripting language. We can easily break out parts into modules. I had recently some attempts to move more to the find modules. But to be honest I consider our CMakeList still in a maintainable state.

[daschuer]

Back to this PR:

• The idea to describe our configs in a preset is a good idea.
• If we provide a minimal config for home builds and package maintainers it will work for me.
• The "all settings fixed" config should be clearly recognizable as "Do not use". Maybe we can even assert that it is used on a GitHub CI runner. This ensures that we don't have maintain it for home use.

[Holzhaus]

I'd be okay with adding a preset for CI. If the name it ci or something like this (and maybe a descriptive comment), it's sufficiently clear that it's not recommended for other uses. Asserting that it's used on a GitHub CI runner just makes it harder to test this config locally.

However, maybe even better would be to remove all the hardcoded flags, and instead add a new build step after configuration which parses the output of cmake -L to determine if all flags have been set properly. That way, we'd even test if the auto-selection of flags works properly.

[github-actions[bot]]

This PR is marked as stale because it has been open 90 days with no activity.

[github-actions[bot]]

This PR is marked as stale because it has been open 90 days with no activity.