Add support for branch- and branding-based feature flagging #10361

DHowett · 2021-06-08T03:53:01Z

This pull request implements a "feature flagging" system that will let
us turn Terminal and conhost features on/off by branch, "release" status
or branding (Dev, Preview, etc.).

It's loosely modelled after the Windows OS concept of "Velocity," but
only insofar as it is driven by an XML document and there's a tool that
emits a header file for you to include.

It only supports toggling features at compile time, and the feature flag
evaluators are intended to be fully constant expressions.

Features are added to src\features.xml and marked with a "stage". For
now, the only stages available are AlwaysDisabled and AlwaysEnabled.
Features can be toggled to different states using branch and branding
tokens, as documented in the included feature flag docs.

For a given feature Feature_XYZ, we will emit two fixtures visible to
the compiler:

A preprocessor define TIL_FEATURE_XYZ_ENABLED (usable from MIDL,
C++ and C)
A feature class type Feature_XYZ with a static constexpr member
IsEnabled() (usable from C++, designed for if constexpr()).

Like Velocity, we rely on the compiler to eliminate dead code caused by
things that compile down to if constexpr (false). :)

Michael suggested that we could use WindowsInbox as a branding to
determine when we were being built inside Windows to supplant our use of
the __INSIDE_WINDOWS preprocessor token. It was brilliant.

Design Decisions

Emitting the header as part of an MSBuild project
- WHY: This allows the MSBuild engine to ensure that the build is
  only run once, even in a parallel build situation.
Only having one feature flag document for the entire project
- WHY: Ease.
Forcibly including TilFeatureStaging with /FI for all CL compiler
invocations.
- WHY: If this is a project-wide feature system, we should make it as
  easy as possible to use.
Emitting preprocessor definitions instead of constexpr/consteval
- WHY: Removing entire functions/includes is impossible with if constexpr.
- WHY: MIDL cannot use a static constexpr bool, but it can rely on
  the C preprocessor to remove text.
Using MSBuild to emit the text instead of PowerShell
- WHY: This allows us to leverage MSBuild's WriteOnlyWhenDifferent
  task parameter to avoid changing the file's modification time when
  it would have resulted in the same contents. This lets us use the
  same FeatureStaging header across multiple builds and multiple
  branches and brandings assuming that they do not result in a
  feature flag change.
- The risk in using a force-include is always that it, for some
  reason, determines that the entire project is out of date. We've
  gone to great lengths to make sure that it only does so if the
  features actually materially changed.

miniksa · 2021-06-08T17:17:42Z

doc/feature_flags.md

+2. Enabled branches
+3. Disabled branches
+   * The longest branch token that matches your branch will win.
+3. Enabled brandings


weird numbering

miniksa · 2021-06-08T17:18:06Z

build/rules/GenerateFeatureFlags.proj

+    <WriteLinesToFile File="$(OpenConsoleCommonOutDir)\inc\TilFeatureStaging.h"
+      Lines="@(_FeatureFlagFileLines)"
+      Overwrite="true"
+      WriteOnlyWhenDifferent="true" />


This is some straight up wizardry.

DHowett · 2021-06-08T19:05:50Z

src/internal/stubs.cpp

    // True so propsheet will show configuration options but be sure that
    // the open one won't attempt handoff from double click of OpenConsole.exe
-    isEnabled = true;
+    isEnabled = Feature_AttemptHandoff::IsEnabled();


this actually breaks @miniksa's intent here, so NTS to fix it

miniksa

Looks good to me, including the conversions from __INSIDE_WINDOWS. Now they actually have intent. If you've rid us of them all, though, you may want to remove that def completely so someone doesn't accidentally use it going forward and migrates to the feature flagging.

DHowett · 2021-06-10T16:03:04Z

document: https://github.com/microsoft/terminal/blob/dev/duhowett/jerk/doc/feature_flags.md

src/features.xml

…atures

tools/FeatureStagingSchema.xsd

carlos-zamora

This is fantastic! So excited for this to land :)

ghost · 2021-06-10T23:09:49Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This pull request implements a "feature flagging" system that will let us turn Terminal and conhost features on/off by branch, "release" status or branding (Dev, Preview, etc.). It's loosely modelled after the Windows OS concept of "Velocity," but only insofar as it is driven by an XML document and there's a tool that emits a header file for you to include. It only supports toggling features at compile time, and the feature flag evaluators are intended to be fully constant expressions. Features are added to `src\features.xml` and marked with a "stage". For now, the only stages available are `AlwaysDisabled` and `AlwaysEnabled`. Features can be toggled to different states using branch and branding tokens, as documented in the included feature flag docs. For a given feature Feature_XYZ, we will emit two fixtures visible to the compiler: 1. A preprocessor define `TIL_FEATURE_XYZ_ENABLED` (usable from MIDL, C++ and C) 2. A feature class type `Feature_XYZ` with a static constexpr member `IsEnabled()` (usable from C++, designed for `if constexpr()`). Like Velocity, we rely on the compiler to eliminate dead code caused by things that compile down to `if constexpr (false)`. :) Michael suggested that we could use `WindowsInbox` as a branding to determine when we were being built inside Windows to supplant our use of the `__INSIDE_WINDOWS` preprocessor token. It was brilliant. Design Decisions ---------------- * Emitting the header as part of an MSBuild project * WHY: This allows the MSBuild engine to ensure that the build is only run once, even in a parallel build situation. * Only having one feature flag document for the entire project * WHY: Ease. * Forcibly including `TilFeatureStaging` with `/FI` for all CL compiler invocations. * WHY: If this is a project-wide feature system, we should make it as easy as possible to use. * Emitting preprocessor definitions instead of constexpr/consteval * WHY: Removing entire functions/includes is impossible with `if constexpr`. * WHY: MIDL cannot use a `static constexpr bool`, but it can rely on the C preprocessor to remove text. * Using MSBuild to emit the text instead of PowerShell * WHY: This allows us to leverage MSBuild's `WriteOnlyWhenDifferent` task parameter to avoid changing the file's modification time when it would have resulted in the same contents. This lets us use the same FeatureStaging header across multiple builds and multiple branches and brandings _assuming that they do not result in a feature flag change_. * The risk in using a force-include is always that it, for some reason, determines that the entire project is out of date. We've gone to great lengths to make sure that it only does so if the features _actually materially changed_. (cherry picked from commit 31a39b3)

This comment has been minimized.

Sign in to view

miniksa reviewed Jun 8, 2021

View reviewed changes

DHowett commented Jun 8, 2021

View reviewed changes

miniksa reviewed Jun 9, 2021

View reviewed changes

DHowett added 8 commits June 9, 2021 19:04

JERK

d44ee40

Document!

d8525bc

schema and gen: fix token uniques, check for cplusplus

2e8c95f

rule: convert to a forced include

67c6403

Don't try to add it to the solution.

a6b6032

spaces, not tabs, yo

0a0da67

Automatically recalculate when the branch/branding change!

d743af0

Try very hard to avoid rebuilds when branch is changeD

8a10a63

DHowett force-pushed the dev/duhowett/jerk branch from 81ead62 to 8a10a63 Compare June 10, 2021 00:05

DHowett added 2 commits June 9, 2021 19:07

Use an example feature file for now.

f254eeb

Speelbot

ce4d7bb

This comment has been minimized.

Sign in to view

microsoft deleted a comment from github-actions bot Jun 10, 2021

DHowett changed the title ~~[DRAFT] homebrew Velocity-like feature flagging~~ Add support for branch and branding-based feature flagging Jun 10, 2021

DHowett changed the title ~~Add support for branch and branding-based feature flagging~~ Add support for branch- and branding-based feature flagging Jun 10, 2021

DHowett marked this pull request as ready for review June 10, 2021 16:03

DHowett requested review from carlos-zamora and lhecker June 10, 2021 16:03

DHowett commented Jun 10, 2021

View reviewed changes

src/features.xml Show resolved Hide resolved

DHowett added 2 commits June 10, 2021 12:01

Emit a mismatch pragma to make sure two binaries don't lik w/ diff fe…

49664ee

…atures

spellbot ODR

f301545

miniksa reviewed Jun 10, 2021

View reviewed changes

tools/FeatureStagingSchema.xsd Outdated Show resolved Hide resolved

Get your tabs outta here

4781e0d

miniksa approved these changes Jun 10, 2021

View reviewed changes

microsoft deleted a comment from github-actions bot Jun 10, 2021

carlos-zamora approved these changes Jun 10, 2021

View reviewed changes

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2021

ghost merged commit 31a39b3 into main Jun 10, 2021

ghost deleted the dev/duhowett/jerk branch June 10, 2021 23:09

adrastogi mentioned this pull request Sep 27, 2023

Add support for building / testing new features in Dev Home without impacting existing customers microsoft/devhome#1374

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for branch- and branding-based feature flagging #10361

Add support for branch- and branding-based feature flagging #10361

Uh oh!

DHowett commented Jun 8, 2021 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

miniksa Jun 8, 2021

Uh oh!

DHowett Jun 8, 2021

Uh oh!

miniksa Jun 8, 2021

Uh oh!

DHowett Jun 8, 2021

Uh oh!

miniksa left a comment

Uh oh!

This comment has been minimized.

DHowett commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Uh oh!

ghost commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add support for branch- and branding-based feature flagging #10361

Add support for branch- and branding-based feature flagging #10361

Uh oh!

Conversation

DHowett commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Decisions

Uh oh!

This comment has been minimized.

This comment has been minimized.

miniksa Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

DHowett commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 10, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DHowett commented Jun 8, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.