Major MSBuildWorkspace update by DustinCampbell · Pull Request #21670 · dotnet/roslyn

DustinCampbell · 2017-08-22T21:24:50Z

Remove dependency on old COM-based host objects.
Add support for SDK-style projects (including multi-TFM projects).
Get general buy off on this source breaking change.
Add a Nuspec for the new Workspaces.MSBuild package.
Change API to account for the fact that a multi-TFM project will need to be loaded as multiple projects.
Add design-time batch build logic to improve performance.
Document how MSBuildWorkspace can be used by clients.
Create sample application that demonstrates how to use MSBuildWorkspace.

Pilchie

Generally favorable, but:

Need to figure out the breaking change story, update process, etc.
Will need new new .nuspec file, publishing related stuff, etc.

Pilchie · 2017-08-22T21:48:34Z

Do you know why these switched to Any CPU?

It appears that these are the TreeTransform samples (which I did not touch). I can't imagine why these would have been x86 in the Any CPU solution configuration to begin with.

Pilchie · 2017-08-22T21:49:41Z

Any idea what this one is?

I bet VS just removed it because there isn't a project GUID in the solution file matching that.

Oh, missed that this was Samples.sln, you're probably right.

DustinCampbell · 2017-08-22T22:00:18Z

Will need new new .nuspec file, publishing related stuff, etc.

Yes, but I don't want to do that until this project is portable. That, unfortunately, may be a bit complicated.

DustinCampbell · 2017-08-22T22:28:05Z

Need to figure out the breaking change story, update process, etc.

Yes, this is going to be a breaking change. I don't see a good way around that. Users will have to add a new package (Microsoft.CodeAnalysis.Workspaces.MSBuild) in order to use the new MSBuildWorkspace. It's also a binary breaking change since anyone could have been using MSBuildWorkspace inside Visual Studio, where we ship Workspaces.Desktop. However, I don't believe we intended for MSBuildWorkspace to be used within VS. Thoughts?

Pilchie · 2017-08-22T22:33:45Z

Can/Should we use typeforwarding for the public APIs that moved?

@jaredpar @jasonmalinowski ?

DustinCampbell · 2017-08-22T23:14:12Z

Can/Should we use typeforwarding for the public APIs that moved?

I think that might cause an awkward dependency. AFAIK, type-forwarding requires the old assembly to reference the new assembly. So, Workspaces.Desktop would need to reference Workspaces.MSBuild. However, that would break the NuGet packaging (if we want Workspaces.MSBuild to be in its own package) because Workspaces and Workspaces.Desktop are both included in Microsoft.CodeAnalysis.Workspaces.Common. So, Workspaces.MSBuild would need to be included in that package too.

Pilchie · 2017-08-22T23:23:42Z

Definitely problematic if true. I've never actually authored a type forwarder though.

DustinCampbell · 2017-08-22T23:27:52Z

https://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.typeforwardedtoattribute(v=vs.110).aspx

Pilchie · 2017-08-22T23:41:30Z

What about https://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.typeforwardedfromattribute(v=vs.110).aspx?

DustinCampbell · 2017-08-23T00:04:03Z

My understanding is that was added later (in .NET 4.0) to allow types to be traced back to their original assembly so that the serializer can function with forwarded types. @jaredpar / @jasonmalinowski: please let me know if I'm wrong about that.

jaredpar · 2017-08-23T13:05:38Z

@DustinCampbell explanation around forwarding matches my understanding. I don't really think there is a lot we can do here in terms of compat.

Know this is just testing CI at the moment. But can we get a quick explanation about why we can't have Workspaces.Desktop reference Workspaces.MSBuild? That order would allow for type forwarding. But guessing there is a deliberate reason for breaking it up as said in the PR.

DustinCampbell · 2017-08-23T13:13:58Z

Know this is just testing CI at the moment. But can we get a quick explanation about why we can't have Workspaces.Desktop reference Workspaces.MSBuild?

The issue is as follows:

Microsoft.CodeAnalysis.Workspaces.Common contains both Workspaces and Workspaces.Desktop
Workspaces.MSBuild has a dependency on Workspaces
We want Workspaces.MSBuild to live in its own package.

We'd like to have a packaging scheme where Microsoft.CodeAnalysis.Workspaces.MSBuild depends on Microsoft.CodeAnalysis.Workspaces.Common. However, if Workspaces.Desktop depends on Workspaces.MSBuild, we would be forced to push it into Microsoft.CodeAnalysis.Workspaces.Common instead.

DustinCampbell · 2017-09-26T22:36:33Z

Hmmm... They ran on my box. It's an improvement, but I'll need to dig in to figure out way they aren't passing in CI.

jaredpar · 2017-10-02T13:59:40Z

@DustinCampbell dont forget that we dont have VS installed on these boxes. Possible there is subtle machine state that allows it to pass on your box

sharwell · 2017-10-02T14:14:21Z

+    <RootNamespace>Microsoft.CodeAnalysis</RootNamespace>
+    <AssemblyName>Microsoft.CodeAnalysis.Workspaces.MSBuild</AssemblyName>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <TargetFramework>net46</TargetFramework>


📝 I have a branch locally where the goal was making this netstandard1.5 (the lowest standard which the MSBuild APIs support). It was mostly complete but there was a section of important code which couldn't be purely extracted from Workspaces.Desktop using strictly type forwarding to preserve compatibility so I didn't push it forward.

Ultimately, the plan is to make this netstandard2.0.

Cool, that wasn't an option at the time I looked so here's to hoping netstandard2.0 has all the APIs to make it straightforward success. 🍻

To be honest, it's not quite an option for this yet either. Moving to netstandard2.0 has implications for Visual Studio, though that might be a good thing to sort through and address now. 😄

📝 It will be a big deal for a long-standing request to provide command line automation for analyzers/fixes (on non-Windows systems).

Do we ever load this assembly in Visual Studio though?

Maybe...

Microsoft.CodeAnalysis.Workspaces.Desktop ships in VS today. It's possible that VS extensions may have targeted it.

What is the use case for this particular library? Is it only ever shipped via NuGet? That will impact our netstandard discussions.

In reply to: 156729257 [](ancestors = 156729257)

DustinCampbell · 2017-10-02T16:59:43Z

@DustinCampbell dont forget that we dont have VS installed on these boxes. Possible there is subtle machine state that allows it to pass on your box

Yeah, I realized that later but didn't update here. I intend to add ConditionalFacts to address this.

maca88 · 2017-10-29T21:32:34Z

I'm happy to say that thanks to this PR, I'm able to open and analyze projects using the MSBuildWorkspace on Linux and OSX using .NET Core. In order to make it work on .NET Core I've had to remove the BuildingInsideVisualStudio flag, otherwise it didn't worked. Another thing that I had to change are the project paths located in the solution file, as a backslash is not a directory separator on Unix e.g. ("Folder\MyProject.csproj" -> "Folder/MyProject.csproj").
Out of my curiosity, is this change planned for version 3.0.0 or a later one?

sharwell · 2017-10-29T23:41:57Z

We'd like to have a packaging scheme where Microsoft.CodeAnalysis.Workspaces.MSBuild depends on Microsoft.CodeAnalysis.Workspaces.Common. However, if Workspaces.Desktop depends on Workspaces.MSBuild, we would be forced to push it into Microsoft.CodeAnalysis.Workspaces.Common instead.

We can split Microsoft.CodeAnalysis.Workspaces.Common into 3 packages:

Microsoft.CodeAnalysis.Workspaces
Microsoft.CodeAnalysis.Workspaces.Desktop
Microsoft.CodeAnalysis.Workspaces.MSBuild

Then make Microsoft.CodeAnalysis.Workspaces.Common a metadata-only package that declares dependencies on the 3 new packages. Within the 3 new packages the dependencies are clear. Users upgrading to the new version via the metadata-only package will not be broken, nor will users who compiled against an older version. (Does not address cases where type forwarding does not apply, should any such cases exist.)

…tion fails

DustinCampbell · 2018-04-02T21:28:54Z

@jasonmalinowski: Except for the type forwarder (which is currently blocked), I believe I've addressed your CR feedback or asked further questions. Could you take a sweep through your comments and make sure they've been addressed to your satisfaction?

Since we're moving this type from Microsoft.CodeAnalysis.Workspaces.Desktop to Microsoft.CodeAnalysis.Workspaces, a type forward is required. There is currently a bug with the Public API analyzer with properties on forwarded types that I have to work around in the PublicAPI.Shipped.txt, but a fix is already out for that dotnet/roslyn-analyzers#1657. Once the analyzers are updated in the roslyn repo, this will get fixed.

DustinCampbell · 2018-04-03T18:42:59Z

-Microsoft.CodeAnalysis.FileTextLoader.Path.get -> string
+Microsoft.CodeAnalysis.FileTextLoader (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)
+Microsoft.CodeAnalysis.FileTextLoader.DefaultEncoding.get -> System.Text.Encoding (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)
+Microsoft.CodeAnalysis.FileTextLoader.DefaultEncoding { get; } -> System.Text.Encoding (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)


Note: This is working around a bug in the public API analyzer with properties on forwarded types. This is already fixed in the analyzer, and this file will be updated when the analyzers are updated in the Roslyn repo.

DustinCampbell · 2018-04-03T18:43:04Z

+Microsoft.CodeAnalysis.FileTextLoader.DefaultEncoding { get; } -> System.Text.Encoding (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)
+Microsoft.CodeAnalysis.FileTextLoader.FileTextLoader(string path, System.Text.Encoding defaultEncoding) -> void (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)
+Microsoft.CodeAnalysis.FileTextLoader.Path.get -> string (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)
+Microsoft.CodeAnalysis.FileTextLoader.Path { get; } -> string (forwarded, contained in Microsoft.CodeAnalysis.Workspaces)


Note: This is working around a bug in the public API analyzer with properties on forwarded types. This is already fixed in the analyzer, and this file will be updated when the analyzers are updated in the Roslyn repo.

DustinCampbell · 2018-04-03T18:48:31Z

@jasonmalinowski: The type forward is now in place. I believe I've addressed all of your feedback. Please let me know if that's not the case.

jasonmalinowski

jasonmalinowski · 2018-04-04T21:46:31Z

-                return commandLineArgs.SetItem(platformIndex, "/platform:anycpu");
-            }
-
+            // return AdjustPlatformCommandLineArg(commandLineArgs);


Commented code left?

jasonmalinowski · 2018-04-04T21:54:00Z

    <ProjectReference Include="..\..\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj" />
    <ProjectReference Include="..\..\Compilers\Test\Resources\Core\CompilerTestResources.csproj" />
    <ProjectReference Include="..\..\Compilers\VisualBasic\Portable\BasicCodeAnalysis.vbproj" />
+    <ProjectReference Include="..\..\EditorFeatures\Core\EditorFeatures.csproj" />


Any idea which project reference was added that required this to be added? BuildBoss is forcing transitivity which I get, but still seems odd our MSBuild workspace tests depends on IDE code, even through some indirect chain?

It's because WorkspaceTestServices now references EditorFeatures. I'm not a fan of this reference. It feels like a layering violation. This is from @sharwell's recent changes for unit tests using MEF.

DustinCampbell · 2018-04-05T01:07:20Z

OK. Once CI passes, I'll (finally) merge.

Pilchie · 2018-04-05T02:19:59Z

🎆 💯 👍

OmerRaviv · 2018-04-05T08:49:16Z

@DustinCampbell Congratulations and thanks for all your hard work! We'd love to start using this as soon as we can and provide feedback. However, the new Workspaces.MSBuild NuGet package does not appear to be available yet on the roslyn-master-nightly feed?

Is there any publicly available way we can already consume this ?

DustinCampbell · 2018-04-05T10:56:39Z

Patience @OmerRaviv. It's only just been merged and its still nighttime in the U.S. (my clock reads 3:54 am 😄).

I'll follow up with folks today to see if there're any additional steps to push this package to our nightly feed.

OmerRaviv · 2018-04-09T15:28:18Z

Thanks, @DustinCampbell! Will try my best to be patient 😄 I still don't see it on the roslyn-master-nightly MyGet stream, so I assume there's some additional steps needed? Happy to send a PR if this is something an outsider can fix.

jaredpar · 2018-04-09T15:43:45Z

@OmerRaviv no additional steps are needed here, at least in the GitHub repository. Looks like there is an issue with how our official builds are running at the moment. I'm following up.

DustinCampbell · 2018-04-10T21:38:21Z

@OmerRaviv: The package is now available from the roslyn feed: https://dotnet.myget.org/feed/roslyn/package/nuget/Microsoft.CodeAnalysis.Workspaces.MSBuild.

Here is the first draft of a document I'm working to describe how to use MSBuildWorkspace: https://gist.github.com/DustinCampbell/32cd69d04ea1c08a16ae5c4cd21dd3a3.

DustinCampbell · 2018-04-10T21:38:48Z

@daveaglick: You might be interested in that as well.

daveaglick · 2018-04-10T21:46:43Z

Thanks for all your hard work getting this out the door. Looking forward to taking it for a spin.

DustinCampbell · 2018-04-10T21:48:57Z

FYI that I'm also working on a sample application here: https://github.com/DustinCampbell/MSBuildWorkspaceTester.

DustinCampbell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 22, 2017

dnfclas added the cla-already-signed label Aug 22, 2017

Pilchie reviewed Aug 22, 2017

View reviewed changes

DustinCampbell force-pushed the msbuildworkspace branch 4 times, most recently from 9b0dea5 to 0a38f54 Compare August 28, 2017 22:22

DustinCampbell force-pushed the msbuildworkspace branch 3 times, most recently from 86835b7 to 6613bf5 Compare September 26, 2017 15:45

DustinCampbell mentioned this pull request Sep 26, 2017

MSBuild tests need to fully support a VS-local MSBuild #16301

Open

sharwell reviewed Oct 2, 2017

View reviewed changes

maca88 mentioned this pull request Oct 21, 2017

NetStandard (or NetCore) version? maca88/AsyncGenerator#24

Closed

DustinCampbell mentioned this pull request Nov 6, 2017

Microsoft.CodeAnalysis.Workspaces.Desktop.dll is missing from the netstandard1.3 version of Microsoft.CodeAnalysis.Workspaces.Common NuGet package #23011

Closed

DustinCampbell force-pushed the msbuildworkspace branch from fa4638a to 897deb5 Compare December 7, 2017 22:11

DustinCampbell added 2 commits April 2, 2018 14:11

Remove unnecessary 'language support' check

7457d88

Ensure that MSBuildProjectLoader reports progress, even when an opera…

b260640

…tion fails

mavasani mentioned this pull request Apr 2, 2018

Move Roslyn.sln to newer analyzers (with Dispose rules disabled) #25891

Merged

Merge branch 'master' into msbuildworkspace

1df5359

DustinCampbell force-pushed the msbuildworkspace branch from a378a48 to 1df5359 Compare April 3, 2018 14:45

DustinCampbell added 3 commits April 3, 2018 08:50

Merge branch 'master' into msbuildworkspace

22faa2f

Appease the build boss

7ff6fb4

DustinCampbell commented Apr 3, 2018

View reviewed changes

jasonmalinowski approved these changes Apr 4, 2018

View reviewed changes

DustinCampbell added 3 commits April 4, 2018 16:54

Merge branch 'master' into msbuildworkspace

01149ae

Remove dead comment

66f85bd

Merge branch 'master' into msbuildworkspace

d6fa173

DustinCampbell merged commit a3f904c into dotnet:master Apr 5, 2018

Pilchie mentioned this pull request Aug 16, 2018

Version used in VS 15.8 not available on NuGet (Breaks VS extension using MSBuildWorkspace) #29334

Closed

jmarolf mentioned this pull request Aug 17, 2018

Update Microsoft.CodeAnalysis package to 2.9.0 is not straightforward. Missing MSBuildWorkspace #29375

Closed

DustinCampbell deleted the msbuildworkspace branch August 24, 2021 19:53

Conversation

DustinCampbell commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Aug 22, 2017

Uh oh!

DustinCampbell commented Aug 22, 2017

Uh oh!

Pilchie commented Aug 22, 2017

Uh oh!

DustinCampbell commented Aug 22, 2017

Uh oh!

Pilchie commented Aug 22, 2017

Uh oh!

DustinCampbell commented Aug 22, 2017

Uh oh!

Pilchie commented Aug 22, 2017

Uh oh!

DustinCampbell commented Aug 23, 2017

Uh oh!

jaredpar commented Aug 23, 2017

Uh oh!

DustinCampbell commented Aug 23, 2017

Uh oh!

DustinCampbell commented Sep 26, 2017

Uh oh!

jaredpar commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Oct 2, 2017

Uh oh!

maca88 commented Oct 29, 2017

Uh oh!

sharwell commented Oct 29, 2017

Uh oh!

DustinCampbell commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Apr 3, 2018

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Aug 22, 2017 •

edited

Loading

sharwell Oct 2, 2017 •

edited

Loading

OmerRaviv commented Apr 5, 2018 •

edited

Loading