Enable Vertical build PoC as a more first-class concept #14274

mmitche · 2023-12-08T19:24:33Z

For the vertical builds, we need some of the arcade infra required for source build, but not all. This PR introduces two switches:

ArcadeBuildVertical - Enables arcade infra for vertical build. Similar to source build but without some SB customization (e.g. filtering)
DotNetBuildVertical - Passed to inner build when ArcadeBuildVertical is enabled.

Note that BOTH switches may be passed if you wanted.

I probably could have introduced a more polished switch structure here. But I don't want to disturb existing SB functionality before a full design and rollout plan is determined.

In addition:

Add inner-clone support for Windows.
Add ExcludeFromVerticalBuild switch. We could use ExcludeFromBuild for the PoCs but there are other uses of ExcludeFromBuild in the product and I want to avoid confusion.

To double check:

The right tests are in and and the right validation has happened. Guidance: https://github.com/dotnet/arcade/tree/main/Documentation/Validation

For the vertical builds, we need some of the arcade infra required for source build, but not all. This PR introduces two switches: - ArcadeBuildVertical - Enables arcade infra for vertical build. Similar to source build but without some SB customization (e.g. filtering) - DotNetBuildVertical - Passed to inner build when ArcadeBuildVertical is enabled. Note that BOTH switches may be passed if you wanted.

…ject

mmitche · 2023-12-08T23:48:36Z

https://dev.azure.com/dnceng/internal/_build/results?buildId=2332244&view=results

mmitche · 2023-12-08T23:49:23Z

@ViktorHofer This works now when building arcade with itself with -verticalBuild switch on windows. I think we'll need this infra to get the PoC working.

It should be possible to pull a build of this bootstrap arcade into your branch of the PoC. The PoC should be passing:

ArcadeBuildVertical.

This workaround is required for arcade to build itself with this in vb mode, Something odd about the in-tree usage of the built xliff binaries in this specific project. Nothing jumped out and it looks like it's not critical for fact finding, so I moved on. https://github.com/dotnet/arcade/pull/14274/files#diff-8e1f1364a8ce67c9d91692b95b9e2235e4de9b6efe15e2f05b61be99dd04f66cR42-R43

ViktorHofer · 2023-12-09T08:56:54Z

src/Microsoft.DotNet.Arcade.Sdk/Microsoft.DotNet.Arcade.Sdk.csproj

@@ -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>


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.

ViktorHofer · 2023-12-09T09:00:23Z

src/Microsoft.DotNet.Arcade.Sdk/tools/TargetFrameworkDefaults.props

@@ -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>


That looks wrong. We don't want to change the meaning of TFMs when building from the VMR.

Yeah, you're probably right on this one.

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeTools.targets

ViktorHofer · 2023-12-09T09:07:15Z

eng/common/build.ps1

@@ -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."


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

echo " --sourceBuild Source-build the solution (short: -sb)"

echo " Will additionally trigger the following actions: --restore, --build, --pack"

echo " If --configuration is not set explicitly, will also set it to 'Release'"

What does "source build" infra mode mean?

I added this for completeness. It would end up being "windows build without prebuilts". I can remove it.

OK. Let's remove this until there's a need and until it works with the existing infrastructure.

ViktorHofer · 2023-12-09T09:08:17Z

eng/common/build.ps1

@@ -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."


Why do we need a new switch and not just use the already defined DotNetBuildVertical one? What does "vertical build infra mode" mean?

This basically is the PoC mode for Windows and potentially other platforms. You need two switches to make thsi work:

ArcadeBuildVertical

DotNetBuildVertical

ArcadeBuildVertical should be initially passed

ViktorHofer · 2023-12-09T09:13:43Z

src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

    <!--
      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'"/>


Wouldn't it make more sense to just not pass the -publish argument in when building the repos from the VMR? That's done here: https://github.com/dotnet/dotnet/blob/ab06cf16219bc309a2a725008556abbbdab01007/repo-projects/Directory.Build.props#L152

Maaybe? I'm not sure why that was put there in the first place so I'm skeptical of removing it at this point.

ViktorHofer · 2023-12-09T09:20:24Z

src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

@@ -296,14 +297,14 @@
    -->
    <MSBuild Projects="SourceBuild\AfterSourceBuild.proj"
             Properties="@(_CommonProps);_NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild"
-             Condition="'$(ArcadeBuildFromSource)' == 'true'"/>
+             Condition="'$(ArcadeBuildFromSource)' == 'true' or '$(ArcadeBuildVertical)' == 'true'"/>


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 AfterSourceBuild.proj file or create a new AfterBuildVertical.proj and share the code between both projects, i.e. via a targets file.

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.

ViktorHofer

Thank you for working on this. I see a lot of improvements but also a few changes that I don't understand / that don't make sense to me. Marking as "Request changes" to prevent an accidental merge before we resolve the feedback.

…rcadeTools.targets Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

tkapin · 2023-12-11T19:36:22Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/git-clone-to-dir.ps1

+      #   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"


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?

… into enable-vb-in-arcade

mmitche requested review from MichaelSimons and ViktorHofer December 8, 2023 19:24

Bind variable

3abeaeb

steveisok self-requested a review December 8, 2023 20:21

steveisok previously approved these changes Dec 8, 2023

View reviewed changes

mmitche added 7 commits December 8, 2023 12:29

Fix parens

111bd97

Fix parens

03c73b1

Call powershell for script

280124e

Fix powershell call for script

085ba56

Fix powershell call for script

b7ea374

Various fixes for conditions

71b3ab2

Quote the dotnet tool path in the env var in case of spaces

c5d1803

mmitche dismissed steveisok’s stale review via c5d1803 December 8, 2023 23:07

mmitche added 2 commits December 8, 2023 15:24

Exclude asmdiff (unknown issue) and import the source build tools pro…

77f14c2

…ject

Fix conditionals. Don't run validate prebuilts in VB mode

7245f17

ViktorHofer reviewed Dec 9, 2023

View reviewed changes

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeTools.targets Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 9, 2023

View reviewed changes

ViktorHofer requested changes Dec 9, 2023

View reviewed changes

Update src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildA…

548be8c

…rcadeTools.targets Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

tkapin reviewed Dec 11, 2023

View reviewed changes

mmitche added 2 commits December 11, 2023 12:22

Remove SB switch from eng/common/build.ps1

b3ea9ab

Revert tool TFM

f935735

Merge branch 'enable-vb-in-arcade' of https://github.com/mmitche/arcade…

b0844c7

… into enable-vb-in-arcade

ViktorHofer approved these changes Dec 11, 2023

View reviewed changes

mmitche merged commit 0a0217f into dotnet:main Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Vertical build PoC as a more first-class concept #14274

Enable Vertical build PoC as a more first-class concept #14274

mmitche commented Dec 8, 2023 •

edited

Loading

mmitche commented Dec 8, 2023

mmitche commented Dec 8, 2023 •

edited

Loading

ViktorHofer Dec 9, 2023

ViktorHofer Dec 9, 2023

mmitche Dec 11, 2023

ViktorHofer Dec 9, 2023

mmitche Dec 11, 2023

ViktorHofer Dec 11, 2023

ViktorHofer Dec 9, 2023 •

edited

Loading

mmitche Dec 11, 2023

ViktorHofer Dec 9, 2023

mmitche Dec 11, 2023

ViktorHofer Dec 9, 2023

mmitche Dec 11, 2023

ViktorHofer left a comment •

edited

Loading

tkapin Dec 11, 2023

	echo " --sourceBuild Source-build the solution (short: -sb)"
	echo " Will additionally trigger the following actions: --restore, --build, --pack"
	echo " If --configuration is not set explicitly, will also set it to 'Release'"

Enable Vertical build PoC as a more first-class concept #14274

Enable Vertical build PoC as a more first-class concept #14274

Conversation

mmitche commented Dec 8, 2023 • edited Loading

To double check:

mmitche commented Dec 8, 2023

mmitche commented Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmitche commented Dec 8, 2023 •

edited

Loading

mmitche commented Dec 8, 2023 •

edited

Loading

ViktorHofer Dec 9, 2023 •

edited

Loading

ViktorHofer left a comment •

edited

Loading