lukka / run-cmake

GitHub Action to build C++ applications with CMake (CMakePresets.json), Ninja and vcpkg on GitHub.
MIT License
173 stars 19 forks source link

Permit additional cmake variables to be set in addition to the preset #92

Closed justusranvier closed 1 year ago

justusranvier commented 1 year ago

Presets are a good feature but it can be impractical to write a CMakePresets.json that contains every necessary variable, particularly if you have a cmake variable whose value needs to match a matrix variable.

The only way to use this action in that case is to hard code the matrix from the action into CMakePresets.json which means now you have two files which must be kept in sync with each other or else your action will break.

cmake itself allows additional variables to be set on the command line even when a preset is specified.

lukka commented 1 year ago

@justusranvier seems reasonable to have this feature in order to avoid having to keep in sync two files. What would be exactly needed? How would you like to invoke the action?

lukka commented 1 year ago

@justusranvier looking forward to hear what would be useful for your scenario.

Would this be ok:

uses: run-cmake@v10
with:
    configurePreset: name
    buildPreset: name
    buildPresetAdditionalArguments: "-DName=Value -DName2=Value2"

then the action would run the following when building:

cmake --build --preset name -DName=Value -DName2=Value2

where the provided additional argument as used as is without any processing executed by the action.

Similarly for configurePreset and testPreset.

justusranvier commented 1 year ago

That should work. I vaguely remember that this action had a similar option for passing raw uninterpreted cmake arguments before preset support was added and that is precisely what I was looking for.

pintsized commented 1 year ago

Perhaps related to this, would it include support for --config [CMAKE_CONFIGURATION_TYPES]?

My use case is multi-config generators (Ninja Multi-Config), where the whole point is to not have to supply the desired configuration type up front. e.g.

cmake --preset ninja-multi-config      # Prepare for all configuration types
cmake --build --preset my-app --config [Debug|Release|RelWithDebugInfo|MinSizeRel|...]

As things stand we would have to create a preset for every required configuration type for every target, which is super verbose and seems unnecessary. I'm kinda working around this for now by setting CMAKE_DEFAULT_BUILD_TYPE to Release, which means I can still have one preset per target and use --config locally, and then my CI workflow always defaults to Release. I'd like to add some Debug builds to CI as well though, which means I'm back to exploding out my presets file.

This might be better as its own configuration option (maybe buildConfiguration)? But perhaps it would just work with buildPresetAdditionalArguments as described anyway?

To be clear, this isn't about CMAKE_BUILD_TYPE, which is ignored in multi-config generators.

lukka commented 1 year ago

@justusranvier @pintsized it makes sense to allow the user to customize the commands to run the configuration, build, test with CMake, especially for minimizing the number of CMake presets when using multi-configurations generators.

You can see how to achieve that today with run-cmake in this branch, how does this sample sound?

https://github.com/lukka/CppCMakeVcpkgTemplate/tree/dev/multi_config_sample_customization

The idea is to use the existing inputs buildPresetCmdString/testPresetCmdString that were created exactly for allowing user customization of those commands.

Another option would be to add a buildPresetAdditionalArgs and testPresetAdditionalArgs as a list of arguments to append to the existing array of buildPresetCmdString/testPresetCmdString. Note that the array syntax is mandatory anyway, because it is basically impossible to parse an arbitrary string and factor it out in individual arguments to pass to the underlying process spawn APIs.

lukka commented 1 year ago

Fixed in https://github.com/lukka/run-cmake/releases/tag/v10.2