From 31a39b3b120018b59f37b558930968da67a5a239 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Thu, 10 Jun 2021 18:09:52 -0500 Subject: [PATCH] Add support for branch- and branding-based feature flagging (#10361) 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_. --- .github/actions/spelling/allow/apis.txt | 2 + .github/actions/spelling/expect/expect.txt | 1 + build/rules/GenerateFeatureFlags.proj | 97 ++++++++++ doc/feature_flags.md | 65 +++++++ src/common.build.post.props | 17 ++ src/features.xml | 13 ++ tools/FeatureStagingSchema.xsd | 83 +++++++++ tools/Generate-FeatureStagingHeader.ps1 | 206 +++++++++++++++++++++ 8 files changed, 484 insertions(+) create mode 100644 build/rules/GenerateFeatureFlags.proj create mode 100644 doc/feature_flags.md create mode 100644 src/features.xml create mode 100644 tools/FeatureStagingSchema.xsd create mode 100644 tools/Generate-FeatureStagingHeader.ps1 diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index 8252b10b7..56142fc56 100644 --- a/.github/actions/spelling/allow/apis.txt +++ b/.github/actions/spelling/allow/apis.txt @@ -45,6 +45,7 @@ IBind IBox IClass IComparable +IComparer IConnection ICustom IDialog @@ -85,6 +86,7 @@ NOREPEAT ntprivapi oaidl ocidl +ODR osver OSVERSIONINFOEXW otms diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 722631b26..fc07e9d2a 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -163,6 +163,7 @@ BPBF bpp BPPF branchconfig +brandings BRK Browsable bsearch diff --git a/build/rules/GenerateFeatureFlags.proj b/build/rules/GenerateFeatureFlags.proj new file mode 100644 index 000000000..8a8b494a6 --- /dev/null +++ b/build/rules/GenerateFeatureFlags.proj @@ -0,0 +1,97 @@ + + + + + + + + Release + AnyCPU + + + Fuzzing + AnyCPU + + + AuditMode + AnyCPU + + + Debug + AnyCPU + + + + + d97c3c61-53cd-4e72-919b-9a0940e038f9 + + + + $(SolutionDir)obj\$(Configuration)\GenerateFeatureFlags\ + $(SolutionDir)bin\$(Configuration)\ + + <_WTBrandingName Condition="'$(WindowsTerminalBranding)'=='Preview'">Preview + <_WTBrandingName Condition="'$(WindowsTerminalBranding)'=='Release'">Release + <_WTBrandingName Condition="'$(_WTBrandingName)'==''">Dev + + + + + + + + + <_BrandingLines Include="$(_WTBrandingName)" /> + + + + + + + <_BranchBrandingCacheFiles Include="$(IntermediateOutputPath)branch_branding_cache.txt" /> + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/doc/feature_flags.md b/doc/feature_flags.md new file mode 100644 index 000000000..f4c2a7d68 --- /dev/null +++ b/doc/feature_flags.md @@ -0,0 +1,65 @@ +# til::feature + +Feature flags are controlled by an XML document stored at `src/features.xml`. + +## Example Document + +```xml + + + + + Feature_XYZ + + Does a cool thing + + + 1234 + + + AlwaysEnabled|AlwaysDisabled + + + + branch/with/wildcard/* + + + + + + ... + + + + + + Release + + + + + + ... + + + + + + +``` + +## Notes + +Features that are disabled for Release using `alwaysDisabledReleaseTokens` are +*always* disabled in Release, even if they come from a branch that would have +been enabled by the wildcard. + +### Precedence + +1. `alwaysDisabledReleaseTokens` +2. Enabled branches +3. Disabled branches + * The longest branch token that matches your branch will win. +3. Enabled brandings +4. Disabled brandings +5. The feature's default state diff --git a/src/common.build.post.props b/src/common.build.post.props index 87416a898..e5bc2f5f3 100644 --- a/src/common.build.post.props +++ b/src/common.build.post.props @@ -64,4 +64,21 @@ + + + + OCCallFeatureFlagGenerator; + $(BuildDependsOn) + + + + + + + + + + $(SolutionDir)\bin\$(Configuration)\inc\TilFeatureStaging.h;%(ForcedIncludeFiles) + + diff --git a/src/features.xml b/src/features.xml new file mode 100644 index 000000000..7f11a0ba2 --- /dev/null +++ b/src/features.xml @@ -0,0 +1,13 @@ + + + + + Feature_ExampleFeat + This feature will be replaced in the next pull request. + AlwaysEnabled + + Preview + WindowsInbox + + + diff --git a/tools/FeatureStagingSchema.xsd b/tools/FeatureStagingSchema.xsd new file mode 100644 index 000000000..3e6819244 --- /dev/null +++ b/tools/FeatureStagingSchema.xsd @@ -0,0 +1,83 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tools/Generate-FeatureStagingHeader.ps1 b/tools/Generate-FeatureStagingHeader.ps1 new file mode 100644 index 000000000..8206b7bb1 --- /dev/null +++ b/tools/Generate-FeatureStagingHeader.ps1 @@ -0,0 +1,206 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT license. + +################################################################################ +# This script generates a header describing which Terminal/Console features +# should be compiled-in, based on an XML document describing them. + +[CmdletBinding()] +Param( + [Parameter(Position=0, Mandatory=$True)] + [ValidateScript({ Test-Path $_ })] + [string]$Path, + + [ValidateSet("Dev", "Preview", "Release", "WindowsInbox")] + [string]$Branding = "Dev", + + [string]$BranchOverride = $Null, + + [string]$OutputPath +) + +Enum Stage { + AlwaysDisabled; + AlwaysEnabled; +} + +Function ConvertTo-FeatureStage([string]$stage) { + Switch($stage) { + "AlwaysEnabled" { [Stage]::AlwaysEnabled; Return } + "AlwaysDisabled" { [Stage]::AlwaysDisabled; Return } + } + Throw "Invalid feature stage $stage" +} + +Class Feature { + [string]$Name + [Stage]$Stage + [System.Collections.Generic.Dictionary[string, Stage]]$BranchTokenStages + [System.Collections.Generic.Dictionary[string, Stage]]$BrandingTokenStages + [bool]$DisabledReleaseToken + + Feature([System.Xml.XmlElement]$entry) { + $this.Name = $entry.name + $this.Stage = ConvertTo-FeatureStage $entry.stage + $this.BranchTokenStages = [System.Collections.Generic.Dictionary[string, Stage]]::new() + $this.BrandingTokenStages = [System.Collections.Generic.Dictionary[string, Stage]]::new() + $this.DisabledReleaseToken = $Null -Ne $entry.alwaysDisabledReleaseTokens + + ForEach ($b in $entry.alwaysDisabledBranchTokens.branchToken) { + $this.BranchTokenStages[$b] = [Stage]::AlwaysDisabled + } + + # AlwaysEnabled branches win over AlwaysDisabled branches + ForEach ($b in $entry.alwaysEnabledBranchTokens.branchToken) { + $this.BranchTokenStages[$b] = [Stage]::AlwaysEnabled + } + + ForEach ($b in $entry.alwaysDisabledBrandingTokens.brandingToken) { + $this.BrandingTokenStages[$b] = [Stage]::AlwaysDisabled + } + + # AlwaysEnabled brandings win over AlwaysDisabled brandings + ForEach ($b in $entry.alwaysEnabledBrandingTokens.brandingToken) { + $this.BrandingTokenStages[$b] = [Stage]::AlwaysEnabled + } + } + + [string] PreprocessorName() { + return "TIL_$($this.Name.ToUpper())_ENABLED" + } +} + +class FeatureComparer : System.Collections.Generic.IComparer[Feature] { + [int] Compare([Feature]$a, [Feature]$b) { + If ($a.Name -lt $b.Name) { + Return -1 + } ElseIf ($a.Name -gt $b.Name) { + Return 1 + } Else { + Return 0 + } + } +} + +Function Resolve-FinalFeatureStage { + Param( + [Feature]$Feature, + [string]$Branch, + [string]$Branding + ) + + # RELEASE=DISABLED wins all checks + # Then, branch match by most-specific branch + # Then, branding type (if no overriding branch match) + + If ($Branding -Eq "Release" -And $Feature.DisabledReleaseToken) { + [Stage]::AlwaysDisabled + Return + } + + If (-Not [String]::IsNullOrEmpty($Branch)) { + $lastMatchLen = 0 + $branchStage = $Null + ForEach ($branchToken in $Feature.BranchTokenStages.Keys) { + # Match the longest branch token -- it should be the most specific + If ($Branch -Like $branchToken -And $branchToken.Length -Gt $lastMatchLen) { + $lastMatchLen = $branchToken.Length + $branchStage = $Feature.BranchTokenStages[$branchToken] + } + } + If ($Null -Ne $branchStage) { + $branchStage + Return + } + } + + $BrandingStage = $Feature.BrandingTokenStages[$Branding] + If ($Null -Ne $BrandingStage) { + $BrandingStage + Return + } + + $Feature.Stage +} + +$ErrorActionPreference = "Stop" +$x = [xml](Get-Content $Path -EA:Stop) +$x.Schemas.Add('http://microsoft.com/TilFeatureStaging-Schema.xsd', (Resolve-Path (Join-Path $PSScriptRoot "FeatureStagingSchema.xsd")).Path) | Out-Null +$x.Validate($null) + +$featureComparer = [FeatureComparer]::new() +$features = [System.Collections.Generic.List[Feature]]::new(16) + +ForEach ($entry in $x.featureStaging.feature) { + $features.Add([Feature]::new($entry)) +} + +$features.Sort($featureComparer) + +$featureFinalStages = [System.Collections.Generic.Dictionary[string, Stage]]::new(16) + +$branch = $BranchOverride +If ([String]::IsNullOrEmpty($branch)) { + Try { + $branch = & git branch --show-current 2>$Null + } Catch { + Try { + $branch = & git rev-parse --abbrev-ref HEAD 2>$Null + } Catch { + Write-Verbose "Cannot determine current Git branch; skipping branch validation" + } + } +} + +ForEach ($feature in $features) { + $featureFinalStages[$feature.Name] = Resolve-FinalFeatureStage -Feature $feature -Branch $branch -Branding $Branding +} + +### CODE GENERATION + +$script:Output = "" +Function AddOutput($s) { + $script:Output += $s +} + +AddOutput @" +// THIS FILE IS AUTOMATICALLY GENERATED; DO NOT EDIT IT +// INPUT FILE: $Path + +"@ + +ForEach ($feature in $features) { + $stage = $featureFinalStages[$feature.Name] + + AddOutput @" +#define $($feature.PreprocessorName()) $(If ($stage -eq [Stage]::AlwaysEnabled) { "1" } Else { "0" }) + +"@ +} + +AddOutput @" + +#if defined(__cplusplus) + +"@ + +ForEach ($feature in $features) { + AddOutput @" +__pragma(detect_mismatch("ODR_violation_$($feature.PreprocessorName())_mismatch", "$($feature.Stage)")) +struct $($feature.Name) +{ + static constexpr bool IsEnabled() { return $($feature.PreprocessorName()) == 1; } +}; + +"@ +} + +AddOutput @" +#endif +"@ + +If ([String]::IsNullOrEmpty($OutputPath)) { + $script:Output +} Else { + Out-File -Encoding UTF8 -FilePath $OutputPath -InputObject $script:Output +}