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 all 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

Large diffs are not rendered by default.

80 changes: 80 additions & 0 deletions src/Build.UnitTests/Evaluation/ToolsetConfigurationNet5_Tests.cs
@@ -0,0 +1,80 @@
// 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;
using Shouldly;

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;
}
}

toolsetProperties.ShouldContainKey("MSBuildSDKsPath");
toolsetProperties.ShouldContainKey("RoslynTargetsPath");
toolsetProperties["MSBuildSDKsPath"].ShouldNotBeNullOrEmpty();
toolsetProperties["RoslynTargetsPath"].ShouldNotBeNullOrEmpty();

toolsetProperties.ShouldNotContainKey("VCTargetsPath");
toolsetProperties.ShouldNotContainKey("MSBuildToolsRoot");
toolsetProperties.ShouldNotContainKey("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;
}
}

toolsetProperties.ShouldContainKey("MSBuildSDKsPath");
toolsetProperties.ShouldContainKey("RoslynTargetsPath");
toolsetProperties["MSBuildSDKsPath"].ShouldNotBeNullOrEmpty();
toolsetProperties["RoslynTargetsPath"].ShouldNotBeNullOrEmpty();

toolsetProperties.ShouldContainKey("VCTargetsPath");
toolsetProperties.ShouldContainKey("MSBuildToolsRoot");
toolsetProperties.ShouldContainKey("MSBuildExtensionsPath");
toolsetProperties["VCTargetsPath"].ShouldNotBeNullOrEmpty();
toolsetProperties["MSBuildToolsRoot"].ShouldNotBeNullOrEmpty();
toolsetProperties["MSBuildExtensionsPath"].ShouldNotBeNullOrEmpty();
}
}
}
#endif
6 changes: 1 addition & 5 deletions src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj
Expand Up @@ -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
5 changes: 1 addition & 4 deletions src/Build/Definition/ToolsetRegistryReader.cs
Expand Up @@ -3,11 +3,8 @@

#if FEATURE_WIN32_REGISTRY

using Microsoft.Win32;
using System;
using System.Collections.Generic;
using System.IO;
using System.Security;

using Microsoft.Build.Shared;
using error = Microsoft.Build.Shared.ErrorUtilities;
Expand Down Expand Up @@ -346,4 +343,4 @@ private static string GetValue(RegistryKeyWrapper wrapper, string valueName)
}
}
}
#endif
#endif
6 changes: 3 additions & 3 deletions src/Build/Microsoft.Build.csproj
Expand Up @@ -31,6 +31,7 @@
<ProjectReference Include="..\StringTools\StringTools.csproj" />

<PackageReference Include="System.Collections.Immutable" />
<PackageReference Include="System.Configuration.ConfigurationManager" />
<PackageReference Include="System.Threading.Tasks.Dataflow" />
<PackageReference Include="System.Text.Json" />

Expand All @@ -39,7 +40,6 @@

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
<PackageReference Include="Microsoft.VisualStudio.Setup.Configuration.Interop" />
<Reference Include="System.Configuration" />
<Reference Include="System.IO.Compression" />
<PackageReference Include="System.Memory" />
</ItemGroup>
Expand Down Expand Up @@ -419,8 +419,8 @@
<Compile Include="Definition\ResolvedImport.cs" />
<Compile Include="Definition\SubToolset.cs" />
<Compile Include="Definition\Toolset.cs" />
<Compile Include="Definition\ToolsetConfigurationReader.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />
<Compile Include="..\Shared\ToolsetElement.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
<Compile Include="Definition\ToolsetConfigurationReader.cs" />
<Compile Include="..\Shared\ToolsetElement.cs">
<ExcludeFromStyleCop>true</ExcludeFromStyleCop>
</Compile>
<Compile Include="Definition\ToolsetPropertyDefinition.cs" />
Expand Down
4 changes: 0 additions & 4 deletions src/Build/Utilities/RegistryKeyWrapper.cs
Expand Up @@ -3,10 +3,6 @@
#if FEATURE_WIN32_REGISTRY

using System;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Security;

using Microsoft.Build.Shared;
using Microsoft.Win32;
Expand Down
4 changes: 3 additions & 1 deletion src/MSBuild/MSBuild.csproj
Expand Up @@ -221,12 +221,14 @@
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
</ItemGroup>
<ItemGroup>
<PackageReference Include="System.Configuration.ConfigurationManager" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
<!-- File for Assemblies we depend on -->
<Reference Include="System" />
<Reference Include="System.Core" />
<Reference Include="System.Xml" />
<Reference Include="System.Configuration" />
<PackageReference Include="LargeAddressAware" PrivateAssets="All" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">
Expand Down
2 changes: 0 additions & 2 deletions src/MSBuild/XMake.cs
Expand Up @@ -4,9 +4,7 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
#if FEATURE_SYSTEM_CONFIGURATION
using System.Configuration;
#endif
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
Expand Down
6 changes: 0 additions & 6 deletions src/Shared/FrameworkLocationHelper.cs
Expand Up @@ -4,17 +4,11 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
#if FEATURE_SYSTEM_CONFIGURATION
using System.Configuration;
#endif
using System.IO;
using System.Linq;
using System.Runtime.Versioning;
using Microsoft.Win32;

#if FEATURE_SYSTEM_CONFIGURATION
using PropertyElement = Microsoft.Build.Evaluation.ToolsetElement.PropertyElement;
#endif
using Microsoft.Build.Shared.FileSystem;

namespace Microsoft.Build.Shared
Expand Down
5 changes: 0 additions & 5 deletions src/Shared/ToolsetElement.cs
Expand Up @@ -3,17 +3,13 @@

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

namespace Microsoft.Build.Evaluation
{
#if FEATURE_SYSTEM_CONFIGURATION

/// <summary>
/// Helper class for reading toolsets out of the configuration file.
/// </summary>
Expand Down Expand Up @@ -708,5 +704,4 @@ public string DefaultOverrideToolsVersion
}
}
}
#endif
}
2 changes: 2 additions & 0 deletions src/Tasks.UnitTests/Copy_Tests.cs
Expand Up @@ -2397,7 +2397,9 @@ public void CopyToDestinationFolderWithSymbolicLinkCheck()

if (NativeMethodsShared.IsWindows)
{
#pragma warning disable CA1416 // Suppress Warning saying that WindowsPrincipal might not be compatible on Windows (Which shouldn't be an issue...)
if (!new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null)))
#pragma warning restore CA1416 // Suppress Warning saying that WindowsPrincipal might not be compatible on Windows (Which shouldn't be an issue...)
{
isPrivileged = false;
Assert.True(true, "It seems that you don't have the permission to create symbolic links. Try to run this test again with higher privileges");
Expand Down
3 changes: 1 addition & 2 deletions src/Utilities/Microsoft.Build.Utilities.csproj
Expand Up @@ -22,12 +22,11 @@
<ProjectReference Include="..\StringTools\StringTools.csproj" />

<PackageReference Include="System.Collections.Immutable" />
<PackageReference Include="System.Configuration.ConfigurationManager" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETStandard'">
<PackageReference Include="Microsoft.VisualStudio.Setup.Configuration.Interop" />

<Reference Include="System.Configuration" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETStandard'">
Expand Down