From 5da17bea55493dc66d56657bcd87d66c31c676ca Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Mon, 28 Jun 2021 15:45:48 -0700 Subject: [PATCH 1/3] ChangeWave 16.8 transitions to default behavior --- .../Evaluation/Expander_Tests.cs | 70 ------------------- src/Build/Evaluation/Expander.cs | 2 +- .../LazyItemEvaluator.IncludeOperation.cs | 2 +- src/Framework/ChangeWaves.cs | 3 +- src/Shared/NativeMethodsShared.cs | 3 - .../Microsoft.Common.CurrentVersion.targets | 2 +- 6 files changed, 4 insertions(+), 78 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 71d15858f3c..5c949dfc17b 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -1547,76 +1547,6 @@ public void ExpandAllIntoStringTruncated() Assert.Equal(expected, expander.ExpandIntoStringAndUnescape(xmlattribute.Value, ExpanderOptions.ExpandAll | ExpanderOptions.Truncate, MockElementLocation.Instance)); } - /// - /// Exercises ExpandIntoStringAndUnescape and ExpanderOptions.Truncate - /// - [Fact] - public void ExpandAllIntoStringNotTruncated() - { - using (TestEnvironment env = TestEnvironment.Create()) - { - ChangeWaves.ResetStateForTests(); - env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave16_8.ToString()); - BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); - ProjectInstance project = ProjectHelpers.CreateEmptyProjectInstance(); - var manySpaces = "".PadLeft(2000); - var pg = new PropertyDictionary(); - pg.Set(ProjectPropertyInstance.Create("ManySpacesProperty", manySpaces)); - var itemMetadataTable = new Dictionary(StringComparer.OrdinalIgnoreCase) - { - { "ManySpacesMetadata", manySpaces } - }; - var itemMetadata = new StringMetadataTable(itemMetadataTable); - var projectItemGroups = new ItemDictionary(); - var itemGroup = new List(); - StringBuilder longFileName = new StringBuilder(); - StringBuilder longMetadataName = new StringBuilder(); - for (int i = 0; i < 50; i++) - { - var item = new ProjectItemInstance(project, "ManyItems", $"ThisIsAFairlyLongFileName_{i}.bmp", project.FullPath); - item.SetMetadata("Foo", $"ThisIsAFairlyLongMetadataValue_{i}"); - longFileName.Append($"ThisIsAFairlyLongFileName_{i}.bmp" + (i == 49 ? string.Empty : ";")); - longMetadataName.Append($"ThisIsAFairlyLongMetadataValue_{i}" + (i == 49 ? string.Empty : ";")); - itemGroup.Add(item); - } - var lookup = new Lookup(projectItemGroups, pg); - lookup.EnterScope("x"); - lookup.PopulateWithItems("ManySpacesItem", new[] - { - new ProjectItemInstance (project, "ManySpacesItem", "Foo", project.FullPath), - new ProjectItemInstance (project, "ManySpacesItem", manySpaces, project.FullPath), - new ProjectItemInstance (project, "ManySpacesItem", "Bar", project.FullPath), - }); - lookup.PopulateWithItems("Exactly1024", new[] - { - new ProjectItemInstance (project, "Exactly1024", "".PadLeft(1024), project.FullPath), - new ProjectItemInstance (project, "Exactly1024", "Foo", project.FullPath), - }); - lookup.PopulateWithItems("ManyItems", itemGroup); - - Expander expander = new Expander(lookup, lookup, itemMetadata, FileSystems.Default); - - XmlAttribute xmlattribute = (new XmlDocument()).CreateAttribute("dummy"); - xmlattribute.Value = "'%(ManySpacesMetadata)' != '' and '$(ManySpacesProperty)' != '' and '@(ManySpacesItem)' != '' and '@(Exactly1024)' != '' and '@(ManyItems)' != '' and '@(ManyItems->'%(Foo)')' != '' and '@(ManyItems->'%(Nonexistent)')' != ''"; - - var expected = - $"'{"",2000}' != '' and " + - $"'{"",2000}' != '' and " + - $"'Foo;{"",2000};Bar' != '' and " + - $"'{"",1024};Foo' != '' and " + - $"'{longFileName}' != '' and " + - $"'{longMetadataName}' != '' and " + - "';;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;' != ''"; - var actual = expander.ExpandIntoStringAndUnescape(xmlattribute.Value, ExpanderOptions.ExpandAll | ExpanderOptions.Truncate, MockElementLocation.Instance); - // NOTE: semicolons in the last part are *weird* because they don't actually mean anything and you get logging like - // Target "Build" skipped, due to false condition; ( '@(I->'%(nonexistent)')' == '' ) was evaluated as ( ';' == '' ). - // but that goes back to MSBuild 4.something so I'm codifying it in this test. If you're here because you cleaned it up - // and want to fix the test my current opinion is that's fine. - actual.ShouldBe(expected); - ChangeWaves.ResetStateForTests(); - } - } - /// /// Exercises ExpandAllIntoString with a string that does not need expanding. /// In this case the expanded string should be reference identical to the passed in string. diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 80ddea0b0c2..718b8ab9395 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -610,7 +610,7 @@ private static bool IsValidPropertyName(string propertyName) /// private static bool IsTruncationEnabled(ExpanderOptions options) { - return (options & ExpanderOptions.Truncate) != 0 && !Traits.Instance.EscapeHatches.DoNotTruncateConditions && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_8); + return (options & ExpanderOptions.Truncate) != 0 && !Traits.Instance.EscapeHatches.DoNotTruncateConditions; } /// diff --git a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs index 92fecfb7e36..a351b02dbbe 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs @@ -94,7 +94,7 @@ protected override ImmutableList SelectItems(ImmutableList.Builder { // If this item is behind a false condition and represents a full drive/filesystem scan, expanding it is // almost certainly undesired. It should be skipped to avoid evaluation taking an excessive amount of time. - bool skipGlob = !_conditionResult && globFragment.IsFullFileSystemScan && !Traits.Instance.EscapeHatches.AlwaysEvaluateDangerousGlobs && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_8); + bool skipGlob = !_conditionResult && globFragment.IsFullFileSystemScan && !Traits.Instance.EscapeHatches.AlwaysEvaluateDangerousGlobs; if (!skipGlob) { string glob = globFragment.TextFragment; diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index 7761c468d44..d97becfa1d0 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -22,10 +22,9 @@ internal enum ChangeWaveConversionState /// For dev docs: https://github.com/dotnet/msbuild/blob/master/documentation/wiki/ChangeWaves-Dev.md internal class ChangeWaves { - internal static readonly Version Wave16_8 = new Version(16, 8); internal static readonly Version Wave16_10 = new Version(16, 10); internal static readonly Version Wave17_0 = new Version(17, 0); - internal static readonly Version[] AllWaves = { Wave16_8, Wave16_10, Wave17_0 }; + internal static readonly Version[] AllWaves = { Wave16_10, Wave17_0 }; /// /// Special value indicating that all features behind all Change Waves should be enabled. diff --git a/src/Shared/NativeMethodsShared.cs b/src/Shared/NativeMethodsShared.cs index 42e8a3ead07..4e23aae1096 100644 --- a/src/Shared/NativeMethodsShared.cs +++ b/src/Shared/NativeMethodsShared.cs @@ -514,9 +514,6 @@ public static int GetLogicalCoreCount() // https://github.com/dotnet/runtime/issues/29686 // so always double-check it. if (IsWindows -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_8) -#endif #if NETFRAMEWORK // .NET Framework calls Windows APIs that have a core count limit (32/64 depending on process bitness). // So if we get a high core count on full framework, double-check it. diff --git a/src/Tasks/Microsoft.Common.CurrentVersion.targets b/src/Tasks/Microsoft.Common.CurrentVersion.targets index 18c950d23fc..154589de371 100644 --- a/src/Tasks/Microsoft.Common.CurrentVersion.targets +++ b/src/Tasks/Microsoft.Common.CurrentVersion.targets @@ -642,7 +642,7 @@ Copyright (C) Microsoft Corporation. All rights reserved. - + $(NoWarn) $(WarningsAsErrors) From 9020161b00b06de49a6f69531d194673e3a0e638 Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Wed, 30 Jun 2021 13:26:11 -0700 Subject: [PATCH 2/3] Add section for describing waves rotating out. Move 16.8 to section of rotated change waves. --- documentation/wiki/ChangeWaves.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 66b119cd485..bc5a5dd7c28 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -7,6 +7,9 @@ Opt-out is a better approach for us because we'd likely get limited feedback whe ## How do they work? The opt-out comes in the form of setting the environment variable `MSBuildDisableFeaturesFromVersion` to the Change Wave (or version) that contains the feature you want **disabled**. This version happens to be the version of MSBuild that the features were developed for. See the mapping of change waves to features below. +## When do they become permanent? +A wave of features is set to "rotate out" (ie. become standard functionality) two bands after its release. For example, wave 16.8 stayed opt-out through wave 16.10, becoming standard functionalty when wave 17.0 is introduced. + ## MSBuildDisableFeaturesFromVersion Values & Outcomes | `MSBuildDisableFeaturesFromVersion` Value | Result | Receive Warning? | | :------------- | :---------- | :----------: | @@ -19,10 +22,6 @@ The opt-out comes in the form of setting the environment variable `MSBuildDisabl # Change Waves & Associated Features ## Current Rotation of Change Waves -### 16.8 -- [Enable NoWarn](https://github.com/dotnet/msbuild/pull/5671) -- [Truncate Target/Task skipped log messages to 1024 chars](https://github.com/dotnet/msbuild/pull/5553) -- [Don't expand full drive globs with false condition](https://github.com/dotnet/msbuild/pull/5669) ### 16.10 - [Error when a property expansion in a condition has whitespace](https://github.com/dotnet/msbuild/pull/5672) - [Allow Custom CopyToOutputDirectory Location With TargetPath](https://github.com/dotnet/msbuild/pull/6237) @@ -31,3 +30,7 @@ The opt-out comes in the form of setting the environment variable `MSBuildDisabl ### 17.0 ## Change Waves No Longer In Rotation +### 16.8 +- [Enable NoWarn](https://github.com/dotnet/msbuild/pull/5671) +- [Truncate Target/Task skipped log messages to 1024 chars](https://github.com/dotnet/msbuild/pull/5553) +- [Don't expand full drive globs with false condition](https://github.com/dotnet/msbuild/pull/5669) \ No newline at end of file From 7e56a3e68873790f0a51c0dcce13aac3c6323d8f Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Wed, 30 Jun 2021 13:35:22 -0700 Subject: [PATCH 3/3] Update documentation/wiki/ChangeWaves.md Co-authored-by: Rainer Sigwald --- documentation/wiki/ChangeWaves.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index bc5a5dd7c28..ee4aa76dcad 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -8,7 +8,7 @@ Opt-out is a better approach for us because we'd likely get limited feedback whe The opt-out comes in the form of setting the environment variable `MSBuildDisableFeaturesFromVersion` to the Change Wave (or version) that contains the feature you want **disabled**. This version happens to be the version of MSBuild that the features were developed for. See the mapping of change waves to features below. ## When do they become permanent? -A wave of features is set to "rotate out" (ie. become standard functionality) two bands after its release. For example, wave 16.8 stayed opt-out through wave 16.10, becoming standard functionalty when wave 17.0 is introduced. +A wave of features is set to "rotate out" (i.e. become standard functionality) two bands after its release. For example, wave 16.8 stayed opt-out through wave 16.10, becoming standard functionalty when wave 17.0 is introduced. ## MSBuildDisableFeaturesFromVersion Values & Outcomes | `MSBuildDisableFeaturesFromVersion` Value | Result | Receive Warning? | @@ -33,4 +33,4 @@ A wave of features is set to "rotate out" (ie. become standard functionality) tw ### 16.8 - [Enable NoWarn](https://github.com/dotnet/msbuild/pull/5671) - [Truncate Target/Task skipped log messages to 1024 chars](https://github.com/dotnet/msbuild/pull/5553) -- [Don't expand full drive globs with false condition](https://github.com/dotnet/msbuild/pull/5669) \ No newline at end of file +- [Don't expand full drive globs with false condition](https://github.com/dotnet/msbuild/pull/5669)