Enable Change Waves by benvillalobos · Pull Request #5710 · dotnet/msbuild

benvillalobos · 2020-09-04T06:55:53Z

Enable Change Waves

Provide customers with the ability to opt out of potentially breaking changes. This effectively warns them that there is a breaking change coming and gives them time to prepare for it.

To Do:

Reach out to Kathleen and get approval on using MSBuildDisableFeaturesFromVersion
Rename intrinsic function to AreFeaturesEnabled
If MSBuildDisableFeaturesFromVersion is set to a value higher than the current highest change wave, clamp it to the current highest change wave.
If MSBuildDisableFeaturesFromVersion is set to a value within the bounds of versions, but not set to a specific wave, set MSBuildDisableFeaturesFromVersion to the next highest wave.
Have the build manager throw warnings as necessary in BeginBuild [Warnings are logged but not visible in logs or output window?]

cdmihai · 2020-09-04T15:08:14Z

src/Shared/Traits.cs

Who sets this and what do they expect should happen? I guess some user facing documentation would be nice at some point :)

cdmihai · 2020-09-04T15:09:47Z

src/Shared/ChangeWaves.cs

All this version parsing might get noticeable, especially if the under-the-wave features are on hotter paths. Consider storing the version constants directly as Version instead of strings.

cdmihai · 2020-09-04T15:22:20Z

src/Shared/ChangeWaves.cs

Warning as in logger BuildWarningEventArgs? Throw a custom exception type, catch it in the nearest places that can log events, and convert it to a warning event there.

I was hoping to avoid this, but it looks like that won't be possible. What was suggested in the PR review meeting was to return some enum ChangeWavesReturnType.InvalidFormat, then at each feature site we check what the return value was and log a warning based on that.

That wouldn't work for features implemented in places which don't have access to a LoggingService.

An alternative to bubbling up a special exception would be to push down the LoggingService everywhere (potentially guised as a more generic logging interface, maybe). Then you can change the method to public static bool IsChangeWaveEnabled(string wave, IWhateverLoggingInteface log)

I got around this by throwing warnings in Evaluator.cs, as I'm now setting MSBUILDCHANGEWAVEVERSION as a default property. The version gets sanitized there and a warning is thrown for out-of-rotation waves and invalid formats.

cdmihai · 2020-09-04T15:26:22Z

What if a wave has multiple breaking changes and the user wants to turn a subset of them off, not the entire wave? Should developers check both the wave and a Trait.EscapeHatch? Or should we instead compose a wave out of multiple Trait.EscapeHatches?

benvillalobos · 2020-09-04T17:25:28Z

I believe this would happen on a case by case basis. Which falls in line with the intention of change waves. Ideally we get feedback from customers that state they want to have a particular feature enabled while disabling all others.

Feature code would then look something like:

if(ChangeWaves.IsChangeWaveEnabled(ChangeWaves.Wave16_8) || Traits.Instance.SomeFeatureEscapeHatch)
{
    <feature>
}

The discussion around having subsets of change-waves indicated that we don't want to do this for every feature, as it would become very cumbersome over time.

cdmihai · 2020-09-04T19:07:50Z

I believe this would happen on a case by case basis. Which falls in line with the intention of change waves. Ideally we get feedback from customers that state they want to have a particular feature enabled while disabling all others.

But if you need customer feedback on which feature to disable, you need to do another release to add the escape hatch for it and the customer would have to wait for that release instead of being able to immediately opt out from a particular feature.

Alternative you can hide the change waive inside Traits to make the conditions easier to write. Have a lookup table in Traits that maps each escape hatch to a change wave number. Feature writers would then just need to check for if(Traits.Instance.SomeFeatureEscapeHatch), or Traits.Instance.IsActive(Traits.SomeFeatureEscapeHatch))

benvillalobos · 2020-09-18T06:29:15Z

/azp run

azure-pipelines · 2020-09-18T06:29:20Z

Pull request contains merge conflicts.

benvillalobos · 2020-09-18T06:39:04Z

Alternative you can hide the change waive inside Traits to make the conditions easier to write. Have a lookup table in Traits that maps each escape hatch to a change wave number. Feature writers would then just need to check for if(Traits.Instance.SomeFeatureEscapeHatch), or Traits.Instance.IsActive(Traits.SomeFeatureEscapeHatch))

Discussion for our PR meeting: I find that this wouldn't add much value over just adding escape hatches for features we feel would need them. I may be misunderstanding though.

rainersigwald · 2020-09-18T14:52:24Z

documentation/wiki/ChangeWaves-Dev.md

Suggested change

Sometimes we want to make a breaking change _and_ give customers a heads up as to what's breaking. So we develop the change and provide an opt-out while acknowledging that this feature will become a standard functionality down the line.

Sometimes we want to make a breaking change _and_ give customers a heads up as to what's breaking. So we develop the change and provide an opt-out while acknowledging that this feature will become standard functionality down the line.

rainersigwald · 2020-09-18T14:53:35Z

documentation/wiki/ChangeWaves.md

Should this doc also say "don't use change waves if you're not part of the core MSBuild/SDK/Microsoft set of stuff?"

I'll add a note at the top in the dev docs.

rainersigwald · 2020-09-18T14:54:00Z

ref/Microsoft.Build/net/Microsoft.Build.cs

Why is this public? Isn't this an internal implementation detail?

I switched it over to internal, and now I think it needs to be back at public. The latest addition to TestEnvironment (Making a SetChangeWave function) means that it needs to set ChangeWaves.DisabledWave to null, which it can't do if the class is anything less than public.

What if you did that in ResetInstance_ForUnitTestsOnly instead? I would expect that method to reset DisabledWave anyway, and they're both in Microsoft.Build.Framework.UnitTests.csproj. It would also clean up the tests somewhat.

rainersigwald · 2020-09-18T14:54:19Z

src/Build.UnitTests/ChangeWaves_Tests.cs

Copyright header

src/Build/Evaluation/Evaluator.cs

src/Build/Resources/Constants.cs

documentation/wiki/ChangeWaves-Dev.md

documentation/wiki/ChangeWaves.md

src/Build.UnitTests/ChangeWaves_Tests.cs

Forgind · 2020-09-18T19:45:58Z

src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj

Did you mean to do this?

src/Build/Evaluation/Evaluator.cs

src/Shared/ChangeWaves.cs

Forgind · 2020-09-18T19:51:37Z

src/Shared/ChangeWaves.cs

Do we need this if it's the default?

It's just a nice-to-have for easy comparison in my out-of-rotation-check function

benvillalobos · 2020-09-20T04:10:22Z

Interesting failure.

Log was expected to contain 'out of rotation', but did not. Full log:\n=======\nEvaluation started ("C:\Users\VssAdministrator\AppData\Local\Temp\3hwntpzc.jcx\Temporary80f421333ba042c887a872ec6a412cf7\proj.csproj")\r\nEvaluation finished ("C:\Users\VssAdministrator\AppData\Local\Temp\3hwntpzc.jcx\Temporary80f421333ba042c887a872ec6a412cf7\proj.csproj")\r\nBuild started.\r\nProject "proj.csproj" (default targets):\r\nBuilding with tools version "Current".\r\nTarget "HelloWorld" skipped, due to false condition; ('$(MSBUILDDISABLEFEATURESFROMVERSION)' == '16.8' and $([MSBuild]::IsChangeWaveEnabled('16.8')) == false) was evaluated as ('999.999' == '16.8' and True == false).\r\nDone building project "proj.csproj".\r\nBuild succeeded.\r\n\n=======\r\nExpected: True\r\nActual:   False

It's looking like the property is being set incorrectly? This doesn't fail locally.

Forgind · 2020-09-20T05:13:37Z

Interesting failure.

Log was expected to contain 'out of rotation', but did not. Full log:\n=======\nEvaluation started ("C:\Users\VssAdministrator\AppData\Local\Temp\3hwntpzc.jcx\Temporary80f421333ba042c887a872ec6a412cf7\proj.csproj")\r\nEvaluation finished ("C:\Users\VssAdministrator\AppData\Local\Temp\3hwntpzc.jcx\Temporary80f421333ba042c887a872ec6a412cf7\proj.csproj")\r\nBuild started.\r\nProject "proj.csproj" (default targets):\r\nBuilding with tools version "Current".\r\nTarget "HelloWorld" skipped, due to false condition; ('$(MSBUILDDISABLEFEATURESFROMVERSION)' == '16.8' and $([MSBuild]::IsChangeWaveEnabled('16.8')) == false) was evaluated as ('999.999' == '16.8' and True == false).\r\nDone building project "proj.csproj".\r\nBuild succeeded.\r\n\n=======\r\nExpected: True\r\nActual:   False

It's looking like the property is being set incorrectly? This doesn't fail locally.

Try adding BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly.

benvillalobos · 2020-09-20T19:07:54Z

@Forgind I did, and didn't notice any differences in the tests locally (examing the TestEnvironment that was created). Could be worth a shot.

Forgind · 2020-09-20T19:14:55Z

@Forgind Nathan Mytelka FTE I did, and didn't notice any differences in the tests locally (examing the TestEnvironment that was created). Could be worth a shot.

You said it didn't fail locally, and when running the tests locally, they each had their own test environment. Running them all together in CI requires the test environment to be reset.

src/Build.UnitTests/ChangeWaves_Tests.cs

src/Shared/ChangeWaves.cs

src/Build.UnitTests/ChangeWaves_Tests.cs

Maybe I should bring this into another PR?

Forgind · 2020-09-22T23:40:31Z

Any chance you could get it to only print once?

Evaluate runs even when not doing a full build, so this could turn into a lot of messages.

benvillalobos · 2020-09-23T03:22:15Z

Ideally yes. Some ideas:

Static boolean inside of ChangeWaves, HasLoggedError. The warnings in evaluator would then check that.
Condition it based on if (ChangeWaves.DisabledWave != null), it's guaranteed to be null the first time.

Though both of these solutions only work for the first build. Beyond that it wouldn't throw a warning on the same node.

A flag within evaluator that checks this?

I could theoretically call SetChangeWave at any point, so long as I have some logging context to throw a warning with.

benvillalobos · 2020-09-23T17:54:58Z

Documenting that I got sign off on the environment variable MSBuildDisableFeaturesFromVersion from @KathleenDollard via teams.

…value.

Forgind · 2020-09-23T21:56:43Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+                case ChangeWaveConversionState.OutOfRotation:
+                    log.LogWarning(BuildEventContext.Invalid, "", new BuildEventFileInfo(""), "ChangeWave_OutOfRotation", ChangeWaves.AllWaves[0], Traits.Instance.MSBuildDisableFeaturesFromVersion);
+                    break;
+                case ChangeWaveConversionState.InvalidVersion:


Did we agree to a warning here? It feels far too strong to me, since people might do this intentionally

I realize now that we did lean toward not having a warning here. Thanks for the catch

Forgind · 2020-09-23T22:28:30Z

Would you mind making a checklist of what you've done and what you still have to do?

…on and building

benvillalobos · 2020-09-24T19:06:28Z

Current status:
Hitting issues with a locally-deployed msbuild.

The warnings don't show up in the build logs (command line or in VS)
The warnings aren't output in the output window (VS)

Command line experience is fine, minus the build logs not containing the warning.

It seems like I just need to find the correct way to log warnings in within the BuildManager

src/Build.UnitTests/ChangeWaves_Tests.cs

Forgind · 2020-09-24T23:28:24Z

src/Build.UnitTests/ChangeWaves_Tests.cs

+                    TransientTestFile file = env.CreateFile("proj.csproj", projectFile);
+
+                    ProjectCollection collection = new ProjectCollection();
+                    MockLogger log = new MockLogger();


Also check whether it's correctly set for calling your API from C# (for this and the next test)

I'm not sure what there is to check? Something like ChangeWaves.DisabledVersion.ShouldBe(blah)?

Forgind · 2020-09-24T23:28:35Z

src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj

Forgind · 2020-09-24T23:38:49Z

src/Shared/ChangeWaves.cs

+            Version changeWave;
+
+            // If unset, enable all features.
+            if (DisabledWave.Length == 0)


nit:
convert to switch statement and reorder according to which you think will be used most often. (So EnableAllFeaturesAsVersion at top, for instance.

A switch statement based on what? I do agree this can be implemented better.

Forgind · 2020-09-24T23:41:39Z

src/Shared/ChangeWaves.cs

+            // Ensure it's set to an existing version within the current rotation
+            if (!AllWavesAsVersion.Contains(changeWave))
+            {
+                foreach (Version wave in AllWavesAsVersion)


This can be part of the if/else chain or switch statement, right? Also, why do you have to loop through them? Can't you just set it to whatever it is, since < (AreFeaturesEnabled) works the same either way?

This loop is to find the next available version, because we only get here if:

It is a valid version between the bounds

it isn't already in the array
So we need to find the next highest one to clamp it to.

src/Shared/ChangeWaves.cs

src/Shared/Traits.cs

src/Shared/UnitTests/TestEnvironment.cs

… eval time

Enable change waves. Added static ChangeWaves class that contains AreFeaturesEnabled and ApplyChangeWave. Documentation is in the wiki.

Makes three PRs (#5669, #5625, and #5653) use Change Waves (#5710) to opt out as a unit. Also added a test to ensure that change waves worked properly for one of the three PRs and corrected a minor issue with tests for changes under a change wave that assume it is enabled when there exist tests that disable the relevant change wave without resetting it to empty at the end.

benvillalobos added the WIP label Sep 4, 2020

cdmihai reviewed Sep 4, 2020

View reviewed changes

benvillalobos removed the WIP label Sep 17, 2020

benvillalobos force-pushed the change-waves branch from 686398b to 60e2328 Compare September 18, 2020 06:35

rainersigwald reviewed Sep 18, 2020

View reviewed changes

Forgind reviewed Sep 18, 2020

View reviewed changes

Forgind reviewed Sep 20, 2020

View reviewed changes

src/Build.UnitTests/ChangeWaves_Tests.cs Outdated Show resolved Hide resolved

Forgind reviewed Sep 21, 2020

View reviewed changes

src/Shared/ChangeWaves.cs Outdated Show resolved Hide resolved

src/Build.UnitTests/ChangeWaves_Tests.cs Outdated Show resolved Hide resolved

benvillalobos and others added 10 commits September 22, 2020 13:32

Initial implementation of changewaves

5dab044

Changing to use the Version class. Added tests

b4ef40b

cleanup, tests, comments

b3383e5

Tests, throw on invalid call, change env variable.

cc46fb4

Change wave dev documentation

7909b6e

Maybe I should bring this into another PR?

Enum WIP

90a2dbc

Unit tests. Global property

bc42a01

allow setting changewave trait. sanitize the wave

64663ea

Remove enum return type. More documentation

7077e3a

Tests complete. Reworked wave setup

952f5b3

marcpopMSFT added this to the MSBuild 16.8 milestone Sep 23, 2020

Rename 'IsFeatureEnabled' to 'AreFeaturesEnabled'

23802fd

benvillalobos added 2 commits September 23, 2020 10:56

Change waves higher than the highest wave get clamped to the highest …

dcb4868

…value.

Update warning messages

6d018e3

Forgind mentioned this pull request Sep 23, 2020

Use change waves for new features #5751

Closed

benvillalobos added 2 commits September 23, 2020 13:49

warning on invalid versions. added test for it.

9838fce

Throwing warning at buildmanager.BeginBuild

54f6d5d

This was referenced Sep 23, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

Forgind reviewed Sep 23, 2020

View reviewed changes

benvillalobos added 3 commits September 23, 2020 16:39

remove warning on invalidversion

429008f

Working around weird test quirks, such as the timing between evaluati…

ebb59bc

…on and building

Remove using

0402024

benvillalobos added 3 commits September 24, 2020 15:22

State is persistent

0f72c69

Revert to one warning per project

17f4f0d

Fix warning message to show correct defaulted version

bee6d6c

Forgind approved these changes Sep 24, 2020

View reviewed changes

benvillalobos added 3 commits September 24, 2020 17:04

PR Feedback

ec8d934

PR feedback. taking warning count into account

75d6c63

Apply change wave when AreFeatureAreEnabled is called before build or…

474fb4a

… eval time

benvillalobos merged commit cda0060 into dotnet:master Sep 25, 2020

Forgind pushed a commit to Forgind/msbuild that referenced this pull request Sep 25, 2020

Enable Change Waves (dotnet#5710)

c787df3

Enable change waves. Added static ChangeWaves class that contains AreFeaturesEnabled and ApplyChangeWave. Documentation is in the wiki.

benvillalobos mentioned this pull request Sep 25, 2020

Change Waves Refactoring #5756

Closed

10 tasks

	Sometimes we want to make a breaking change _and_ give customers a heads up as to what's breaking. So we develop the change and provide an opt-out while acknowledging that this feature will become a standard functionality down the line.
	Sometimes we want to make a breaking change _and_ give customers a heads up as to what's breaking. So we develop the change and provide an opt-out while acknowledging that this feature will become standard functionality down the line.

Conversation

benvillalobos commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!