Skip to content

Build more projects during source-build#57165

Closed
dagood wants to merge 2 commits into
dotnet:mainfrom
dagood:source-build-roslyn-sln
Closed

Build more projects during source-build#57165
dagood wants to merge 2 commits into
dotnet:mainfrom
dagood:source-build-roslyn-sln

Conversation

@dagood
Copy link
Copy Markdown
Member

@dagood dagood commented Oct 14, 2021

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.

Here's the diff of nupkgs built before/after this PR in a local ./build.sh --restore --build --pack /p:ArcadeBuildFromSource=true:

 Microsoft.CodeAnalysis.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.Build.Tasks.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.Common.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.CSharp.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.CSharp.CodeStyle.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.CSharp.Features.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.CSharp.Scripting.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.CSharp.Workspaces.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.Features.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.Scripting.Common.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.VisualBasic.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.VisualBasic.CodeStyle.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.VisualBasic.Features.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.VisualBasic.Workspaces.4.1.0-dev.nupkg
 Microsoft.CodeAnalysis.Workspaces.Common.4.1.0-dev.nupkg
+Microsoft.CodeAnalysis.Workspaces.MSBuild.4.1.0-dev.nupkg
 Microsoft.Net.Compilers.Toolset.4.1.0-dev.nupkg
 Microsoft.NETCore.Compilers.4.1.0-dev.nupkg

This is for:

@eerhardt @MichaelSimons /cc @dotnet/source-build-internal

FYI @lbussell, I haven't looked at how this affects prebuilts. (RE: Roslyn prebuilt removal: dotnet/source-build#2419)

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.
@dagood dagood requested review from a team as code owners October 14, 2021 23:28
@ghost ghost added the Area-Infrastructure label Oct 14, 2021
This makes source-build stop producing the Microsoft.CodeAnalysis.Collections package. Downstream repos (MSBuild) can't safely use a source-built version.
@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 15, 2021

In the context of a current 6.0 tarball build, src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj fails:

/.../Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj error NU1202:
  Package Microsoft.Build 17.0.0-preview-21474-03 is not compatible with netcoreapp3.1 (.NETCoreApp,Version=v3.1).
  Package Microsoft.Build 17.0.0-preview-21474-03 supports: net6.0 (.NETCoreApp,Version=v6.0)
/.../Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj error NU1202:
  Package Microsoft.Build 17.0.0-preview-21474-03 is not compatible with net472 (.NETFramework,Version=v4.7.2).
  Package Microsoft.Build 17.0.0-preview-21474-03 supports: net6.0 (.NETCoreApp,Version=v6.0)                                                                                                     

By default, the project targets Microsoft.Build 16.5.0, which has netcoreapp2.1 and net472 support. The new 17.0.0-... only has net6.0. Adding 16.5.0 to SBRP and dodging the override is the safe way forward here.

@jaredpar
Copy link
Copy Markdown
Member

@333fred PTAL

@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 18, 2021

FYI @lbussell is working on the Microsoft.Build 16.5.0 vs. 17.0.0-preview... issue. I think it'll need to be figured out before merging.

@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 18, 2021

Roslyn folks, would you be ok with a global rename of these version properties to add a RefOnly prefix?

<MicrosoftBuildVersion>$(MicrosoftBuildPackagesVersion)</MicrosoftBuildVersion>
<MicrosoftBuildFrameworkVersion>$(MicrosoftBuildPackagesVersion)</MicrosoftBuildFrameworkVersion>
<MicrosoftBuildLocatorVersion>1.2.6</MicrosoftBuildLocatorVersion>
<MicrosoftBuildRuntimeVersion>$(MicrosoftBuildPackagesVersion)</MicrosoftBuildRuntimeVersion>
<MicrosoftBuildTasksCoreVersion>$(MicrosoftBuildPackagesVersion)</MicrosoftBuildTasksCoreVersion>

Basically, this opts out of the source-built versions. (Renaming prevents source-build from forcing them to the 17.0 version by not matching the override properties that source-build passes in.) This strategy was used in dotnet/runtime 5.0:

https://github.com/dotnet/runtime/blob/d1a6945c4ab23b2d621b6e0738d9411409ab8b56/eng/Versions.props#L123-L132

There is an opt-in approach that dotnet/roslyn-analyzers went for when onboarding ArPow: https://github.com/dotnet/roslyn-analyzers/blob/01f57af818ca7b93e1c27b69e7ad6cfb3d275f82/eng/Versions.props#L2. Maybe this is something Roslyn would be interested in, too.

It seems risky and potentially a time sink to switch Roslyn from opt-out to opt-in for 6.0, so what I'm thinking is:

  • Add a patch in dotnet/installer 6.0.1xx that renames the property.
  • Merge the patch into dotnet/roslyn servicing branches.
  • Switch to opt-in approach in dotnet/roslyn main (if desired) for future releases.

For now we're going forward with that first point to get prebuilt removal unblocked. Any thoughts?

@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 18, 2021

It seems risky and potentially a time sink to switch Roslyn from opt-out to opt-in for 6.0, so what I'm thinking is:

  • Add a patch in dotnet/installer 6.0.1xx that renames the property.
  • Merge the patch into dotnet/roslyn servicing branches.
  • Switch to opt-in approach in dotnet/roslyn main (if desired) for future releases.

For now we're going forward with that first point to get prebuilt removal unblocked. Any thoughts?

This seems fine to me.

@dagood dagood marked this pull request as draft October 20, 2021 20:17
@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 20, 2021

I moved this back to draft status because it needs more work per the plan. Right now, it causes src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj to fail in the tarball.

@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 21, 2021

  • Switch to opt-in approach in dotnet/roslyn main (if desired) for future releases.

Can anyone confirm this is actually wanted? Procedure-wise, it would be better to get the property-renaming approach in main (port #57277 to main) to make sure 7.0 doesn't regress, and then replace it with an opt-in approach as appropriate, rather than worrying about tracking this.

@dagood
Copy link
Copy Markdown
Member Author

dagood commented Oct 28, 2021

Procedure-wise, it would be better to get the property-renaming approach in main (port #57277 to main) to make sure 7.0 doesn't regress, and then replace it with an opt-in approach as appropriate, rather than worrying about tracking this.

The RefOnlyMicrosoftBuildPackagesVersion has been merged into main now, so I'm going to go ahead and close this PR.

If someone is interested in changing to an opt-in approach rather than this RefOnly- opt-out approach, please go ahead and file an issue to track. (This would purely be for Roslyn infrastructure reasons--source-build works fine either way.)

@dagood dagood closed this Oct 28, 2021
@dagood dagood deleted the source-build-roslyn-sln branch October 28, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants