-
Notifications
You must be signed in to change notification settings - Fork 361
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
Enable Vertical build PoC as a more first-class concept #14274
Changes from 10 commits
dde16d2
3abeaeb
111bd97
03c73b1
280124e
085ba56
b7ea374
71b3ab2
c5d1803
77f14c2
7245f17
548be8c
b3ea9ab
f935735
b0844c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,8 @@ Param( | |
[switch] $pack, | ||
[switch] $publish, | ||
[switch] $clean, | ||
[switch] $sourceBuild, | ||
[switch] $verticalBuild, | ||
[switch][Alias('bl')]$binaryLog, | ||
[switch][Alias('nobl')]$excludeCIBinarylog, | ||
[switch] $ci, | ||
|
@@ -58,6 +60,8 @@ function Print-Usage() { | |
Write-Host " -sign Sign build outputs" | ||
Write-Host " -publish Publish artifacts (e.g. symbols)" | ||
Write-Host " -clean Clean the solution" | ||
Write-Host " -sourceBuild Run in 'source build' infra mode." | ||
Write-Host " -verticalBuild Run in 'vertical build' infra mode." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a new switch and not just use the already defined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This basically is the PoC mode for Windows and potentially other platforms. You need two switches to make thsi work:
ArcadeBuildVertical should be initially passed |
||
Write-Host "" | ||
|
||
Write-Host "Advanced settings:" | ||
|
@@ -120,6 +124,8 @@ function Build { | |
/p:Deploy=$deploy ` | ||
/p:Test=$test ` | ||
/p:Pack=$pack ` | ||
/p:ArcadeBuildFromSource=$sourceBuild ` | ||
/p:ArcadeBuildVertical=$verticalBuild ` | ||
/p:IntegrationTest=$integrationTest ` | ||
/p:PerformanceTest=$performanceTest ` | ||
/p:Sign=$sign ` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
|
||
<EnableDefaultNoneItems>false</EnableDefaultNoneItems> | ||
<_GeneratedVersionFilePath>$(IntermediateOutputPath)DefaultVersions.Generated.props</_GeneratedVersionFilePath> | ||
<NoWarn>$(NoWarn);3021;NU5105;SYSLIB0013</NoWarn> | ||
<NoWarn>$(NoWarn);3021;NU5105;NU5111;SYSLIB0013</NoWarn> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment above indicating what the individual NoWarns are for. The others were already there but adding a comment for each is a best practice that we follow in other places. |
||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
Configuration Build configuration: "Debug", "Release", etc. | ||
DotNetBuildFromSource Building the entire stack from source with no external dependencies. | ||
DotNetBuildVertical Building the entire stack in "Vertical" PoC mode. This is a superset of DotNetBuildFromSource. | ||
DotNetOutputBlobFeedDir Directory to publish Source Build assets to (packages, symbol packages, installers, etc.). | ||
DotNetPublishUsingPipelines Publish assets to Build Asset Registry. | ||
DotNetSymbolServerTokenMsdl Personal access token for MSDL symbol server. Available from variable group DotNet-Symbol-Server-Pats. | ||
|
@@ -47,7 +48,7 @@ | |
|
||
<Import Project="RepoLayout.props"/> | ||
|
||
<Import Project="SourceBuild/SourceBuildArcadeBuild.targets" Condition="'$(ArcadeBuildFromSource)' == 'true'" /> | ||
<Import Project="SourceBuild/SourceBuildArcadeBuild.targets" Condition="'$(ArcadeBuildFromSource)' == 'true' or '$(ArcadeBuildVertical)' == 'true'" /> | ||
|
||
<!-- Allow for repo specific Build properties such as the list of Projects to build --> | ||
<Import Project="$(RepositoryEngineeringDir)Build.props" Condition="Exists('$(RepositoryEngineeringDir)Build.props')" /> | ||
|
@@ -121,10 +122,10 @@ | |
<_CommonProps Include="VersionsPropsPath=$(VersionsPropsPath)"/> | ||
|
||
<!-- | ||
When building from source we suppress restore for projects that set ExcludeFromSourceBuild=true. | ||
When building from source we suppress restore for projects that set ExcludeFromSourceBuild=true or ExcludeFromVerticalBuild. | ||
NuGet Restore task reports a warning for such projects, which we suppress here. | ||
--> | ||
<_CommonProps Include="DisableWarnForInvalidRestoreProjects=true" Condition="'$(DotNetBuildFromSource)' == 'true'"/> | ||
<_CommonProps Include="DisableWarnForInvalidRestoreProjects=true" Condition="'$(DotNetBuildFromSource)' == 'true' or '$(DotNetBuildVertical)' == 'true'"/> | ||
|
||
<!-- | ||
C++ projects expect VCTargetsPath property to be set. MSBuild generates this property to solution | ||
|
@@ -296,14 +297,14 @@ | |
--> | ||
<MSBuild Projects="SourceBuild\AfterSourceBuild.proj" | ||
Properties="@(_CommonProps);_NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild" | ||
Condition="'$(ArcadeBuildFromSource)' == 'true'"/> | ||
Condition="'$(ArcadeBuildFromSource)' == 'true' or '$(ArcadeBuildVertical)' == 'true'"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This confuses me. Why would invoke source build infrastructure when not building from source? Which infrastructure from AfterSourceBuild.proj would you want a VMR build to execute? We don't create prebuilt usage reports in the VMR today when not building from source and I don't see that in scope for the PoCs. Similarly, I was under the impression that we don't want to create the source built intermediate nuget packages when not building from source. If there really is some common infrastructure (please explain which part), then we should rename the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to create the intermediate nupkgs coming out of each repo build for use in downstream builds for the orchestrated build infra to work today. IMO, we should keep the prebuilt detection in place for VB (but only error in "strict" SB mode). Yes, we should rename AfterSourceBuild to something else, but I want to do this after .NET 9: Redesign source-build behavior controls is complete. What we want to do with the PoC, IMO, is use the source build infra, while not necessarily using the SB repo behavior. |
||
|
||
<!-- | ||
Publish artifacts. | ||
--> | ||
<MSBuild Projects="Publish.proj" | ||
Properties="@(_PublishProps);_NETCORE_ENGINEERING_TELEMETRY=Publish" | ||
Targets="Publish" | ||
Condition="'$(Publish)' == 'true' and '$(DotNetBuildFromSource)' != 'true'"/> | ||
Condition="'$(Publish)' == 'true' and '$(DotNetBuildFromSource)' != 'true' and '$(DotNetBuildVertical)' != 'true'"/> | ||
Comment on lines
302
to
+308
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make more sense to just not pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maaybe? I'm not sure why that was put there in the first place so I'm skeptical of removing it at this point. |
||
</Target> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
<# | ||
.SYNOPSIS | ||
Creates a git clone of <source> into <dest> directory, optionally saving WIP changes. | ||
.PARAMETER Source | ||
The source Git directory | ||
.PARAMETER Dest | ||
The destination Git directory. Created if doesn't exist. | ||
.PARAMETER Clean | ||
If dest directory already exists, delete it. | ||
.PARAMETER CopyWip | ||
Transfer most types of uncommitted change into the destination directory. Useful for dev workflows. | ||
#> | ||
|
||
param ( | ||
[Parameter(Mandatory=$true)] | ||
[string]$Source, | ||
[Parameter(Mandatory=$true)] | ||
[string]$Dest, | ||
[switch]$Clean, | ||
[switch]$CopyWip | ||
) | ||
|
||
if (-not (Test-Path $Source -PathType Container)) { | ||
Write-Host "-Source not a directory: $Source" | ||
exit 1 | ||
} | ||
|
||
Write-Host "Cloning repository at: $Source -> $Dest ..." | ||
|
||
if (Test-Path $Dest) { | ||
Write-Host "Destination already exists!" | ||
if (Test-Path $Dest -PathType Leaf) { | ||
Write-Host "Existing destination is a regular file, not a directory. This is unusual: aborting." | ||
exit 1 | ||
} | ||
elseif ($Clean) { | ||
Write-Host "Clean is enabled: removing $dest and continuing..." | ||
Remove-Item -Path $Dest -Recurse -Force | ||
} | ||
else { | ||
Write-Host "Clean is not enabled: aborting." | ||
exit 1 | ||
} | ||
} | ||
|
||
Push-Location -Path $Source | ||
|
||
if (-not (Test-Path $Dest)) { | ||
New-Item -Path $Dest -ItemType Directory | Out-Null | ||
|
||
if ($CopyWip) { | ||
# Copy over changes that haven't been committed, for dev inner loop. | ||
# This gets changes (whether staged or not) but misses untracked files. | ||
$stashCommit = (git stash create) | ||
|
||
if ($stashCommit) { | ||
Write-Host "WIP changes detected: created temporary stash $stashCommit to transfer to inner repository..." | ||
} | ||
else { | ||
Write-Host "No WIP changes detected..." | ||
} | ||
} | ||
|
||
Write-Host "Creating empty clone at: $Dest" | ||
|
||
# Combine the results of git rev-parse --git-dir with the "shallow" subdirectory | ||
# to get the full path to the shallow file. | ||
|
||
$shallowFile = Join-Path (git rev-parse --git-dir) "shallow" | ||
|
||
if (Test-Path $shallowFile) { | ||
Write-Host "Source repository is shallow..." | ||
if ($stashCommit) { | ||
Write-Host "WIP stash is not supported in a shallow repository: aborting." | ||
exit 1 | ||
} | ||
|
||
# If source repo is shallow, old versions of Git refuse to clone it to another directory. First, | ||
# remove the 'shallow' file to trick Git into allowing the clone. | ||
$shallowContent = Get-Content -Path $shallowFile | ||
Remove-Item -Path $shallowFile | ||
|
||
# Then, run the clone: | ||
# * 'depth=1' avoids encountering the leaf commit in the shallow repo that points to a parent | ||
# that doesn't exist. (The commit marked "grafted".) Git would fail here, otherwise. | ||
# * '--no-local' allows a shallow clone from a git dir on the same filesystem. This means the | ||
# clone will not use hard links and takes up more space. However, since we're doing a shallow | ||
# clone anyway, the difference is probably not significant. (This has not been measured.) | ||
# * '-c protocol.file.allow=alwyas allows cloning from the local filesystem even on newer versions | ||
# of git which have disabled this for security reasons. | ||
git clone -c protocol.file.allow=always --depth=1 --no-local --no-checkout "$Source" "$Dest" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When native executables calls fail, the error is not propagated in PowerShell (unless I'm missing some external setup) and $LASTEXITCODE has to be checked. Do we have a solution for this in arcade (such as a wrapper function for calling native binaries that checks the LASTEXITCODE and throws if non-zero) or that's something that we'd need to do yet? |
||
|
||
# Restore the 'shallow' file | ||
Set-Content -Path $shallowFile -Value $shallowContent | ||
} else { | ||
git clone -c protocol.file.allow=always --no-checkout "$Source" "$Dest" | ||
} | ||
|
||
echo "Checking out sources..." | ||
# If no changes were stashed, stashCommit is empty string, and this is a simple checkout. | ||
git -C $Dest checkout $stashCommit | ||
echo "Clone complete: $Source -> $Dest" | ||
|
||
} | ||
|
||
Pop-Location |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ | |
MSBuild tasks and tools should use this version to target the latest TFM that is supported by tooling. | ||
Identical with NetCurrent when building from source. --> | ||
<NetToolCurrent Condition="'$(DotNetBuildFromSource)' != 'true'">net8.0</NetToolCurrent> | ||
<NetToolCurrent Condition="'$(DotNetBuildFromSource)' == 'true'">$(NetCurrent)</NetToolCurrent> | ||
<NetToolCurrent Condition="'$(DotNetBuildFromSource)' == 'true' or '$(DotNetBuildVertical)' == 'true'">$(NetCurrent)</NetToolCurrent> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks wrong. We don't want to change the meaning of TFMs when building from the VMR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're probably right on this one. |
||
|
||
<!-- Lowest version of .NET at the time of the release of NetCurrent that is supported by tooling. | ||
Undefined when NetToolCurrent and NetToolMinimum are identical. --> | ||
|
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 doesn't look right. The same switch exists in the build.sh file for Unix platforms but does something completely different:
arcade/eng/common/build.sh
Lines 22 to 24 in 7245f17
What does "source build" infra mode mean?
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 added this for completeness. It would end up being "windows build without prebuilts". I can remove it.
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.
OK. Let's remove this until there's a need and until it works with the existing infrastructure.