-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Introduce a join point for SdkLocator and VSTemplateLocator #43723
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
Conversation
/azp run sdk-unified-build-full |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -25,6 +25,9 @@ | |||
<BuildArgs>$(BuildArgs) /p:Architecture=$(TargetArchitecture)</BuildArgs> | |||
<BuildArgs>$(BuildArgs) /p:DOTNET_INSTALL_DIR=$(DotNetRoot)</BuildArgs> | |||
|
|||
<!-- Pass through DotNetBuildPass due to conditional behaviour for this project on join points --> | |||
<BuildArgs Condition="'$(DotNetBuildPass)' != ''">$(BuildArgs) /p:DotNetBuildPass=$(DotNetBuildPass)</BuildArgs> |
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 think this can be hoisted out to Directory.Build.props.
@@ -17,6 +17,7 @@ | |||
<InnerBuildArgs>$(InnerBuildArgs) /p:IncludeAdditionalSharedFrameworks=false</InnerBuildArgs> | |||
<InnerBuildArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true'">$(InnerBuildArgs) /p:DISABLE_CROSSGEN=true</InnerBuildArgs> | |||
<InnerBuildArgs Condition="'$(PgoInstrument)' == 'true'">$(InnerBuildArgs) /p:PgoInstrument=true</InnerBuildArgs> | |||
<InnerBuildArgs Condition="'$(DotNetBuildPass)' != ''">$(InnerBuildArgs) /p:DotNetBuildPass=$(DotNetBuildPass)</InnerBuildArgs> |
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 should be forwarded in the inner arcade command buildup: https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets#L67
You can keep it here for now though.
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 property shouldn't be needed at all. Global properties are automatically passed from the outer to the inner build.
that can pull assets from multiple legs. https://github.com/dotnet/source-build/issues/4336 | ||
--> | ||
<ExcludeFromDotNetBuild>true</ExcludeFromDotNetBuild> | ||
<ExcludeFromDotNetBuild Condition="'$(DotNetBuildPass)' == ''">true</ExcludeFromDotNetBuild> |
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.
Do you think there is likely to be a pass 3 (that isn't the merge pass)? If so, then I think we should be explicit about this being pass 2.
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 think it depends. If we think of pass 2 as "a pass that needs artifacts from pass 1 on other verticals" and pass 3 as "a pass that needs artifacts from pass 2 on other verticals", there could be scenarios where that's needed, especially around workloads.
Do we want to constrain this to only ever work in pass 2, or simply "not in pass 1, which by definition only has one vertical and therefore won't include the required packages"
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.
Also, would we expect a pass 2 job to upload all its packages (including the pass 1 stuff it already downloaded), therefore a pass 3 would have access to both pass 2 and pass 1 dependencies of a given job, therefore building these project files would work in pass 3+
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.
Yes, we should be explicit and only produce and upload artifacts that are needed in that build pass. I will submit a PR to fine tune this.
Does this exclude all the other sdk projects in pass 2? @ViktorHofer built a bit of infra to do this: https://github.com/dotnet/arcade/blob/f88082a3a89505296ec57f1a5f94fdc407fe1e58/Documentation/UnifiedBuild/Unified-Build-Join-Point.md?plain=1#L109 I think you may just need to include those two csprojs in the pass 2 itemgroup. |
Submitted #44153 as a follow-up. |
For full builds (i.e. all lanes, not the reduced CI-only set), this switches the Windows x64 build to second pass, so it can ingest packages from the Windows x86 and Windows ARM64 builds
Closes: dotnet/source-build#4336
Closes: dotnet/source-build#4633