-
Notifications
You must be signed in to change notification settings - Fork 762
[CMake] Feature flags #4552
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] Feature flags #4552
Conversation
19c7842 to
a33ee1e
Compare
We wrap all calls into the Virtualization Framework in a mockable singleton for testing purposes
Co-authored-by: Mustafa Kemal Gılor <mustafa.gilor@canonical.com> Signed-off-by: ScottH <59572507+sharder996@users.noreply.github.com>
5450a6f to
d87a67b
Compare
a33ee1e to
e6ec9a5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/apple-vz-boilerplate #4552 +/- ##
================================================================
+ Coverage 87.19% 87.20% +0.01%
================================================================
Files 246 246
Lines 14134 14134
================================================================
+ Hits 12323 12324 +1
+ Misses 1811 1810 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c13410e to
37e436f
Compare
37e436f to
a169d5c
Compare
The extra cmake preset is workflow for a limitation in cmake where inheritance is evaluated regardless of `condition` outcome. See https://gitlab.kitware.com/cmake/cmake/-/issues/23283
c3798fc to
2abd7fb
Compare
CMake Preset doesn't support conditionally inheriting over presets.
2abd7fb to
b51d488
Compare
| with: | ||
| script: | | ||
| const matrix = { include: [{ | ||
| "build-type": "Debug", |
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.
Not really related to this pr but we can get rid of the build-type now since build type is now defined by the preset.
| "inherits": "common" | ||
| }, | ||
| { | ||
| "name": "macOS-feature-flags", |
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.
Presets are not a way to "declare" the option flags -- this still has to be done in a CMake file. I'd say it'd be better to declare all feature flags as options in a dedicated CMake module and include it as the first thing in the project. cmake_dependent_option() would be a good way to declare feature options with prerequisites, such as:
include(CMakeDependentOption)
# Foo is the prerequisite. Assume it's an implicitly defined platform variable (e.g. APPLE)
cmake_dependent_option(FOO_BAR_ENABLED "Enable foo bar" OFF FOO OFF)
if(FOO_BAR_ENABLED)
message(STATUS "foo bar is enabled"*)
endif()The message "foo bar is enabled" would only print when FOO is set. Otherwise, it's forced to be OFF regardless of what the user actually sets at configure time:
cmake -S . -B build -DFOO:BOOL=TRUE -DFOO_BAR_ENABLED:BOOL=TRUE # would print
cmake -S . -B build -DFOO:BOOL=FALSE -DFOO_BAR_ENABLED:BOOL=TRUE # would not print the messageThat means it wouldn't matter either the preset or the configure parameters are setting a flag to true/false unless the precondition has met (e.g. platform or other hard requirements), meaning we can use a single preset for all platforms per configuration.
As for the use of the presets, they should be really about defining a specific way that the project is going to be built, for example the "CI", or the "public release" build could be the two presets to begin with. The CI preset would enable all the feature options whereas the public release would only enable the features that we intend to ship to the user. That way, the CI code becomes much simpler, and we can also reproduce the CI build locally.
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'd have to take a bit of time to think about how to implement the specifics, but what if we defined a new CMake helper function for our feature flags? That function could keep track of all defined feature flags for us. Then we could do the following:
# CMakeLists.txt
feature_flag(FOO_FEATURE ...)
feature_flag(BAR_FEATURE ...)
$ cmake -S . -B build -DENABLE_ALL_FEATURES=TRUEThat way, when one of us adds a new feature flag, we only have to define the flag in one spot (CMakeLists.txt), and the only other work we need to do is integrating the flag into the build (e.g. with a CMake if block or passing a preprocessor macro to the compiler).
cd9efab to
8fd2e2f
Compare
d655f8e to
0d4f2d5
Compare
As feature flags will be turned on by default, this PR selectively turns them off for *new* nightly builds as well as any builds triggered from a `release/*` branch. Couple other notes: - For nightly builds, Mattermost will be notified on a build failure - For snap builds, an additive preset is used to turn off `MP_ENABLE_ALL_FEATURES` with an environment variable that is set from CI - Unrelated to feature flags, but when a snap is published to the store, it is deleted from GitHub artifacts Theoretically, package naming should take care of itself provided the `-noff` suffix is added through CMake and the Multipass version. Closes #4552
A simple first approach for adding feature flags in Multipass based on defining CMake variables through various CMake Presets. This PR is based on #4542 to showcase its use in conditionally enabling the integration of the Apple Virtualization Framework.
The general idea is to have one main preset;feature-flags-enabledthat you can apply on any platform. Depending on your current platform, all common and relevant platform specific CMake variables will be defined. For example, on macOS,MULTIPASS_BACKENDSwill be defined toapple, but on Linux and Windows, it will remain undefined.CMake Presets does not support inheritance of conditional presets (see https://gitlab.kitware.com/cmake/cmake/-/issues/23283), so we are forced to have three presets, one for each platform. Features that are common to all platforms, i.e. availability zones, can still be added to a common preset,
common-feature-flags, that all platform specific presets inherit from.For CI, I rely on other developers to manually set
["vendor"]["enabled-in-ci"]inCMakePresets.jsonto signify it should be compiled in CI rather than try to parse the json file and evaluate whether or not there is any meaningful difference in "Edge" vs "Release" builds. This metadata is used to conditionally add another runner(s) to do a Debug build with the appropriate preset applied.The rest is just making sure that Release and Edge; the name I've decided to give to builds with feature flags turned on, stay logically separated.
Example #4555 that has all feature flags turned on
Example #4557 that demonstrates a build failure within code that is enabled by a feature flag
As demonstrated, my philosophy was that a feature flag enabled build should fail the entire workflow and effectively block development in
main. Ultimately, the development of a feature should be managed by an engineer, but the rest of team should not have to maintain it throughout its development cycle. For example, an engineer refactoringvirtual_machine.hshould not have to be responsible for also modifying all the virtual machine code forapple_virtual_machine.h.Closes #4555
Closes #4557
MULTI-2401