Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ArPow stage 1: local source-build infrastructure #31235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ArPow stage 1: local source-build infrastructure #31235
Changes from all commits
7230a91
1440a78
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the TLDR info we need to understand "inner and outer" in this context as well as w/ respect to "inner builds"❔ I think inner builds will happen in one or more Docker containers after some setup on the "outer" build agent but am not sure…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Arcade Powered Source-Build infra re-clones the current repo somewhere in
artifacts/
, then runs msbuild on this source with source-build parameters. This msbuild invocation is what I am referring to the "inner" build here. As noted here this causes a conflict of the .globalconfig entries and therefore all settings are ignored.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CI, this seems unnecessary. Why not patch the current repo in-place❔
Even in a local build, dev could
git restore .
to undo the patches when they're done w/ source builds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way I think this problem will go away when we add our own CI and remove patches? I'm ready to merge this PR when this is clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a new source-build problem the ArPow effort caused. The 5.0 source-build infra didn't encounter this because it doesn't have an inner clone - it clones the source in the source-build repo. I could have introduced a patch to address the problem but the long term goal is to eliminate patches and since this was a is problem I wanted to find a permanent solution.
One thing to call out here is the patches only get applied to the inner clone. They get applied during each invocation of source-build. If a dev does 'git restore .' it isn't going to affect source-build because the patches aren't applied to the dev's repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems strange because this patch duplicates existing code in src/Tools/Directory.Build.targets. Should ensure (if we don't already) the tools don't build in the
$(DotNetBuildFromSource)
case. Bottom line, I'd rather we didn't have or need this patch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patches being introduced here are the current patches that exist in the current source-build. Our plan as described in the Arcade Powered Source-Build Implementation Plan, is for the repo teams to incorporate/eliminate/refactor these patches after getting the initial source-build infra in place. Some of these patches may be out of date, some may be better achieved via a different means, etc. The real key here is that we will have CI legs in each repo to ensure that the repo is always source-buildable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need this given https://github.com/dotnet/aspnetcore/blob/607e2164ec4a3fbe020bf3dda578ecb1ffd5f22e/Directory.Build.targets#L11-L19 and existing
'$(IsAspNetCoreApp)' != 'true'
clause in$(ExcludeFromSourceBuild)
setting a bit above.Good news is that as long as we use
$(SkipTestBuild)
for source builds, we shouldn't need many (any❔) special cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the global.json part of this patch but not why the other files (which shouldn't be involved in a source build) need to be touched. Could you explain❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say with certainty, it may have been the result of a global search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, global.json and the Arcade SDK don't support conditional SDKs. Is this something the Arcade team is working on❔ Otherwise, what's the recommended approach to handling SDKs we need almost everywhere but in source builds❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a suggestion. I'll bring this up in our next source-build sync and will reach out to Arcade on the possibility of supporting this scenario.