Skip to content

Conversation

@omajid
Copy link
Member

@omajid omajid commented Mar 5, 2021

This enables 'source-build', which makes it easier to build the entire shipping .NET SDK from source.

This is the first and second step of arcade-powered-source-build: https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/README.md

See dotnet/sourcelink#692 for a similar PR, that this is based on.

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci-build branch 2 times, most recently from 83fe8ec to 8133033 Compare March 6, 2021 00:34
@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #4930 (437b074) into main (9e5f533) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4930      +/-   ##
==========================================
+ Coverage   95.54%   95.59%   +0.04%     
==========================================
  Files        1185     1188       +3     
  Lines      272465   272671     +206     
  Branches    16428    16472      +44     
==========================================
+ Hits       260338   260649     +311     
+ Misses      10001     9895     -106     
- Partials     2126     2127       +1     

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci-build branch from 8133033 to 35ce29a Compare March 12, 2021 16:14
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes this error I was seeing:

roslyn-analyzers/artifacts/source-build/self/src/src/Utilities/Compiler/WellKnownTypeProvider.cs(87,10): error CS0122: 'PerformanceSensitiveAttribute' is inaccessible due to its protection level [roslyn-analyzers/artifacts/source-build/self/src/src/Microsoft.CodeAnalysis.Analyzers/Core/Microsoft.CodeAnalysis.Analyzers.csproj]

I have no idea what the consequences of this change might be.

Choose a reason for hiding this comment

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

This error would be reported if the build incorrectly omitted Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers, which is a required analyzer package.

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci-build branch from 35ce29a to 55d6a13 Compare March 12, 2021 16:31
Copy link
Member Author

Choose a reason for hiding this comment

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

I got some feedback about the CS warnings (safe to disable), but not sure about the AD ones... looks like the VB analogue of CS

Choose a reason for hiding this comment

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

These should be moved to a conditionally-included .globalconfig in eng/config/globalconfigs. See dotnet/roslyn for an example:

https://github.com/dotnet/roslyn/tree/main/eng/config/globalconfigs

Copy link
Member Author

Choose a reason for hiding this comment

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

This duplicates the contents eng/common/templates/jobs/source-build.yaml. Is there some way to just delegate to that (but only for source-build)?

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci-build branch from 55d6a13 to ff96e2d Compare March 12, 2021 16:33
@omajid omajid changed the title WIP: Add arcade-powered-source-build local and ci builds Add arcade-powered-source-build local and ci builds Mar 12, 2021
@omajid omajid marked this pull request as ready for review March 12, 2021 16:35
@omajid omajid requested a review from a team as a code owner March 12, 2021 16:35
@mavasani mavasani requested a review from jmarolf March 12, 2021 16:51
@mavasani
Copy link

@jmarolf Can you please review?

@sharwell sharwell marked this pull request as draft March 12, 2021 17:03

Choose a reason for hiding this comment

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

AD0001 should not be in this list.

Choose a reason for hiding this comment

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

CS8600 and many others in this list fall under nullable. If there is intent to disable all NRT warnings, all items under the nullable identifier umbrella should be removed from this list, and nullable used by name instead.

Choose a reason for hiding this comment

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

IDE0005 should not be in this list.

Comment on lines +25 to +26

Choose a reason for hiding this comment

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

❗ We should not patch RoslynAnalyzers.sln. If you need to build a subset of projects, create a SourceBuild.slnf solution filter, which is derived from RoslynAnalyzers.sln and only contains the projects needed for the build.

Choose a reason for hiding this comment

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

This patch is going to break every time we update the code analysis version. We should find a different approach.

Comment on lines +33 to +34

Choose a reason for hiding this comment

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

netstandard1.3 is the correct target for this package. Builds using netstandard2.0 would not be correct.

Comment on lines +123 to +124

Choose a reason for hiding this comment

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

❗ None of the *.Setup.csproj projects should be included in the solution filter which is used for source build. As such, no changes are expected to any of them.

Choose a reason for hiding this comment

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

😕 Why the suppression here?

Copy link

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

The patches here are producing a totally different build than the repository. If we aren't going to build the real branch properly, we need to make a new branch which contains the code used for source build so it's clear the two build outputs are not equivalent. This branch would not need or use any patches.

@omajid
Copy link
Member Author

omajid commented Mar 12, 2021

@crummel @dseefeld @MichaelSimons any thoughts about the separate branching?

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci-build branch from ff96e2d to 437b074 Compare March 12, 2021 17:28
@omajid
Copy link
Member Author

omajid commented Mar 12, 2021

I am going to echo something @dagood wrote here:

I think we should merge this PR as-is, and fix up the patches and this functional issue as followup. This PR represents the state of dotnet/source-build, it's not necessary to solve every source-build issue right here and now.

I dont think a separate branch will work. https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/implementation-plan.md#stage-descriptions says:

  • Merging into main/master strongly preferred.
  • If a dev branch is created, the repo team must merge it into main/master before ending stage (3).

The end goal is that the exact same git commit that produces official releases can be used for source-build.

@dseefeld
Copy link

The patches here are producing a totally different build than the repository. If we aren't going to build the real branch properly, we need to make a new branch which contains the code used for source build so it's clear the two build outputs are not equivalent. This branch would not need or use any patches.

I echo @omajid's comments above. Creating a branch defeats the purpose of what we're trying to do with Arcade-powered source-build. The goal is to build source-build alongside the official build to ensure that all repos can be built from source, rather than doing this after the fact during a release. The patches represent changes that the source-build team has made to be able to build the repo with source-build. The idea is that these would be merged in and maintained in the main code base and that an additional source-build PR validation build would be added to exercise them.

@omajid
Copy link
Member Author

omajid commented Mar 17, 2021

@mavasani @jmarolf @sharwell Hey folks, any updates? Are you totally opposed to our approach of merging in these patches and then coming back later to clean them up?

@sharwell
Copy link

sharwell commented Mar 17, 2021

I don't believe the proposed approach is maintainable for individual repositories (i.e. the division of work pushes too many changes from devops to individual repository contributors). This impairs the ability to be a proper open source project with external contributors, and the value to those contributors is minimal if any. I would prefer the design be revisited before trying to add it to the main branch.

CS1712: Type parameter 'parameter' has no matching typeparam tag in the XML comment on 'type_or_member' (but other type parameters do)
-->
<NoWarn Condition="'$(Language)' == 'C#'">$(NoWarn),1573,1591,1712</NoWarn>
<NoWarn Condition=" '$(DotnetBuildFromSource)' == 'true' ">$(NoWarn);AD0001;CS8600;CS8601;CS8602;CS8603;CS8604;CS8605;CS8609;CS8619;CS8620;CS8625;CS8621;CS8631;CS8714;CS8762;CS8765;IDE0005</NoWarn>

Choose a reason for hiding this comment

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

It's not clear why source build would be different from normal builds here. We have warnings treated as errors in our build, and the build is clean. If these warnings start to show up for source build, then source build is by definition not matching our actual build and cannot be considered valid.

@@ -0,0 +1,23 @@
<Project>

Choose a reason for hiding this comment

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

❔ Can we move the source build supporting files to a subfolder?

@sharwell
Copy link

sharwell commented Mar 17, 2021

There are so many problems with the proposed patches, it's really difficult for me to see a viable path from this proposal forward to something that works. It certainly seems easier to just remove all of them, and work through the resulting issues by making the build machine match repository expectations rather than trying to update the code to a different reality. The number/severity of deviations in the resulting artifacts certainly warrants a new branch, if not an entirely different fork.

@sharwell sharwell changed the base branch from main to features/source-build March 19, 2021 21:05
@sharwell sharwell marked this pull request as ready for review March 19, 2021 21:06
@sharwell sharwell merged commit 4aba1a5 into dotnet:features/source-build Mar 19, 2021
@omajid
Copy link
Member Author

omajid commented Mar 19, 2021

Thanks for merging this! Please let me know how you want to coordinate a cleanup.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants