Skip to content
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

Toolset configuration net5.0 #6220

Merged
merged 20 commits into from Mar 11, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions eng/Packages.props
Expand Up @@ -18,6 +18,7 @@
<PackageReference Update="SourceLink.Create.CommandLine" Version="2.1.2" />
<PackageReference Update="System.CodeDom" Version="4.4.0" />
<PackageReference Update="System.Collections.Immutable" Version="5.0.0" />
<PackageReference Update="System.Configuration.ConfigurationManager" Version="4.7.0" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we own this? Also thinking we should try running this through RPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this is my first time making a PR in MSBuild so I'm not familiar with the process. Mihai created an exp/* branch for me here and it triggered an VS insertion and that passed. Is that RPS or do we need to run something else for it?

And I do think we own the package as shown here. The package is owned by Microsoft and dotnetframework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! That is what I was thinking of with RPS. It failed symbol check, but I'm assuming that was innocuous.

I should have clarified that I was wondering if MSBuild owns S.C.CM. I hit a problem at one point when MSBuild updated a package that Roslyn owned, and we had mismatching versions with them. I think that sort of problem would have been caught by RPS, so I think this is good? But I don't feel very confident about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up to our convo this morning: I talked to @cdmihai and he's not sure about who owns this package exactly either, but believes it should be fine since it is not referenced in VS's config.corext.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found AssemblyVersions.tt, and it isn't referenced there either.

<PackageReference Update="System.Memory" Version="4.5.4" />
<PackageReference Update="System.Reflection.Metadata" Version="1.6.0" />
<PackageReference Update="System.Resources.Extensions" Version="4.6.0" />
Expand Down
@@ -1,10 +1,7 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#if FEATURE_SYSTEM_CONFIGURATION

using System.Configuration;
using Microsoft.Win32;
using Microsoft.Build.Collections;
using Microsoft.Build.Evaluation;
using Microsoft.Build.Execution;
Expand Down Expand Up @@ -683,4 +680,3 @@ private ToolsetConfigurationReader GetStandardConfigurationReader()

}
}
#endif
2 changes: 0 additions & 2 deletions src/Build.UnitTests/Definition/ToolsetReader_Tests.cs
Expand Up @@ -3,9 +3,7 @@

using System;
using System.Collections.Generic;
#if FEATURE_SYSTEM_CONFIGURATION
using System.Configuration;
#endif
using System.IO;

using Microsoft.Build.Collections;
Expand Down
@@ -1,10 +1,6 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#if FEATURE_SYSTEM_CONFIGURATION

using System.Configuration;
using Microsoft.Win32;
using Microsoft.Build.Collections;
using Microsoft.Build.Evaluation;
using Microsoft.Build.Exceptions;
Expand All @@ -15,7 +11,6 @@
using System;
using System.Collections.Generic;
using System.IO;
using Microsoft.Build.UnitTests;

namespace Microsoft.Build.UnitTests.Evaluation
{
Expand Down Expand Up @@ -52,7 +47,11 @@ public void ImportFromExtensionsPathNotFound()
extnDir1 = GetNewExtensionsPathAndCreateFile("extensions1", Path.Combine("foo", "extn.proj"), GetExtensionTargetsFileContent1());
mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", GetMainTargetFileContent());

#if FEATURE_SYSTEM_CONFIGURATION
var projColln = new ProjectCollection();
#else
var projColln = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to extract this to a new method GetProjectCollection() to avoid repeating this code in multiple places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops forgot to reply to this one but this is a good call I made this change as well 😃

projColln.ResetToolsetsForTests(WriteConfigFileAndGetReader("MSBuildExtensionsPath", extnDir1, Path.Combine("tmp", "nonexistent")));
var logger = new MockLogger();
projColln.RegisterLogger(logger);
Expand Down Expand Up @@ -297,7 +296,11 @@ public void ImportFromExtensionsPathInvalidFile()
extnDir1 = GetNewExtensionsPathAndCreateFile("extensions1", Path.Combine("foo", "extn.proj"), extnTargetsFileContent);
mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", GetMainTargetFileContent());

#if FEATURE_SYSTEM_CONFIGURATION
var projColln = new ProjectCollection();
#else
var projColln = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
#endif
projColln.ResetToolsetsForTests(WriteConfigFileAndGetReader("MSBuildExtensionsPath", extnDir1,
Path.Combine("tmp", "nonexistent")));
var logger = new MockLogger();
Expand Down Expand Up @@ -396,7 +399,11 @@ public void ImportFromExtensionsPathSearchOrder2()

// MSBuildExtensionsPath* property value has highest priority for the lookups
try {
#if FEATURE_SYSTEM_CONFIGURATION
var projColln = new ProjectCollection();
#else
var projColln = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
#endif
projColln.ResetToolsetsForTests(WriteConfigFileAndGetReader("MSBuildExtensionsPath", Path.Combine("tmp", "non-existent"), extnDir1));
var logger = new MockLogger();
projColln.RegisterLogger(logger);
Expand Down Expand Up @@ -485,7 +492,12 @@ public void ImportFromExtensionsPathAnd32And64()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(String.Format(configFileContents, extnDir1, extnDir2, extnDir3));
var reader = GetStandardConfigurationReader();

#if FEATURE_SYSTEM_CONFIGURATION
var projColln = new ProjectCollection();
#else
var projColln = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
#endif

projColln.ResetToolsetsForTests(reader);
var logger = new MockLogger();
projColln.RegisterLogger(logger);
Expand Down Expand Up @@ -559,7 +571,11 @@ public void ExpandExtensionsPathFallback()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
var reader = GetStandardConfigurationReader();

var projectCollection = new ProjectCollection(new Dictionary<string, string> {["FallbackExpandDir1"] = extnDir1});
#if FEATURE_SYSTEM_CONFIGURATION
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 });
#else
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 }, null, ToolsetDefinitionLocations.ConfigurationFile);
#endif

projectCollection.ResetToolsetsForTests(reader);
var logger = new MockLogger();
Expand Down Expand Up @@ -620,7 +636,11 @@ public void ExpandExtensionsPathFallbackInErrorMessage()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
var reader = GetStandardConfigurationReader();

#if FEATURE_SYSTEM_CONFIGURATION
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 });
#else
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 }, null, ToolsetDefinitionLocations.ConfigurationFile);
#endif

projectCollection.ResetToolsetsForTests(reader);
var logger = new MockLogger();
Expand Down Expand Up @@ -690,7 +710,11 @@ public void FallbackImportWithIndirectReference()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
var reader = GetStandardConfigurationReader();

#if FEATURE_SYSTEM_CONFIGURATION
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 });
#else
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 }, null, ToolsetDefinitionLocations.ConfigurationFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the argument name before null so we know what parameter it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, I added the argument name in the GetProjectCollection function

#endif

projectCollection.ResetToolsetsForTests(reader);
var logger = new MockLogger();
Expand Down Expand Up @@ -755,7 +779,11 @@ public void FallbackImportWithUndefinedProperty()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
var reader = GetStandardConfigurationReader();

#if FEATURE_SYSTEM_CONFIGURATION
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 });
#else
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 }, null, ToolsetDefinitionLocations.ConfigurationFile);
#endif

projectCollection.ResetToolsetsForTests(reader);
var logger = new MockLogger();
Expand Down Expand Up @@ -814,7 +842,11 @@ public void FallbackImportWithFileNotFoundWhenPropertyNotDefined()
ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
var reader = GetStandardConfigurationReader();

#if FEATURE_SYSTEM_CONFIGURATION
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 });
#else
var projectCollection = new ProjectCollection(new Dictionary<string, string> { ["FallbackExpandDir1"] = extnDir1 }, null, ToolsetDefinitionLocations.ConfigurationFile);
#endif

projectCollection.ResetToolsetsForTests(reader);
var logger = new MockLogger();
Expand Down Expand Up @@ -865,7 +897,11 @@ void CreateAndBuildProjectForImportFromExtensionsPath(string extnPathPropertyNam
Action<Project, MockLogger> action)
{
try {
#if FEATURE_SYSTEM_CONFIGURATION
var projColln = new ProjectCollection();
#else
var projColln = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
#endif
projColln.ResetToolsetsForTests(WriteConfigFileAndGetReader(extnPathPropertyName, extnDirs));
var logger = new MockLogger();
projColln.RegisterLogger(logger);
Expand Down Expand Up @@ -977,4 +1013,3 @@ private ToolsetConfigurationReader GetStandardConfigurationReader()
}
}
}
#endif
79 changes: 79 additions & 0 deletions src/Build.UnitTests/Evaluation/ToolsetConfigurationNet5_Tests.cs
@@ -0,0 +1,79 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#if !FEATURE_SYSTEM_CONFIGURATION
/* This test is designed especially to test Configuration parsing in net5.0
* which means it WON'T work in net472 and thus we don't run it in net472 */

using Microsoft.Build.Evaluation;
using Microsoft.Build.Execution;

using Xunit;
using System.Collections.Generic;

namespace Microsoft.Build.UnitTests.Evaluation
{
/// <summary>
/// Unit tests for MSBuild Net5.0 Configuration Parsing
/// </summary>
public class ToolsetConfigurationNet5Test
{
[Fact]
// The default ToolsetDefintionLocations is None, which results in only the local which results in only the several included
// paths such as SDK path and RoslynTargetPath and nothing else. This behavior is expected and the exact same as before.
public void ToolsetDefinitionLocationsIsDefault()
{
var projectCollection = new ProjectCollection();
IDictionary<string, string> toolsetProperties
= new Dictionary<string, string>();

foreach (Toolset toolset in projectCollection.Toolsets)
{
foreach (KeyValuePair<string, ProjectPropertyInstance> properties in toolset.Properties)
{
toolsetProperties[properties.Value.Name] = properties.Value.EvaluatedValue;
}
}

Assert.True(toolsetProperties.ContainsKey("MSBuildSDKsPath"));
Assert.True(toolsetProperties.ContainsKey("RoslynTargetsPath"));
Assert.NotEqual(string.Empty, toolsetProperties["MSBuildSDKsPath"]);
Assert.NotEqual(string.Empty, toolsetProperties["RoslynTargetsPath"]);

Assert.False(toolsetProperties.ContainsKey("VCTargetsPath"));
Assert.False(toolsetProperties.ContainsKey("MSBuildToolsRoot"));
Assert.False(toolsetProperties.ContainsKey("MSBuildExtensionsPath"));
}

[Fact]
// With ToolsetDefintionLocations set to ConfigurationFile (Which would only happen in net5.0 if the user decides to set it).
// Most toolsets are available and the MsBuildTools and SDK paths are all in the net5.0 runtime.
public void ToolsetDefinitionLocationsIsConfiguration()
{
var projectCollection = new ProjectCollection(ToolsetDefinitionLocations.ConfigurationFile);
IDictionary<string, string> toolsetProperties
= new Dictionary<string, string>();

foreach (Toolset toolset in projectCollection.Toolsets)
{
foreach (KeyValuePair<string, ProjectPropertyInstance> properties in toolset.Properties)
{
toolsetProperties[properties.Value.Name] = properties.Value.EvaluatedValue;
}
}

Assert.True(toolsetProperties.ContainsKey("MSBuildSDKsPath"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked into using Shouldly? Some other tests use Shouldly, if you just search for it for examples. It's better than the standard asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to Shouldly

Assert.True(toolsetProperties.ContainsKey("RoslynTargetsPath"));
Assert.NotEqual(string.Empty, toolsetProperties["MSBuildSDKsPath"]);
Assert.NotEqual(string.Empty, toolsetProperties["RoslynTargetsPath"]);

Assert.True(toolsetProperties.ContainsKey("VCTargetsPath"));
Assert.True(toolsetProperties.ContainsKey("MSBuildToolsRoot"));
Assert.True(toolsetProperties.ContainsKey("MSBuildExtensionsPath"));
Assert.NotEqual(string.Empty, toolsetProperties["VCTargetsPath"]);
Assert.NotEqual(string.Empty, toolsetProperties["MSBuildToolsRoot"]);
Assert.NotEqual(string.Empty, toolsetProperties["MSBuildExtensionsPath"]);
}
}
}
#endif
8 changes: 2 additions & 6 deletions src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hyper-Nit: Is this a BOM change from a text editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep seems like it... Though I thought I only opened it in VS so it's odd but it should be fixed now 👍


<PropertyGroup>
<TargetFrameworks>$(RuntimeOutputTargetFrameworks)</TargetFrameworks>
Expand All @@ -16,6 +16,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="System.Configuration.ConfigurationManager" />
<PackageReference Include="Shouldly" />
<PackageReference Include="Microsoft.CodeAnalysis.Build.Tasks" />
<PackageReference Include="NuGet.Frameworks" >
Expand Down Expand Up @@ -44,17 +45,12 @@
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(MonoBuild)' == 'true'">TargetFramework=$(FullFrameworkTFM)</SetTargetFramework>
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">TargetFramework=net5.0</SetTargetFramework>
</ProjectReference>

<Reference Include="System.Configuration" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />
</ItemGroup>

<ItemGroup>
<Compile Include="..\Shared\FxCopExclusions\Microsoft.Build.Shared.Suppressions.cs">
<ExcludeFromStyleCop>true</ExcludeFromStyleCop>
</Compile>

<Compile Remove="Definition\ToolsetConfigurationReaderTestHelper.cs" />
<Compile Include="Definition\ToolsetConfigurationReaderTestHelper.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />

<Compile Include="..\Shared\UnitTests\BuildEventArgsExtension.cs">
<!-- Extension methods -->
Expand Down
6 changes: 0 additions & 6 deletions src/Build/Definition/ProjectCollection.cs
Expand Up @@ -1729,15 +1729,13 @@ private void CreateLoggingService(int maxCPUCount, bool onlyLogCriticalEvents)
_loggingService.OnlyLogCriticalEvents = onlyLogCriticalEvents;
}

#if FEATURE_SYSTEM_CONFIGURATION
/// <summary>
/// Reset the toolsets using the provided toolset reader, used by unit tests
/// </summary>
internal void ResetToolsetsForTests(ToolsetConfigurationReader configurationReaderForTestsOnly)
{
InitializeToolsetCollection(configReader:configurationReaderForTestsOnly);
}
#endif

#if FEATURE_WIN32_REGISTRY
/// <summary>
Expand All @@ -1757,9 +1755,7 @@ internal void ResetToolsetsForTests(ToolsetRegistryReader registryReaderForTests
#if FEATURE_WIN32_REGISTRY
ToolsetRegistryReader registryReader = null,
#endif
#if FEATURE_SYSTEM_CONFIGURATION
ToolsetConfigurationReader configReader = null
#endif
)
{
_toolsets = new Dictionary<string, Toolset>(StringComparer.OrdinalIgnoreCase);
Expand All @@ -1769,9 +1765,7 @@ internal void ResetToolsetsForTests(ToolsetRegistryReader registryReaderForTests
#if FEATURE_WIN32_REGISTRY
registryReader,
#endif
#if FEATURE_SYSTEM_CONFIGURATION
configReader,
#endif
EnvironmentProperties, _globalProperties, ToolsetLocations);

_toolsetsVersion++;
Expand Down
4 changes: 4 additions & 0 deletions src/Build/Definition/ToolsetConfigurationReader.cs
Expand Up @@ -253,7 +253,11 @@ private static Configuration ReadApplicationConfiguration()
{
// When running from the command-line or from VS, use the msbuild.exe.config file.
if (BuildEnvironmentHelper.Instance.Mode != BuildEnvironmentMode.None &&
// This FEATURE_SYSTEM_CONFIGURATION is needed as OpenExeConfiguration for net5.0 works differently, without this condition unit tests won't pass.
// ConfigurationManager.OpenExeConfiguration in net5.0 will find testhost.exe instead which does not contain any configuration and therefore fail.
#if FEATURE_SYSTEM_CONFIGURATION
!BuildEnvironmentHelper.Instance.RunningTests &&
#endif
FileSystems.Default.FileExists(BuildEnvironmentHelper.Instance.CurrentMSBuildConfigurationFile))
{
var configFile = new ExeConfigurationFileMap { ExeConfigFilename = BuildEnvironmentHelper.Instance.CurrentMSBuildConfigurationFile };
Expand Down
8 changes: 0 additions & 8 deletions src/Build/Definition/ToolsetReader.cs
Expand Up @@ -76,7 +76,6 @@ internal abstract class ToolsetReader
get;
}

#if FEATURE_WIN32_REGISTRY || FEATURE_SYSTEM_CONFIGURATION
/// <summary>
/// Gathers toolset data from the registry and configuration file, if any:
/// allows you to specify which of the registry and configuration file to
Expand All @@ -88,12 +87,9 @@ internal static string ReadAllToolsets(Dictionary<string, Toolset> toolsets, Pro
#if FEATURE_WIN32_REGISTRY
null,
#endif
#if FEATURE_SYSTEM_CONFIGURATION
null,
#endif
environmentProperties, globalProperties, locations);
}
#endif

/// <summary>
/// Gathers toolset data from the registry and configuration file, if any.
Expand All @@ -105,9 +101,7 @@ internal static string ReadAllToolsets
#if FEATURE_WIN32_REGISTRY
ToolsetRegistryReader registryReader,
#endif
#if FEATURE_SYSTEM_CONFIGURATION
ToolsetConfigurationReader configurationReader,
#endif
PropertyDictionary<ProjectPropertyInstance> environmentProperties,
PropertyDictionary<ProjectPropertyInstance> globalProperties,
ToolsetDefinitionLocations locations
Expand All @@ -124,7 +118,6 @@ ToolsetDefinitionLocations locations
string overrideTasksPathFromConfiguration = null;
string defaultOverrideToolsVersionFromConfiguration = null;

#if FEATURE_SYSTEM_CONFIGURATION
if ((locations & ToolsetDefinitionLocations.ConfigurationFile) == ToolsetDefinitionLocations.ConfigurationFile)
{
if (configurationReader == null)
Expand All @@ -137,7 +130,6 @@ ToolsetDefinitionLocations locations
initialProperties, true /* accumulate properties */, out overrideTasksPathFromConfiguration,
out defaultOverrideToolsVersionFromConfiguration);
}
#endif

string defaultToolsVersionFromRegistry = null;
string overrideTasksPathFromRegistry = null;
Expand Down