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

Conversation

minglunli
Copy link
Contributor

@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 commented Mar 3, 2021

CLA assistant check
All CLA requirements met.

@cdmihai
Copy link
Contributor

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
Contributor Author

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
Contributor Author

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

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 😃

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

}
}

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

@benvillalobos
Copy link
Member

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

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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 👍

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 9, 2021
@benvillalobos benvillalobos merged commit 6848774 into dotnet:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants