Apply source-build patches #58727

lbussell · 2022-01-07T23:14:21Z

See dotnet/source-build#2708. The source-build team is making an effort to upstream all of our patches.

Summary of changes:

Disable apphost build of 'csi', 'vbi' for source-build to avoid including prebuilt packages
Condition some package references out of Tools.props to avoid including prebuilt packages

tmat · 2022-01-09T22:15:57Z

Build a specific subset of projects using a new solution file to avoid including non-open-source references.

Why is this needed? We already set <ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> in projects that should be excluded.

sharwell · 2022-01-10T18:45:39Z

Build a specific subset of projects using a new solution file to avoid including non-open-source references.

Why is this needed? We already set <ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> in projects that should be excluded.

This was a red flag to me as well. We should ideally use one approach or the other. I have no issue with either approach, as long as we either drop the other approach or include a new build validation step that verifies the two are consistent (i.e. the set of projects marked <ExcludeFromSourceBuild> is guaranteed to be the same set of projects in the solution filter).

If we use a solution filter, it should be relocated to the eng subdirectory.

sharwell · 2022-01-10T18:47:38Z

eng/SourceBuild.props

+      <InnerBuildArgs Condition="'$(DotNetBuildFromSource)' != 'true'">$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\Roslyn.sln"</InnerBuildArgs>
+      <InnerBuildArgs Condition="'$(DotNetBuildFromSource)' == 'true'">$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\Roslyn.SourceBuild.slnf"</InnerBuildArgs>


😕 Why would SourceBuild.props be included in non-sourcebuild builds? Can we just make the <Target> element itself conditional?

sharwell · 2022-01-10T18:48:20Z

eng/Tools.props

@@ -1,6 +1,6 @@
 <Project>

-  <ItemGroup>
+  <ItemGroup Condition="'$(DotNetBuildFromSource)' != 'true'">


❓ Why is Tools.props being included in sourcebuild? Can we exclude the file through some other means?

It looks like it is included by Arcade: https://github.com/dotnet/arcade/blob/aa647dcfc48bd3a644625e549ff71b7d06ed0958/src/Microsoft.DotNet.Arcade.Sdk/tools/Tools.proj#L53

sharwell · 2022-01-10T18:50:54Z

...ools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/CSharpSyntaxGenerator.csproj

@@ -28,4 +28,10 @@

    <PackageReference Include="Microsoft.CodeAnalysis.Common" Version="$(SourceGeneratorMicrosoftCodeAnalysisVersion)" PrivateAssets="all" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
  </ItemGroup>
+  <ItemGroup Condition="'$(DotNetBuildFromSource)' == 'true'">
+    <PackageReference Include="System.Collections.Immutable" Version="$(RefOnlySystemCollectionsImmutableVersion)" PrivateAssets="all" Condition="'$(TargetFramework)' == 'netstandard2.0'" />


❓ Why isn't this implicit via the Microsoft.CodeAnalysis.Common reference above?

sharwell · 2022-01-10T18:51:38Z

...ools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/CSharpSyntaxGenerator.csproj

@@ -28,4 +28,10 @@

    <PackageReference Include="Microsoft.CodeAnalysis.Common" Version="$(SourceGeneratorMicrosoftCodeAnalysisVersion)" PrivateAssets="all" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
  </ItemGroup>
+  <ItemGroup Condition="'$(DotNetBuildFromSource)' == 'true'">
+    <PackageReference Include="System.Collections.Immutable" Version="$(RefOnlySystemCollectionsImmutableVersion)" PrivateAssets="all" Condition="'$(TargetFramework)' == 'netstandard2.0'" />


💡 Consider restructuring this as a <Choose> element to avoid needing the same condition on each <PackageReference>

lbussell · 2022-01-10T18:54:29Z

Why is this needed? We already set <ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> in projects that should be excluded.

See #57342 for a better description of why <ExcludeFromSourceBulid/> doesn't work for projects depending on the Windows Desktop SDK.

I can look into getting rid of the <ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> in project files if the solution filter is what we need to use long term.

tmat · 2022-01-10T19:13:54Z

Would it be possible for source build to create an empty WindowsDesktop SDK package and build against that?

MichaelSimons · 2022-01-10T22:14:20Z

Would it be possible for source build to create an empty WindowsDesktop SDK package and build against that?

This was mentioned in dotnet/installer#12500 (comment). Using a solution filter is a maintainability concern so avoiding them is preferable. On the other hand, avoiding the SDK reference in the first place is preferable if possible IMO as it is more obvious to what the behavior is instead of injecting an empty SDK during source-build.

tmat · 2022-01-10T22:31:04Z

avoiding the SDK reference in the first place is preferable if possible IMO as it is more obvious to what the behavior is instead of injecting an empty SDK during source-build.

I don't see a problem with injecting SDK that itself can specify ExcludeFromSourceBulid = true.
The benefit of the SDK excluding itself from source build is a centralized solution. No need to update all repos, maintain separate lists of projects and having two mechanisms for source build exclusion. Besides if we decided in future to include the SDK in source build it could be done by swapping the fake one with the real one.

crummel · 2022-01-11T22:12:09Z

Would it be possible for source build to create an empty WindowsDesktop SDK package and build against that?

I'm a bit concerned that a fake WindowsDesktop SDK could escape to the final source-built SDK output and cause problems for users trying to create a project with it. I think this would be worth following up on but the solution filter seems like the most straightforward way to avoid any potential issues for a fix right now.

tmat · 2022-01-11T22:20:48Z

the solution filter seems like the most straightforward way to avoid any potential issues for a fix right now.

Sure, but it adds debt. I'd prefer solution that does not add debt.

lbussell · 2022-01-12T19:38:53Z

@tmat @sharwell for now we will take the solution filter change out of this PR and tackle the problem (probably with a different approach) in another PR.

eng/Versions.props

333fred · 2022-01-20T23:12:06Z

Looks like BuildBoss will need to be updated: https://github.com/dotnet/roslyn/blob/main/src/Tools/BuildBoss/ProjectCheckerUtil.cs#L144

lbussell · 2022-01-21T18:26:25Z

Looks like BuildBoss will need to be updated:

Is this something I need to do?

333fred · 2022-01-21T20:04:08Z

Is this something I need to do?

Yes, or the correctness build leg will not pass.

…sly source built versions

Pull request for applying this patch: dotnet#57159

Creating an apphost for a netcoreapp3.1 project uses the apphost pack as a prebuilt. Stopping these projects from creating the apphost removes the prebuilt for source-build. See: dotnet#57233 PR: dotnet#57306

Some projects use Microsoft.NET.Sdk.WindowsDesktop sdk which cannot be built using a boostrapped source-build sdk, since WindowsDesktop is not supported. These cannot be ignored using ExcludeFromSourceBuild because the project still needs to be loaded and the sdk cannot be found. See dotnet#57342

…ren't set

lbussell · 2022-02-10T21:01:00Z

@333fred, @sharwell do I need to make any other changes for this to be merged?

I have also solved the Windows Desktop SDK issue in source build without needing to change anything in Roslyn, so the solution filter approach is no longer necessary.

sharwell · 2022-02-14T23:24:51Z

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.
+    -->
+    <RefOnlyMicrosoftBuildVersion>$(RefOnlyMicrosoftBuildPackagesVersion)</RefOnlyMicrosoftBuildVersion>
+    <RefOnlyMicrosoftBuildFrameworkVersion>$(RefOnlyMicrosoftBuildPackagesVersion)</RefOnlyMicrosoftBuildFrameworkVersion>
+    <RefOnlyMicrosoftBuildRuntimeVersion>$(RefOnlyMicrosoftBuildPackagesVersion)</RefOnlyMicrosoftBuildRuntimeVersion>
+    <RefOnlyMicrosoftBuildTasksCoreVersion>$(RefOnlyMicrosoftBuildPackagesVersion)</RefOnlyMicrosoftBuildTasksCoreVersion>


❔ Is there a reason this needs to move to the bottom of the file?

sharwell · 2022-02-14T23:27:27Z

eng/Versions.props

+    <SourceBuildLiftedSystemCollectionsImmutableVersion Condition="'$(SourceBuildLiftedSystemCollectionsImmutableVersion)' == ''">$(SystemCollectionsImmutableVersion)</SourceBuildLiftedSystemCollectionsImmutableVersion>
+    <SourceBuildLiftedSystemReflectionMetadataVersion Condition="'$(SourceBuildLiftedSystemReflectionMetadataVersion)' == ''">$(SystemReflectionMetadataVersion)</SourceBuildLiftedSystemReflectionMetadataVersion>
+    <SourceBuildLiftedSystemRuntimeCompilerServicesUnsafeVersion Condition="'$(SourceBuildLiftedSystemRuntimeCompilerServicesUnsafeVersion)' == ''">$(SystemRuntimeCompilerServicesUnsafeVersion)</SourceBuildLiftedSystemRuntimeCompilerServicesUnsafeVersion>
+    <SourceBuildLiftedSystemTextEncodingCodePagesVersion Condition="'$(SourceBuildLiftedSystemTextEncodingCodePagesVersion)' == ''">$(SystemTextEncodingCodePagesVersion)</SourceBuildLiftedSystemTextEncodingCodePagesVersion>


❓ What is SourceBuildLifted prefix? Why do none of the existing naming approaches work? A comment should be added for future readers.

sharwell · 2022-02-14T23:29:23Z

...ools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/CSharpSyntaxGenerator.csproj

+      <ItemGroup Condition="'$(DotNetBuildFromSource)' == 'true'">
+        <PackageReference Include="System.Collections.Immutable" Version="$(SourceBuildLiftedSystemCollectionsImmutableVersion)" PrivateAssets="all" />
+        <PackageReference Include="System.Reflection.Metadata" Version="$(SourceBuildLiftedSystemReflectionMetadataVersion)" PrivateAssets="all" />
+        <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SourceBuildLiftedSystemRuntimeCompilerServicesUnsafeVersion)" PrivateAssets="all" />
+        <PackageReference Include="System.Text.Encoding.CodePages" Version="$(SourceBuildLiftedSystemTextEncodingCodePagesVersion)" PrivateAssets="all" />
+      </ItemGroup>


❔ Why do these need to be explicit? Aren't they inherited from Microsoft.CodeAnalysis.Common?

The packages are inherited from Microsoft.CodeAnalysis.Common. However, roslyn builds before runtime in the source-build tarball, so we don't have access to the "current" versions of the packages to build against. The SourceBuildLifted- versions point to the previously source-built versions of the packages (i.e. the last build's output) so that we don't try to restore versions of these packages we don't have. That is also why the RefOnly- prefix is not applicable here.

I have just realized that a better solution for this might be to add the packages to SBRP, so I will get working on that and hopefully this change won't be necessary.

I have added the necessary packages to SBRP and verified that the changes to CSharpSyntaxGenerator.csproj will no longer be necessary in the context of a full source-build tarball build and bootstrap. I also reverted those changes in this PR.

lbussell · 2022-03-10T19:03:53Z

As far as I can tell these CI failures are consistent with the main branch. @333fred, @sharwell I'd like to get this merged soon.

333fred · 2022-03-10T22:19:48Z

/azp run roslyn-integration-ci

azure-pipelines · 2022-03-10T22:19:59Z

Azure Pipelines successfully started running 1 pipeline(s).

333fred · 2022-03-10T22:20:07Z

/azp run roslyn-integration-corehost

azure-pipelines · 2022-03-10T22:20:17Z

Azure Pipelines successfully started running 1 pipeline(s).

333fred · 2022-03-15T16:18:38Z

@sharwell I'm going ahead and overriding the one failing integration test.

dotnet-issue-labeler bot added the Area-Infrastructure label Jan 7, 2022

lbussell mentioned this pull request Jan 7, 2022

[main] Backport source-build patches to repos. dotnet/source-build#2708

Closed

sharwell reviewed Jan 10, 2022

View reviewed changes

lbussell force-pushed the source-build-patches branch from 0226d2d to d188c8d Compare January 10, 2022 22:01

lbussell marked this pull request as ready for review January 12, 2022 20:01

lbussell requested review from a team as code owners January 12, 2022 20:01

runfoapp bot mentioned this pull request Jan 12, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

333fred reviewed Jan 20, 2022

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

lbussell mentioned this pull request Jan 20, 2022

Override Microsoft.Net.Sdk.WindowsDesktop references during source-build in Roslyn dotnet/installer#13093

Merged

lbussell force-pushed the source-build-patches branch 2 times, most recently from db30781 to 47ee45b Compare January 20, 2022 21:43

lbussell and others added 5 commits January 24, 2022 15:58

lift version of Microsoft.CodeAnalysis.Common dependencies to previou…

35d6ebc

…sly source built versions

remove Microsoft.DotNet prebuilts

a8ee96d

Pull request for applying this patch: dotnet#57159

Disable apphost build of 'csi', 'vbi' for source-build

077fcca

Creating an apphost for a netcoreapp3.1 project uses the apphost pack as a prebuilt. Stopping these projects from creating the apphost removes the prebuilt for source-build. See: dotnet#57233 PR: dotnet#57306

change lifted package version names to refonly and set them if they a…

f0d0e9a

…ren't set

MichaelSimons mentioned this pull request Jan 26, 2022

Disable apphost build of 'csi', 'vbi' for source-build #57306

Closed

lbussell requested a review from sharwell January 27, 2022 19:43

lbussell mentioned this pull request Feb 1, 2022

Resolve Microsoft.DotNet prebuilts for source-build #57159

Closed

Merge branch 'main' into source-build-patches

c91e00b

sharwell reviewed Feb 14, 2022

View reviewed changes

Merge branch 'main' into source-build-patches

b5bb06b

lbussell mentioned this pull request Feb 16, 2022

Add System.Runtime.CompilerServices.Unsafe 6.0.0 and System.Text.Encoding.CodePages 6.0.0 dotnet/source-build-reference-packages#335

Merged

lbussell added 2 commits February 18, 2022 16:33

Merge branch 'main' into source-build-patches

35464c3

Revert changes for lifting some package versions for source-build

caa111e

lbussell requested review from 333fred and sharwell February 19, 2022 00:48

lbussell added 2 commits March 3, 2022 15:10

Merge branch 'main' into source-build-patches

d3189c0

Merge branch 'main' into source-build-patches

ad01398

333fred approved these changes Mar 10, 2022

View reviewed changes

sharwell approved these changes Mar 10, 2022

View reviewed changes

333fred enabled auto-merge (squash) March 10, 2022 19:18

333fred disabled auto-merge March 15, 2022 16:18

333fred merged commit 41e91ea into dotnet:main Mar 15, 2022

ghost added this to the Next milestone Mar 15, 2022

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply source-build patches #58727

Apply source-build patches #58727

lbussell commented Jan 7, 2022 •

edited

Loading

tmat commented Jan 9, 2022

sharwell commented Jan 10, 2022 •

edited

Loading

sharwell Jan 10, 2022

sharwell Jan 10, 2022

lbussell Jan 10, 2022

sharwell Jan 10, 2022

sharwell Jan 10, 2022

lbussell commented Jan 10, 2022

tmat commented Jan 10, 2022 •

edited

Loading

MichaelSimons commented Jan 10, 2022

tmat commented Jan 10, 2022 •

edited

Loading

crummel commented Jan 11, 2022

tmat commented Jan 11, 2022

lbussell commented Jan 12, 2022

333fred commented Jan 20, 2022

lbussell commented Jan 21, 2022

333fred commented Jan 21, 2022

lbussell commented Feb 10, 2022

sharwell Feb 14, 2022

sharwell Feb 14, 2022

sharwell Feb 14, 2022

lbussell Feb 15, 2022

lbussell Feb 19, 2022

lbussell commented Mar 10, 2022

333fred commented Mar 10, 2022

azure-pipelines bot commented Mar 10, 2022

333fred commented Mar 10, 2022

azure-pipelines bot commented Mar 10, 2022

333fred commented Mar 15, 2022

		<InnerBuildArgs Condition="'$(DotNetBuildFromSource)' != 'true'">$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\Roslyn.sln"</InnerBuildArgs>
		<InnerBuildArgs Condition="'$(DotNetBuildFromSource)' == 'true'">$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\Roslyn.SourceBuild.slnf"</InnerBuildArgs>

Apply source-build patches #58727

Apply source-build patches #58727

Conversation

lbussell commented Jan 7, 2022 • edited Loading

tmat commented Jan 9, 2022

sharwell commented Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbussell commented Jan 10, 2022

tmat commented Jan 10, 2022 • edited Loading

MichaelSimons commented Jan 10, 2022

tmat commented Jan 10, 2022 • edited Loading

crummel commented Jan 11, 2022

tmat commented Jan 11, 2022

lbussell commented Jan 12, 2022

333fred commented Jan 20, 2022

lbussell commented Jan 21, 2022

333fred commented Jan 21, 2022

lbussell commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbussell commented Mar 10, 2022

333fred commented Mar 10, 2022

azure-pipelines bot commented Mar 10, 2022

333fred commented Mar 10, 2022

azure-pipelines bot commented Mar 10, 2022

333fred commented Mar 15, 2022

lbussell commented Jan 7, 2022 •

edited

Loading

sharwell commented Jan 10, 2022 •

edited

Loading

tmat commented Jan 10, 2022 •

edited

Loading

tmat commented Jan 10, 2022 •

edited

Loading