@crummel and @dseefeld, can you please review since you are both familiar with the history on the patches?

What resolved the need for the following patches?

1. 0002-Do-not-build-NET-Fx-binaries-in-source-build
2. 0008-remove-RichCodeNav.EnvVarDump-on-source-build

I'm not sure if 0002 is required. I know we have similar patches in other repos. @crummel, do you have any idea on this one?

It looks like 0008 has been applied to eng/targets/Settings.props.

It looks like 0008 has been applied to eng/targets/Settings.props.

You're right, I somehow overlooked that. Thanks

0002-Do-not-build-NET-Fx-binaries-in-source-build

@MichaelSimons This was an unnecessary patch. The net472 reference assemblies are available in source build on all platforms. Even though these outputs aren't executable in source build, we have no difficulty in accurately producing them. It's better to produce additional (accurate) assemblies from the build than to increase conditional logic that moves source build further away from real builds.

This highlights my primary concern with the way source build is getting rolled out:

The initial proposal was thousands of lines of code, but I'm concerned most users won't realize that almost all of them are extraneous. By the time it was corrected for #53225, only ~50 changed lines remained.

@sharwell

This highlights my primary concern with the way source build is getting rolled out

Are you referring to the past (pre 6.0) or with the 6.0 ArPow plan? I don't think you will get much argument from folks that the previous SB approach had issues which is the motivation behind ArPow. The approach of utilizing patches outside the source repos is not maintainable and introduces quality issues. I just want to make sure you are onboard with ArPow.

@MichaelSimons My comment relates to the rollout process itself. For example, if Roslyn infrastructure was consulted earlier in the process, we could have provided information that these two extensive patches were completely unnecessary, and avoided all the work to create and then remove (without applying) them:

0002-Do-not-build-NET-Fx-binaries-in-source-build.patch
0012-Build-Roslyn-as-net6.0-in-source-build.patch

My comment relates to the rollout process itself. For example, if Roslyn infrastructure was consulted earlier in the process

Well, I think that's the pre-6.0 stuff. These patches were added between 2.2 and 6.0, and I've already given the feedback that we could have absolutely incorporated them (or shown that they were unnecessary) if we had been alerted that they had been added. That ship is several years sailed however.

@sharwell, Thanks for the clarification. The ArPow rollout process involves taking the existing source-build patches that exist in 5.0 and check them into the product repos along with the ArPow infrastructure. The consultation that you are referring to should have happened at the time of the initial patch creation. Certainly collaboration between source-build and repos owners in the past should have been better but keep in mind resources and timelines have always been a hinderance.

@MichaelSimons These are the most problematic statements from #52958:

The changes in these .patch files should be incorporated into the main branch of this repo to remove this possibility.

This issue tracks using git am eng/source-build-patches/*.patch and submitting a PR to incorporate the patches, or making changes to the repo that accomplish the same goals as the patches.

It would be better if the issue highlighted the fact that most of the proposed changes are unnecessary/obsolete, and the desired outcome for any unnecessary changes is deleting the patch without application rather than applying it. The current working reads as though this is either discouraged or not an option.