Add Features projects to SourceBuild #57277

JoeRobich · 2021-10-20T20:41:14Z

Resolves #57270

davidwengier · 2021-10-20T20:47:10Z

This would have made my last week easier 😛

EDIT: No it wouldn't, nevermind 😔

src/Features/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj

Compilers.sln

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj

jmarolf

General approach looks fine, but I agree that we want to change source build to build Roslyn.sln instead of Compilers.sln

Use Roslyn.sln, not Compilers.sln, to build more projects during source-build. Update ExcludeFromSourceBuild properties to include more projects and exclude a few projects that shouldn't be in source-build. The newly included projects are used by downstream repos.

This makes source-build stop producing the Microsoft.CodeAnalysis.Collections package. Downstream repos (MSBuild) can't safely use a source-built version.

By default, the projects reference Microsoft.Build 16.5.0, which has netcoreapp2.1 and net472 support. The new 17.0.0-... version built during source-build only has net6.0. This causes Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj to fail. Adding 16.5.0 to SBRP and dodging the override by renaming each package version property is the safe way to fix it.

JoeRobich · 2021-10-20T23:15:38Z

Updated with @dagood's change to build Roslyn.sln and to use RefOnly Microsoft.Build dependencies.

@jasonmalinowski @tmat @jmarolf This is ready for another review

JoeRobich · 2021-10-20T23:18:03Z

@dagood How does this work, will these changes flowing into dev17.0-vs-deps be enough to satisfy source build requirements or will we need an additional insertion into the SDK?

JoeRobich · 2021-10-20T23:27:27Z

Integration build failures are expected and should be resolved separately by #57281

jmarolf

dagood · 2021-10-21T00:33:21Z

How does this work, will these changes flowing into dev17.0-vs-deps be enough to satisfy source build requirements or will we need an additional insertion into the SDK?

I believe an SDK insertion would be needed to make the commit flow into this dependency:
https://github.com/dotnet/installer/blob/931367000f6efd1bc1650c4bcb5ab5dacbc51e10/eng/Version.Details.xml#L144-L148

However, source-build will work fine if we take this PR and include it as a patch file in the dotnet/installer release/6.0.1xx branch--no builds or dependency flow strictly required. So if the gates are very restrictive right now for rebuilds, this doesn't need to trigger one.

eng/Versions.props

src/Compilers/Core/MSBuildTask/Microsoft.Build.Tasks.CodeAnalysis.csproj

src/Dependencies/Directory.Build.props

eerhardt

Thanks for getting to this so quick @JoeRobich!

Just one small suggestion for easier reading.

src/Features/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Features.csproj

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj

src/Features/VisualBasic/Portable/Microsoft.CodeAnalysis.VisualBasic.Features.vbproj

sharwell · 2021-10-21T16:06:15Z

src/Compilers/Core/MSBuildTaskTests/Microsoft.Build.Tasks.CodeAnalysis.UnitTests.csproj

+    <PackageReference Include="Microsoft.Build" Version="$(RefOnlyMicrosoftBuildVersion)" />
+    <PackageReference Include="Microsoft.Build.Framework" Version="$(RefOnlyMicrosoftBuildFrameworkVersion)" />
+    <PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(RefOnlyMicrosoftBuildTasksCoreVersion)" />


❓ Should all uses of RefOnly*Version versions have the ExcludeAssets="Runtime" attribute?

We should update BuildBoss to require the attribute if a RefOnly package is used.

Would ExcludeAssets="Runtime" cause the unit tests to fail in the Microsoft build? I assume the tests actually find and execute the MSBuild code when they run. (In source-build, unit tests don't build or run, so this isn't a concern there.) (But... it's very possible I don't understand the effect of ExcludeAssets="Runtime" here.)

The name of these properties is misleading in this context for someone working on the Microsoft build. Really, any name that indicates these versions are "pinned" would do fine, if this is a concern.

In non-unit-test projects, ExcludeAssets="Runtime" sounds like an interesting idea to me to exercise how ref-only they really are, in a Microsoft build.

Co-authored-by: Eric Erhardt <[email protected]>

JoeRobich · 2021-10-21T23:19:24Z

@jasonmalinowski @tmat Any additional concerns before this is merged?

jasonmalinowski

Seems fine to me, but I'm admittedly not the expert here!

jasonmalinowski · 2021-10-22T20:39:12Z

eng/Versions.props

+      SourceBuild will requires that all dependencies also be source buildable. We are referencing a
+      version of MSBuild that is not SourceBuild compatible, which makes our build incompatible. Since we only
+      use these dependencies as reference assemblies, we can opt them out of this behavior by having their
+      version variable be prefixed with `RefOnly`. This will allow us to reference these libraries and remain
+      Source Build compatible.


Is there a version of MSBuild we can be referencing that doesn't have this caveat?

@dagood is MSBuild 16.11 compatible?

I thought Davis did a good job of explaining the situation here: #57277 (comment). If you haven't read that, check it out.

If you name this property MicrosoftBuildVersion, when building for source-build that property will automatically get overriden to point to the currently built version of Microsoft.Build.nupkg that was being built during source-build. This means this property would point to version 17.0-previewXXX.

However, Roslyn's build doesn't succeed when trying to build against the current source-built MSBuild nuget packages (probably because your tasks projects are building for net472 and the source-build MSBuild packages didn't at the time - that was fixed yesterday dotnet/installer#12472).

You'd have to use 17.0.0 (current latest that is being included in 6.0.100 SDK) and set up a eng/Version.Details.xml dependency and subscription on MSBuild to make sure it's always up to date. Any version other than the one being shipped in the SDK needs this kind of ref-only handling.

My understanding is that this dependency is most likely stuck on an old version because it needs to be compatible with old VS toolsets, anyway--so moving to a non-16.5.0 & non-17.0.0 version wouldn't work from an MSBuild perspective, let alone source-build's.

If MSBuild 16.11 is source build compatible, I will update our dependency and revert the 'RefOnly' changes in a follow up.

If MSBuild 16.11 is source build compatible

It's not, only an actively updated 17.0.0[-*] would be.

Maybe "caveat" is just too strong. Ref-only usage is everywhere, and it would be nice for source-build to get rid of it and have current references everywhere, but we expect old-ref-usage to stay in a lot of places because of the requirements the Microsoft build needs to fulfill. (E.g. NuGet packages need to be usable by people who are using old framework versions; VS has some complex compat requirements with the toolset it ships with.)

To me, this code comment is just a description of a normal process that is fine to have here. I filed dotnet/source-build#2546 so it can be hopefully be replaced with a link to a doc that describes this in more detail and with more context.

My understanding is that this dependency is most likely stuck on an old version because it needs to be compatible with old VS toolsets

@dagood @JoeRobich I'm not sure how well we support this library being used downlevel in that way -- it's not something we have automated test coverage for in any case. I'm not sure if this breaks folks that might have depended on our package and were also relying on us to have the dependency of some version if they didn't have another VS version on the box.

Add Features projects to SourceBuild

674f43e

JoeRobich requested a review from a team as a code owner October 20, 2021 20:41

dotnet-issue-labeler bot added the Area-Infrastructure label Oct 20, 2021

JoeRobich changed the base branch from main to release/dev17.0 October 20, 2021 20:41

davidwengier approved these changes Oct 20, 2021

View reviewed changes

jasonmalinowski reviewed Oct 20, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj Outdated Show resolved Hide resolved

tmat reviewed Oct 20, 2021

View reviewed changes

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj Outdated Show resolved Hide resolved

dagood reviewed Oct 20, 2021

View reviewed changes

Compilers.sln Outdated Show resolved Hide resolved

dseefeld reviewed Oct 20, 2021

View reviewed changes

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj Outdated Show resolved Hide resolved

tmat reviewed Oct 20, 2021

View reviewed changes

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj Outdated Show resolved Hide resolved

jmarolf suggested changes Oct 20, 2021

View reviewed changes

JoeRobich added 2 commits October 20, 2021 14:43

Fix Debugger.Contracts package condition

32c506a

Refine SourceBuild Features exclusions

0569f3d

333fred added the Source-Build Tracking dotnet/source-build related tasks. label Oct 20, 2021

dagood and others added 4 commits October 20, 2021 15:31

Exclude src/Dependencies from source-build

8b1f3cb

This makes source-build stop producing the Microsoft.CodeAnalysis.Collections package. Downstream repos (MSBuild) can't safely use a source-built version.

Remove SourceBuild project excludes

8fa8561

JoeRobich requested review from a team as code owners October 20, 2021 23:10

Revert changes to Compilers.sln

d6ceb3a

jmarolf approved these changes Oct 21, 2021

View reviewed changes

jasonmalinowski reviewed Oct 21, 2021

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

src/Compilers/Core/MSBuildTask/Microsoft.Build.Tasks.CodeAnalysis.csproj Show resolved Hide resolved

MichaelSimons reviewed Oct 21, 2021

View reviewed changes

src/Dependencies/Directory.Build.props Outdated Show resolved Hide resolved

Allow RefOnly prefixed package versions to pass build boss.

3d8ae85

Update comment

df8a913

This was referenced Oct 21, 2021

Document how modern ArPow PVP works, plus context dotnet/source-build#2546

Closed

Build more projects during source-build #57165

Closed

runfoapp bot mentioned this pull request Oct 21, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

eerhardt approved these changes Oct 21, 2021

View reviewed changes

sharwell reviewed Oct 21, 2021

View reviewed changes

JoeRobich and others added 2 commits October 21, 2021 13:50

Move source build condition to ItemGroups

76d7cf8

Co-authored-by: Eric Erhardt <[email protected]>

Revert src/Dependencies/Directory.Build.props

da94933

dagood approved these changes Oct 21, 2021

View reviewed changes

jasonmalinowski approved these changes Oct 22, 2021

View reviewed changes

JoeRobich merged commit 92a5218 into dotnet:release/dev17.0 Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Features projects to SourceBuild #57277

Add Features projects to SourceBuild #57277

JoeRobich commented Oct 20, 2021

davidwengier commented Oct 20, 2021 •

edited

Loading

jmarolf left a comment

JoeRobich commented Oct 20, 2021

JoeRobich commented Oct 20, 2021

JoeRobich commented Oct 20, 2021

jmarolf left a comment

dagood commented Oct 21, 2021

eerhardt left a comment

sharwell Oct 21, 2021 •

edited

Loading

sharwell Oct 21, 2021

dagood Oct 21, 2021 •

edited

Loading

JoeRobich commented Oct 21, 2021

jasonmalinowski left a comment

jasonmalinowski Oct 22, 2021

JoeRobich Oct 22, 2021

eerhardt Oct 22, 2021

dagood Oct 22, 2021 •

edited

Loading

JoeRobich Oct 22, 2021

dagood Oct 22, 2021

dagood Oct 22, 2021 •

edited

Loading

jasonmalinowski Oct 25, 2021

Add Features projects to SourceBuild #57277

Add Features projects to SourceBuild #57277

Conversation

JoeRobich commented Oct 20, 2021

davidwengier commented Oct 20, 2021 • edited Loading

jmarolf left a comment

Choose a reason for hiding this comment

JoeRobich commented Oct 20, 2021

JoeRobich commented Oct 20, 2021

JoeRobich commented Oct 20, 2021

jmarolf left a comment

Choose a reason for hiding this comment

dagood commented Oct 21, 2021

eerhardt left a comment

Choose a reason for hiding this comment

sharwell Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

sharwell Oct 21, 2021

Choose a reason for hiding this comment

dagood Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

JoeRobich commented Oct 21, 2021

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Oct 22, 2021

Choose a reason for hiding this comment

JoeRobich Oct 22, 2021

Choose a reason for hiding this comment

eerhardt Oct 22, 2021

Choose a reason for hiding this comment

dagood Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

JoeRobich Oct 22, 2021

Choose a reason for hiding this comment

dagood Oct 22, 2021

Choose a reason for hiding this comment

dagood Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski Oct 25, 2021

Choose a reason for hiding this comment

davidwengier commented Oct 20, 2021 •

edited

Loading

sharwell Oct 21, 2021 •

edited

Loading

dagood Oct 21, 2021 •

edited

Loading

dagood Oct 22, 2021 •

edited

Loading

dagood Oct 22, 2021 •

edited

Loading