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

Conversation

@minglunli
Copy link
Collaborator

@minglunli minglunli commented Mar 3, 2021

Context

This change will give NET5.0 MSBuild the option to read toolset information from the configuration file ONLY if ToolsetDefinitionLocation is set to 'ConfigurationFile'. Otherwise everything should work the exact same way as before.

This change is beneficial as it allows net5.0 MSBuild to evaluate projects that only net framework MsBuild could before. Since ConfigurationFile wasn't an option in net5.0 before (Due to System.Configuration being only net472), toolsets available to net5.0 MsBuild was minimal, but with this change a net5.0 MsBuild user could specify ConfigurationFile as the ToolsetDefinitionLocation when creating a ProjectCollection and more toolsets would be available depending on the MsBuild.exe provided.

Changes Made

Updated all the System.Configuration references to System.Configuration.ConfigurationManager (Which supports netstandard2.0). This change allows net5.0 to read toolset information from configuration file, but the default remains the same.

Testing

Tested locally to make sure the toolset information remains the same for cases other than when ToolsetDefinitionLocation is set to ConfigurationFile. CI should be good enough to ensure everything else still works as before.

Existing unit tests should be good enough to verify that this change does not affect net472 at all. Freed up a lot of unit tests under FEATURE_SYSTEM_CONFIGURATION which can be used to validate that net5.0 behavior without configuration setting works the same as before. Also added in a couple more tests to ensure setting ToolsetDefinitionLocation to ConfigurationFile actually grabs more configurations comparing to default.

@dnfadmin
Copy link

@dnfadmin dnfadmin commented Mar 3, 2021

CLA assistant check
All CLA requirements met.

@cdmihai
cdmihai approved these changes Mar 4, 2021
@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Mar 4, 2021

Should this test class get added all the time now?

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

@minglunli
Copy link
Collaborator Author

@minglunli minglunli commented Mar 4, 2021

Should this test class get added all the time now?

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

Yes I think it's fine to add it all the time now. I removed the FEATURE_CONFIGURATION_FILE flags for ToolsetConfigurationReader_Tests and ImportFromExtensionPath_Tests and they both require this file and they both seemed to pass. I believe those are the only 2 occurrences when the file is being used.

@minglunli
Copy link
Collaborator Author

@minglunli minglunli commented Mar 4, 2021

Should this test class get added all the time now?

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

Also just a heads up, I think you might have looked at the wrong PR 😄 The line you quoted is from another change

@@ -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" />

This comment has been minimized.

@Forgind

Forgind Mar 5, 2021
Member

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

This comment has been minimized.

@minglunli

minglunli Mar 5, 2021
Author Collaborator

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.

This comment has been minimized.

@Forgind

Forgind Mar 5, 2021
Member

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.

This comment has been minimized.

@minglunli

minglunli Mar 8, 2021
Author Collaborator

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.

This comment has been minimized.

@Forgind

Forgind Mar 8, 2021
Member

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

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Mar 6, 2021
Member

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

This comment has been minimized.

@minglunli

minglunli Mar 9, 2021
Author Collaborator

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

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Mar 6, 2021
Member

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

This comment has been minimized.

@minglunli

minglunli Mar 8, 2021
Author Collaborator

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

}
}

Assert.True(toolsetProperties.ContainsKey("MSBuildSDKsPath"));

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Mar 6, 2021
Member

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.

This comment has been minimized.

@minglunli

minglunli Mar 8, 2021
Author Collaborator

Updated to Shouldly

@BenVillalobos
Copy link
Member

@BenVillalobos BenVillalobos commented Mar 8, 2021

Note that a DDRIT test failed in the VS PR, but it doesn't explicitly look like us

Test method Microsoft.VisualStudio.Web.DDRIT.CSharpWapTest.CSharpWebApplicationTests threw exception:
Microsoft.Test.Apex.VisualStudio.Debugger.DebuggerException: Unable to stop debugging as the debugger is in 'Design' mode.
Assert.Fail failed. (3:34.844) [Visual Studio Host (7200):BreakPointVerifier] Verification failed: Breakpoint in file 'file:///C:/Test/Results/TestResults/Deploy_TestUser 2021-03-07 19_20_23/Out/TestSolutions/CSharpWAPWebForms/CSharpWAPWebForms/WebForm.aspx.cs', line 13 was hit successfully within 00:02:00 seconds.


Stack trace
 
Server stack trace: 
   at Microsoft.Test.Apex.VisualStudio.Debugger.DebuggerService.Stop(TimeSpan timeout) in Q:\cmd\n\src\omni\Apex\HostIntegration\VisualStudio.OM\Debugger\DebuggerService.cs:line 853
   at Microsoft.Test.Apex.VisualStudio.Debugger.DebuggerService.Stop() in Q:\cmd\n\src\omni\Apex\HostIntegration\VisualStudio.OM\Debugger\DebuggerService.cs:line 840
   at System.Runtime.Remoting.Messaging.StackBuilderSink._PrivateProcessMessage(IntPtr md, Object[] args, Object server, Object[]& outArgs)
   at System.Runtime.Remoting.Messaging.StackBuilderSink.SyncProcessMessage(IMessage msg)
Exception rethrown at [0]: 
   at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
   at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke(MessageData& msgData, Int32 type)
   at Microsoft.Test.Apex.VisualStudio.Debugger.DebuggerService.Stop()
   at Microsoft.VisualStudio.Web.DDRIT.CSharpWapTest.CSharpWebApplicationTests() in Q:\cmd\w\src\Tests\Microsoft.VisualStudio.Web.DDRIT\CSharpWapTest.cs:line 163

TestCleanup Stack Trace
   at Microsoft.Test.Apex.DelayedAssertionVerifierSink.RerouteAssertion(String message) in Q:\cmd\y\src\omni\Apex\Framework\Verifier\DelayedAssertionVerifierSink.cs:line 423
   at Microsoft.Test.Apex.DelayedAssertionVerifierSink.DoFinalAssertion() in Q:\cmd\y\src\omni\Apex\Framework\Verifier\DelayedAssertionVerifierSink.cs:line 240
   at Microsoft.Test.Apex.ApexTest.TestCleanup() in Q:\cmd\4\src\omni\Apex\MsTestIntegration\ApexTest.cs:line 551
   at Microsoft.Test.Apex.ExternalProcessHostTest`2.TestCleanup() in Q:\cmd\4\src\omni\Apex\MsTestIntegration\ExternalProcessHostTest.cs:line 136
   at Microsoft.Test.Apex.VisualStudio.VisualStudioHostTest.TestCleanup() in Q:\cmd\4\src\omni\Apex\MsTestIntegration\VisualStudioHostTest.cs:line 232
@Forgind
Forgind approved these changes Mar 8, 2021
Copy link
Member

@BenVillalobos BenVillalobos left a comment

LGTM

@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

This comment has been minimized.

@BenVillalobos

BenVillalobos Mar 8, 2021
Member

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

This comment has been minimized.

@minglunli

minglunli Mar 9, 2021
Author Collaborator

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

@BenVillalobos BenVillalobos merged commit 6848774 into dotnet:master Mar 11, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210308.11 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants