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

Major MSBuildWorkspace update #21670

Merged
merged 123 commits into from Apr 5, 2018

Conversation

Projects
None yet
@DustinCampbell
Member

DustinCampbell commented Aug 22, 2017

Fixes #5557
Fixes #5668
Fixes #15102

  • 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:

  1. Need to figure out the breaking change story, update process, etc.
  2. Will need new new .nuspec file, publishing related stuff, etc.
{D89124D4-FB1D-4D08-BD95-F60BD24D5965}.Debug|Any CPU.Build.0 = Debug|x86
{D89124D4-FB1D-4D08-BD95-F60BD24D5965}.Release|Any CPU.ActiveCfg = Release|x86
{D89124D4-FB1D-4D08-BD95-F60BD24D5965}.Release|Any CPU.Build.0 = Release|x86
{D89124D4-FB1D-4D08-BD95-F60BD24D5965}.Debug|Any CPU.ActiveCfg = Debug|Any CPU

This comment has been minimized.

@Pilchie

Pilchie Aug 22, 2017

Member

Do you know why these switched to Any CPU?

This comment has been minimized.

@DustinCampbell

This comment has been minimized.

@DustinCampbell

DustinCampbell Aug 22, 2017

Member

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.

@@ -364,10 +365,6 @@ Global
{7FE6B002-89D8-4298-9B1B-0B5C247DD1FD}.Debug|Any CPU.Build.0 = Debug|Any CPU
{7FE6B002-89D8-4298-9B1B-0B5C247DD1FD}.Release|Any CPU.ActiveCfg = Release|Any CPU
{7FE6B002-89D8-4298-9B1B-0B5C247DD1FD}.Release|Any CPU.Build.0 = Release|Any CPU
{F7712928-1175-47B3-8819-EE086753DEE2}.Debug|Any CPU.ActiveCfg = Debug|Any CPU

This comment has been minimized.

@Pilchie

Pilchie Aug 22, 2017

Member

Any idea what this one is?

This comment has been minimized.

@DustinCampbell

DustinCampbell Aug 22, 2017

Member

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

This comment has been minimized.

@Pilchie

Pilchie Aug 22, 2017

Member

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

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Aug 22, 2017

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

This comment has been minimized.

Member

DustinCampbell commented Aug 22, 2017

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

This comment has been minimized.

Member

Pilchie commented Aug 22, 2017

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

@jaredpar @jasonmalinowski ?

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Aug 22, 2017

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

This comment has been minimized.

Member

Pilchie commented Aug 22, 2017

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

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Aug 23, 2017

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

This comment has been minimized.

Member

jaredpar commented Aug 23, 2017

@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

This comment has been minimized.

Member

DustinCampbell commented Aug 23, 2017

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

This comment has been minimized.

Member

DustinCampbell commented Sep 26, 2017

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

This comment has been minimized.

Member

jaredpar commented Oct 2, 2017

@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

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

This comment has been minimized.

@sharwell

sharwell Oct 2, 2017

Member

📝 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.

This comment has been minimized.

@DustinCampbell

DustinCampbell Oct 2, 2017

Member

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

This comment has been minimized.

@sharwell

sharwell Oct 2, 2017

Member

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. 🍻

This comment has been minimized.

@DustinCampbell

DustinCampbell Oct 2, 2017

Member

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. 😄

This comment has been minimized.

@sharwell

sharwell Oct 2, 2017

Member

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

This comment has been minimized.

@DustinCampbell

This comment has been minimized.

@Pilchie

Pilchie Dec 13, 2017

Member

Do we ever load this assembly in Visual Studio though?

This comment has been minimized.

@DustinCampbell

DustinCampbell Dec 13, 2017

Member

Maybe...

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

This comment has been minimized.

@jaredpar

jaredpar Mar 21, 2018

Member

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

This comment has been minimized.

Member

DustinCampbell commented Oct 2, 2017

@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

This comment has been minimized.

maca88 commented Oct 29, 2017

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

This comment has been minimized.

Member

sharwell commented Oct 29, 2017

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.)

</None>
<None Include="$(OutputPath)\..\..\Exes\vbc\net46\vbc.exe">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>

This comment has been minimized.

@jaredpar

jaredpar Dec 7, 2017

Member

Can you explain why this is needed? Copying the exes alone isn't generally useful due to issues like binding redirects.

This comment has been minimized.

@DustinCampbell

DustinCampbell Dec 8, 2017

Member

It's all horrible hackery to try and get the targets and props in the Microsoft.Build.Runtime packages. I've decided to take a completely different approach. I'll chat with you about it a bit later.

@@ -0,0 +1,76 @@
using System;

This comment has been minimized.

@jaredpar

jaredpar Dec 7, 2017

Member

Missin copyright header.

This comment has been minimized.

@DustinCampbell

@DustinCampbell DustinCampbell requested a review from dotnet/roslyn-infrastructure as a code owner Dec 9, 2017

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Dec 9, 2017

Just a single MSBuildWorkspace test failing now: TestOpenProject_MetadataReferenceHasDocComments

@Pilchie

Overall, looks great to me, but you'll need to pull in the removal of the samples.

I'm a bit surprised at how many test projects end up referencing the new dll. Is that just something else that needs cleaning up.

{
return new BuildInfo(executedProject, null);
// TODO: Add error message when project does not contain appropriate targets

This comment has been minimized.

@Pilchie

Pilchie Dec 13, 2017

Member

This seems "todone".

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

This comment has been minimized.

@Pilchie

Pilchie Dec 13, 2017

Member

Do we ever load this assembly in Visual Studio though?

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Dec 13, 2017

I'm a bit surprised at how many test projects end up referencing the new dll. Is that just something else that needs cleaning up.

Yup!

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Apr 2, 2018

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

DustinCampbell added some commits Apr 3, 2018

Properly type forward Microsoft.CodeAnalysis.FileTextLoader
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.
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)

This comment has been minimized.

@DustinCampbell

DustinCampbell Apr 3, 2018

Member

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.

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)

This comment has been minimized.

@DustinCampbell

DustinCampbell Apr 3, 2018

Member

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

This comment has been minimized.

Member

DustinCampbell commented Apr 3, 2018

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

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

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Apr 4, 2018

Member

Commented code left?

This comment has been minimized.

@DustinCampbell
@@ -17,12 +17,18 @@
<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" />

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Apr 4, 2018

Member

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?

This comment has been minimized.

@DustinCampbell

DustinCampbell Apr 4, 2018

Member

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

This comment has been minimized.

Member

DustinCampbell commented Apr 5, 2018

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

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

17 checks passed

WIP ready for review
Details
license/cla All CLA requirements met.
Details
microbuild_prtest Build finished.
Details
perf_correctness_prtest Build finished.
Details
ubuntu_16_debug_prtest Build finished.
Details
ubuntu_16_mono_debug_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_coreclr_debug_prtest Build finished.
Details
windows_coreclr_release_prtest Build finished.
Details
windows_debug_spanish_unit32_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_debug_vs-integration_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details
@Pilchie

This comment has been minimized.

Member

Pilchie commented Apr 5, 2018

🎆 💯 👍 :shipit:

@OmerRaviv

This comment has been minimized.

Contributor

OmerRaviv commented Apr 5, 2018

@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

This comment has been minimized.

Member

DustinCampbell commented Apr 5, 2018

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

This comment has been minimized.

Contributor

OmerRaviv commented Apr 9, 2018

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

This comment has been minimized.

Member

jaredpar commented Apr 9, 2018

@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

This comment has been minimized.

Member

DustinCampbell commented Apr 10, 2018

@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

This comment has been minimized.

Member

DustinCampbell commented Apr 10, 2018

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

@daveaglick

This comment has been minimized.

Contributor

daveaglick commented Apr 10, 2018

its-happening

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

@DustinCampbell

This comment has been minimized.

Member

DustinCampbell commented Apr 10, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment