ArPow stage 1: local source-build infrastructure by MichaelSimons · Pull Request #51765 · dotnet/roslyn

MichaelSimons · 2021-03-09T20:56:04Z

This PR adds the local build infrastructure that lets ArPow (arcade-powered source-build) run in this repo. See https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/onboarding/local-onboarding.md for more details about how it works.

To try it out locally, run this on Linux: ./build.sh -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl

This PR should have no effect on ordinary builds, or CI. ArPow stage 2 will add source-build to CI: PR validation and official builds.

For https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/implementation-plan.md

Related to #dotnet/source-build#2071

This PR is currently blocked by the work to bring in Arcade 6.0

MichaelSimons · 2021-03-09T21:35:20Z

eng/targets/Imports.targets

This issue should get addressed before merging.

333fred · 2021-03-09T22:36:24Z

This PR should have no effect on ordinary builds, or CI. ArPow stage 2 will add source-build to CI: PR validation and official builds.

We already have a CI leg for source-build, I think we should update that in the same PR. It's this section of the YAML: https://github.com/dotnet/roslyn/blob/main/azure-pipelines.yml#L201-L223.

333fred · 2021-03-09T22:38:52Z

@MichaelSimons were you actually expecting a review at this stage?

MichaelSimons · 2021-03-09T22:43:56Z

were you actually expecting a review at this stage?

I'm not expecting a review until the Arcade 6.0 blocking issue is resolved. That being said I would appreciate any initial comments such the CI one @333fred made.

Conflicts: eng/Version.Details.xml

Conflicts: azure-pipelines.yml

…ase1-2 Conflicts: eng/Version.Details.xml

MichaelSimons · 2021-04-19T21:30:57Z

@dseefeld, @333fred - this is ready for review now.

333fred · 2021-04-19T21:36:38Z

@MichaelSimons there are a large number of patches here. Why so many? At one point in the 2.2-3.0 timeframe we had removed all the patches from source-build for roslyn, but we appear to have to have added more back since then, for things that really could and should have been fixed in source directly rather than having source-build silently add patches without telling us. Some I don't really understand at all, such as the one that adds IsExternalInit: the compiler would not build without this type defined. I honestly don't feel particularly comfortable adding so many patch files that touch so much of the infrastructure directly, even if the goal is to immediately remove them.

333fred · 2021-04-19T21:42:05Z

eng/source-build-patches/0017-Rename-IReadOnlySet-and-ReferenceEqualityComparer-Ro.patch

+diff --git a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
+index 07c5b16..3522dfa 100644
+--- a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
+++ b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs


This touches too many files to realistically maintain for any amount of time as a patch.

eng/source-build-patches/0013-Add-file-to-include-missing-IsExternalInit-class.patch

eng/source-build-patches/0001-Conditionally-remove-net472-from-TargetFrameworks.patch

333fred

One suggestion and one question before merging.

333fred · 2021-04-20T22:27:15Z

eng/source-build-patches/0006-Dirty-revert-of-https-github.com-dotnet-roslyn-issue.patch

+From: Chris Rummel <crummel@microsoft.com>
+Date: Fri, 4 Sep 2020 11:50:51 -0500
+Subject: [PATCH 6/6] Dirty revert of
+ https://github.com/dotnet/roslyn/issues/10785


Doesn't have to be resolved right now, but @crummel what is this doing? This seems like it introduces a bug?

333fred · 2021-04-20T22:30:02Z

eng/source-build-patches/0012-Build-Roslyn-as-net6.0-in-source-build.patch

+From ed6e61977542a2fb1f69da41eb9806a4212d4046 Mon Sep 17 00:00:00 2001
+From: Davis Goodin <dagood@microsoft.com>
+Date: Mon, 28 Dec 2020 18:21:27 -0600
+Subject: [PATCH] Build Roslyn as net6.0 in source-build


@jaredpar this patch may end up being a bit troublesome. Not sure what you think.

Yeah this patch is problematic. It's essentially forcing us to support running on netcoreapp3.1 even though our code is developed with the assumption that this is just not possible.

What is the reasoning behind this? If we actually need to support netcoreapp3.1 then we may have to accept that. But even so I'd just add it back into the main tree. As long as it exists as a patch like this we're just going to be breaking support left and right here. It's not sustainable.

src/Compilers/Core/Portable/InternalUtilities/PlatformAttributes.cs

333fred · 2021-04-20T22:35:23Z

eng/build.sh

    /p:TreatWarningsAsErrors=true \
    /p:TestRuntimeAdditionalArguments=$test_runtime_args \
-    /p:DotNetBuildFromSource=$source_build \
+    /p:ArcadeBuildFromSource=$source_build \


Does this cause arcade to set the DotNetBuildFromSource environment variable?

Yes it does.

DotNetBuildFromSource is still the appropriate variable to use when conditionalizing for source-build.

MichaelSimons · 2021-04-21T15:49:58Z

@MichaelSimons there are a large number of patches here. Why so many? At one point in the 2.2-3.0 timeframe we had removed all the patches from source-build for roslyn, but we appear to have to have added more back since then, for things that really could and should have been fixed in source directly rather than having source-build silently add patches without telling us.

We (source-build devs and Fred from Roslyn) met on this. The pain and frustration of the patch maintenance and quality is one of the major reasons for the ArPow effort. The current source-build strategy has many flaws/downsides. After the discussion, a decision was made to use a feature branch to commit the ArPow infra and then work to remove the patches before merging to main. Source-build folks will be responsible for the ArPow infra and Roslyn folks will be responsible for the patch removal with assistance from source-build folks on understanding why patches are needed.

333fred

I'm going to go ahead and merge this into the feature branch for now, and we'll work on getting the patches actually merged into the repo. @MichaelSimons and @crummel, there are a couple of questions that need to be answered as well, but I'm not going to hold up the merge since we'll need to address them in follow ups anyway.

MichaelSimons added the Area-Infrastructure label Mar 9, 2021

MichaelSimons requested review from 333fred, crummel and dseefeld March 9, 2021 20:56

MichaelSimons commented Mar 9, 2021

View reviewed changes

eng/targets/Imports.targets Outdated

Copy link

Member Author

MichaelSimons Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue should get addressed before merging.

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

MichaelSimons added 2 commits April 16, 2021 16:19

ArPow stage 1: local source-build infra

89ceb3a

Conflicts: eng/Version.Details.xml

Cleanup and refactor source-build pipeline job

a0b4457

Conflicts: azure-pipelines.yml

MichaelSimons force-pushed the ArPow-Phase1-2 branch from 51d336f to a0b4457 Compare April 16, 2021 16:23

Revert temporary workarounds

12e5caf

MichaelSimons force-pushed the ArPow-Phase1-2 branch from 3592ee7 to 12e5caf Compare April 16, 2021 16:31

Update patches

a4245af

MichaelSimons force-pushed the ArPow-Phase1-2 branch from 7294849 to a4245af Compare April 16, 2021 21:33

MichaelSimons added 5 commits April 19, 2021 15:58

Merge branch 'main' of https://github.com/dotnet/roslyn into ArPow-Ph…

5ab1812

…ase1-2 Conflicts: eng/Version.Details.xml

Update patch

dae1f3c

Revert source-build workaround that was resolved w/latest arcarde

a24bc27

Edit patch

27bdfa0

Eliminate pre-builts

5cefd00

MichaelSimons marked this pull request as ready for review April 19, 2021 21:30

MichaelSimons requested review from a team as code owners April 19, 2021 21:30

333fred reviewed Apr 19, 2021

View reviewed changes

MichaelSimons changed the base branch from main to features/source-build April 20, 2021 20:56

Remove unnecessary IsExternalInit patch

0887c13

333fred reviewed Apr 20, 2021

View reviewed changes

Changes per code review

35b7ab8

333fred approved these changes Apr 21, 2021

View reviewed changes

333fred merged commit 2e0058d into dotnet:features/source-build Apr 21, 2021

jmarolf mentioned this pull request Feb 7, 2022

Add repo files to Roslyn.sln and Compilers.sln as solution items #59328

Closed

Conversation

MichaelSimons commented Mar 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Mar 9, 2021

Uh oh!

333fred commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelSimons commented Mar 9, 2021

Uh oh!

MichaelSimons commented Apr 19, 2021

Uh oh!

333fred commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

333fred 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!

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!

MichaelSimons commented Apr 21, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

333fred commented Mar 9, 2021 •

edited

Loading