Support OfficialBuildId for SB #48327

mthalman · 2025-04-09T22:50:05Z

This enables SB support for the OfficialBuildId property. This gets enabled by setting the EnableDevBuildAsDefaultForSourceOnly property. When that is enabled, SB will use the OfficialBuildId rather than the build numbers from the git-info files. This also allows for dev builds to be supported.

For dev builds, it was necessary to change the pre-release label defined by Arcade to use a different value. This is due to the semantic versioning rules that select the highest version as the last label value in alphabetical sorting. This is problematic when we have pre-release labels like "preview" which end up being selected as the latest version over a version that has the label of "dev" since "preview" comes after "dev" alphabetically. This means that preview packages will be resolved instead of the live dev package when resolving package dependencies. For that reason, a prefix value of "test" was chosen which comes alphabetically after all the other pre-release labels that are used.

The majority of these changes are related to the issue of testing. For the SB tests and scenario tests, we need to be able to resolve the package version of the OfficialBuildId that was provided. But some of those packages aren't produced by source build. For example, some of the self-contained publishing tests rely on packages for the portable linux-<arch> RID which is not produced by source build. For that reason, we require the packages that are produced by other unified build legs. So to test the SB output with OfficialBuildId support, we actually need to integrate a SB leg into the UB pipeline. A separate validation leg is then added that depends on the artifacts from the SB leg as well as other UB legs that provide the necessary packages required by the tests.

Another factor is building the scenario tests. This required modification because it was dependent on building the entire VMR. So that's been decoupled here for SB to allow building and running the scenario tests in a separate job without rebuilding the rest of the VMR.

Also related to the scenario tests is the need to remove the Versions.Details dependency on System.CommandLine. This was necessary because leaving it in would require SB to attempt to use the "live" version of that package which wouldn't exist in this context since it's been decoupled from the rest of the VMR.

Copilot

Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.

Files not reviewed (10)

src/Layout/Directory.Build.props: Language not supported
src/Layout/redist/targets/RestoreLayout.targets: Language not supported
src/SourceBuild/content/repo-projects/Directory.Build.props: Language not supported
src/SourceBuild/content/repo-projects/Directory.Build.targets: Language not supported
src/SourceBuild/content/repo-projects/scenario-tests.proj: Language not supported
src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Tests/Microsoft.DotNet.SourceBuild.Tests.csproj: Language not supported
src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Tests/assets/default.NuGet.Config: Language not supported
src/SourceBuild/content/test/tests.proj: Language not supported
src/SourceBuild/patches/arcade/0001-Use-test-prefix-on-unofficial-build-prerelease-label.patch: Language not supported
src/SourceBuild/patches/scenario-tests/0001-Remove-System.CommandLine-as-dependency.patch: Language not supported

Comments suppressed due to low confidence (3)

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Tests/DotNetHelper.cs:52

Verify that the updated ternary logic correctly differentiates between custom package path modes. In particular, ensure that when CustomPackagesPathIsInclusive is true, the 'default' configuration is intended.

string nugetConfigPrefix = useCustomPackages && !Config.CustomPackagesPathIsInclusive ? "custom" : "default";

eng/pipelines/templates/jobs/vmr-build.yml:168

Ensure that the conditional aggregation for 'dependsOn' correctly builds the dependency list in all scenarios; verify that an empty condition does not add spurious list items.

dependsOn:
    - ${{ if ne(parameters.reuseBuildArtifactsFrom, '') }}:
      - ${{ parameters.reuseBuildArtifactsFrom }}
    - ${{ if ne(parameters.dependsOn, '') }}:
      - ${{ parameters.dependsOn }}

eng/pipelines/templates/jobs/vmr-build.yml:565

Removing the trimming of buildArgs may lead to extra spaces affecting command-line parsing; confirm that buildArgs formatting remains valid after this change.

buildArgs=$(echo $buildArgs | xargs) # Remove extra spaces

ViktorHofer · 2025-04-10T09:04:07Z

Love it. This is a great step towards unified versioning and build for source-build. The PR description is well written. I still have follow-up questions, though.

Another factor is building the scenario tests. This required modification because it was dependent on building the entire VMR. So that's been decoupled here for SB to allow building and running the scenario tests in a separate job without rebuilding the rest of the VMR.

That's definitely the right direction. We started decoupling scenario-tests from the VMR build when we moved it out of the Build target and into the Test target. I see this as the next logical step to truly build it independently. On that thought, can you remind me what the constraints for source-build are in terms of pre-builts for scenario-tests execution? Is the test execution constraint by internet access and must have no pre-builts? Does this change with this work?

EnableDevBuildAsDefaultForSourceOnly

I'm suprised to see this new switch. Is your intention to make this the default at a later point, past P4? If so, what will be the UX for source-build partners?

eng/pipelines/templates/jobs/vmr-build.yml

ViktorHofer · 2025-04-10T09:40:48Z

src/Layout/Directory.Build.props

Please revert this file change

Fixed with 269eba9

src/Layout/redist/targets/RestoreLayout.targets

ViktorHofer · 2025-04-10T09:42:19Z

src/SourceBuild/content/repo-projects/Directory.Build.props

+    <!-- TODO: Remove this import and the EnableDevBuildAsDefaultForSourceOnly when dev builds are the default for source only.
+         https://github.com/dotnet/source-build/issues/4922 -->


nit: the comment refers to an Import but below is a property group. The same is true for the change in L176

Fixed with 2e4029b

ViktorHofer · 2025-04-10T09:45:01Z

src/SourceBuild/content/repo-projects/Directory.Build.targets

@@ -198,7 +198,7 @@

    <ItemGroup>
      <_BuildSources Include="$(PrebuiltNuGetSourceName);$(PreviouslySourceBuiltNuGetSourceName);$(ReferencePackagesNuGetSourceName)"
-                     Condition="'$(DotNetBuildSourceOnly)' == 'true'"/>
+                     Condition="'$(DotNetBuildSourceOnly)' == 'true' and '$(DotNetSourceOnlyTestOnly)' != 'true'"/>


Would this benefit from a boolean indirection property that determines whether prebuilt feeds should be added? i.e.

<IncludePrebuiltFeeds Condition="'$(DotNetSourceOnlyTestOnly)' == 'true'">false</IncludePrebuiltFeeds> <IncludePrebuiltFeeds Condition="'$(IncludePrebuiltFeeds)' == '' and '$(DotNetBuildSourceOnly)' == 'true'">true</IncludePrebuiltFeeds>

Fixed with 2eb97de

ViktorHofer · 2025-04-10T09:46:55Z

src/SourceBuild/content/repo-projects/scenario-tests.proj

@@ -9,21 +9,19 @@

    <!-- Don't use the updated NuGet.config file that includes the live package feeds when testing source-only as in that configuration
         the Microsoft built packages should be used. -->
-    <TestArgs Condition="'$(DotNetBuildSourceOnly)' == 'true'">$(TestArgs) /p:TestRestoreConfigFile=$(OriginalNuGetConfigFile)</TestArgs>
+    <TestArgs Condition="'$(DotNetBuildSourceOnly)' == 'true' and '$(DotNetSourceOnlyTestOnly)' != 'true'">$(TestArgs) /p:TestRestoreConfigFile=$(OriginalNuGetConfigFile)</TestArgs>
+    <UseBootstrapArcade>true</UseBootstrapArcade>


It's weird to have this un-conditionally set to true but then later for non-source-only builds, reference the arcade repo. Maybe add a condition?

Fixed with 483c975

ViktorHofer · 2025-04-10T09:49:48Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Tests/assets/default.NuGet.Config

@@ -8,5 +8,6 @@
    <add key="darc-pub-dotnet-runtime-d398172" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-d3981726/nuget/v3/index.json" />
    <add key="dotnet10" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet10/nuget/v3/index.json" />
    <add key="dotnet10-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet10-transport/nuget/v3/index.json" />
+    <add key="custom-packages" value="CUSTOM_PACKAGE_FEED" />


Did you consider using the RestoreAdditionalProjectSources msbuild property to dynamically add additional nuget feeds? That would avoid rewriting this file.

Great suggestion. Fixed with 5a86952

ViktorHofer · 2025-04-10T09:52:46Z

src/SourceBuild/patches/arcade/0001-Use-test-prefix-on-unofficial-build-prerelease-label.patch

+     <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'release'"/>
+-    <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease' and '$(SemanticVersioningV1)' != 'true'">$(_PreReleaseLabel).final</VersionSuffix>
+-    <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease' and '$(SemanticVersioningV1)' == 'true'">$(_PreReleaseLabel)-final</VersionSuffix>
+    <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease' and '$(SemanticVersioningV1)' != 'true'">$(_PreReleaseLabel)$(_SemVerLabelSeparator)final</VersionSuffix>


I think this was the idea?

Suggested change

+ <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease' and '$(SemanticVersioningV1)' != 'true'">$(_PreReleaseLabel)$(_SemVerLabelSeparator)final</VersionSuffix>

+ <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease'">$(_PreReleaseLabel)$(_SemVerLabelSeparator)final</VersionSuffix>

Fixed with 392a5e6

ViktorHofer · 2025-04-10T09:59:20Z

src/SourceBuild/content/test/tests.proj

@@ -21,6 +21,7 @@
  </PropertyGroup>

  <ItemGroup>
+    <ProjectReference Include="$(TasksDir)Microsoft.DotNet.UnifiedBuild.Tasks\Microsoft.DotNet.UnifiedBuild.Tasks.csproj" />


From the entire PR, this is the only change that I don't understand and which looks wrong to me. Why would we need to build a tasks assembly here in tests.proj?

@mthalman can this be moved to test/Microsoft.DotNet.SourceBuild.Tests/Microsoft.DotNet.SourceBuild.Tests.csproj? We have the same ProjectReference in https://github.com/dotnet/sdk/blob/main/src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj

This reference isn't necessary anymore. Removed with e87b02f

ViktorHofer · 2025-04-10T10:01:34Z

...ld/content/test/Microsoft.DotNet.SourceBuild.Tests/Microsoft.DotNet.SourceBuild.Tests.csproj

@@ -62,6 +62,9 @@
      <RuntimeHostConfigurationOption Include="$(MSBuildProjectName).CustomPackagesPath">
        <Value>$(SourceBuildTestsCustomSourceBuiltPackagesPath)</Value>
      </RuntimeHostConfigurationOption>
+      <RuntimeHostConfigurationOption Include="$(MSBuildProjectName).CustomPackagesPathIsInclusive">
+        <Value>$(SourceBuildTestsCustomSourceBuiltPackagesPathIsInclusive)</Value>


Can you please add a comment describing this property? I assume it means that the packages path contains everything needed for this test library?

This property has been removed as part of 5a86952

mthalman · 2025-04-10T15:07:58Z

On that thought, can you remind me what the constraints for source-build are in terms of pre-builts for scenario-tests execution? Is the test execution constraint by internet access and must have no pre-builts? Does this change with this work?

There are no requirements that tests build without prebuilts. Internet access is allowed for running tests. The changes in this PR don't impact this.

EnableDevBuildAsDefaultForSourceOnly

I'm suprised to see this new switch. Is your intention to make this the default at a later point, past P4? If so, what will be the UX for source-build partners?

Yes, I should have explained that better. I also had a hard time naming it because it's not just specific to dev builds. It applies to official builds too when OfficialBuildId is set. It basically means that we use dev builds or OfficialBuildId instead of git-info. This is a temporary property until we have a fully combined pipeline that includes Microsoft and SB legs. So this property is only set in the new SB leg added to the UB pipeline. This allows the existing SB pipeline to work unchanged. The UX for SB partners at this point is unchanged. I'm open to changing the default behavior here if that's what we want to do (cc @MichaelSimons). We're just in a state of transition right now while we have separate pipelines.

ViktorHofer · 2025-04-10T15:24:15Z

The UX for SB partners at this point is unchanged. I'm open to changing the default behavior here if that's what we want to do (cc @MichaelSimons). We're just in a state of transition right now while we have separate pipelines.

I like the intermediate step as it makes the PR smaller and scoped. My main question here was whether the transition will happen in time for P4 or P5.

eng/pipelines/templates/stages/vmr-verticals.yml

MichaelSimons · 2025-04-10T19:20:46Z

eng/pipelines/templates/stages/vmr-verticals.yml

@@ -995,3 +995,77 @@ stages:
            - Windows_x64
            - Windows_x86
            - Windows_arm64
+
+        # Source Build Leg


Is there value in embedding the SB leg and SB validation leg in new jobs template? The benefit is to ensure they both get done and help in ensuring parameter consistency between the two - e.g. containers, naming, etc. This would make it easier to add new SB legs.

Fixed with b8206dd

MichaelSimons · 2025-04-10T19:27:59Z

I like the intermediate step as it makes the PR smaller and scoped. My main question here was whether the transition will happen in time for P4 or P5.

I too like the thought of working incrementally. The original idea was to complete the necessary changes and validate SB was capable of the new OfficialBuildId mechanism in P3. This was held up by requiring some of the workflow changes. In P4 the goal is to move all legs while still supporting the ability to rollback to the gitinfo files if MSFT ends up not shipping P4 from the new UB infra. Moving all legs in P4 isn't strictly required but would be ideal IMO.

…ial-build-prerelease-label.patch Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

ViktorHofer

Build parts LGTM. I didn't review the YML

MichaelSimons · 2025-04-15T20:00:08Z

eng/pipelines/templates/jobs/source-only-build-and-validate.yml

+    - AzureLinux_x64_Cross_x64
+    - Browser_Shortstack_wasm
+    - Windows_x64
+    - ${{ format('{0}_SourceBuild_Online_MsftSdk_x64', parameters.buildNamePrefix) }}


Should the x64 references in the validation parameter be updated to use ${{ parameters.targetArchitecture }}? This one particular reference jumps out as the preceding job's name is built with the targetArchitecture. I am alright with fixing this when other legs are onboarded so that they can be validated.

mmitche

Aside from removing the semantic versioning patch which has already been discussed, I have only one minor comment. It's not immediately clear to me what DotNetSourceOnlyTestOnly means. Does it mean only testing during a source-only build, with no build? Or with a build?

mthalman · 2025-04-16T17:36:13Z

It's not immediately clear to me what DotNetSourceOnlyTestOnly means. Does it mean only testing during a source-only build, with no build? Or with a build?

That gets set to true when running the source build validation job, which is a separate job that depends on outputs from the SB job and several UB jobs in order to run all of the tests. TestOnly means it's only going to be running tests. SourceOnly means that it's in source-build mode which enables certain test functionality.

ViktorHofer · 2025-04-17T21:04:41Z

You probably mentioned that in today's sync but will this change cause dev builds to fail? What about ci builds which also appear before previewX builds according to semver rules?

Asking as I currently depend on working dev builds for my outer/inner build work. Not blocking as my work can wait.

mthalman · 2025-04-17T21:37:45Z

You probably mentioned that in today's sync but will this change cause dev builds to fail? What about ci builds which also appear before previewX builds according to semver rules?

Asking as I currently depend on working dev builds for my outer/inner build work. Not blocking as my work can wait.

Dev builds will work by default. Dev build mode will only be enabled in source build if you also set EnableDevBuildAsDefaultForSourceOnly.

Support OfficialBuildId for SB

a22e3ab

Copilot AI review requested due to automatic review settings April 9, 2025 22:50

mthalman requested review from a team as code owners April 9, 2025 22:50

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Apr 9, 2025

Copilot AI reviewed Apr 9, 2025

View reviewed changes

build-analysis bot mentioned this pull request Apr 10, 2025

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

ViktorHofer reviewed Apr 10, 2025

View reviewed changes

eng/pipelines/templates/jobs/vmr-build.yml Show resolved Hide resolved

ViktorHofer reviewed Apr 10, 2025

View reviewed changes

src/Layout/redist/targets/RestoreLayout.targets Show resolved Hide resolved

ViktorHofer reviewed Apr 10, 2025

View reviewed changes

mthalman added 4 commits April 10, 2025 10:32

Revert line removal

269eba9

Fix comments

2e4029b

Use common property for determining prebuilt feed inclusion

2eb97de

Use condition for arcade bootstrap

483c975

MichaelSimons reviewed Apr 10, 2025

View reviewed changes

mthalman added 3 commits April 10, 2025 15:43

Use RestoreAdditionalProjectSources for SB tests

5a86952

Update condition in arcade patch

392a5e6

Remove test ref on UB.Tasks

e87b02f

mthalman and others added 3 commits April 14, 2025 14:00

Update src/SourceBuild/patches/arcade/0001-Use-test-prefix-on-unoffic…

848abe1

…ial-build-prerelease-label.patch Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

Define empty-ish packages.props file for test only model

51fe342

Downgrade importance of msg

850f56c

mthalman requested review from ViktorHofer, NikolaMilosavljevic and MichaelSimons April 14, 2025 19:02

mthalman added 2 commits April 14, 2025 14:04

Merge branch 'main' into sb4855-officialbuildid

01552a2

Encode semicolons for msbuild prop

7d1383e

ViktorHofer reviewed Apr 14, 2025

View reviewed changes

Merge branch 'main' into sb4855-officialbuildid

7f6e3e2

MichaelSimons approved these changes Apr 15, 2025

View reviewed changes

mmitche approved these changes Apr 16, 2025

View reviewed changes

mthalman mentioned this pull request Apr 17, 2025

Remove direct dependency on System.Composition dotnet/roslyn-analyzers#7630

Merged

Remove arcade patch for prerelease labels

5c06683

mthalman mentioned this pull request Apr 17, 2025

Remove xUnit from source-build-externals dotnet/source-build#5059

Open

Merge branch 'main' into sb4855-officialbuildid

ed42450

mthalman enabled auto-merge (squash) April 17, 2025 20:43

build-analysis bot mentioned this pull request Apr 17, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

mthalman disabled auto-merge April 18, 2025 00:15

mthalman enabled auto-merge (squash) April 18, 2025 00:19

mthalman disabled auto-merge April 18, 2025 03:02

mthalman merged commit 4dc9016 into dotnet:main Apr 18, 2025
36 of 39 checks passed

mthalman deleted the sb4855-officialbuildid branch April 18, 2025 03:03

This was referenced Apr 18, 2025

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Unable to checkout dotnet/dnceng#4115

Open

This was referenced Apr 18, 2025

Source build needs to support dev/ci builds dotnet/source-build#5061

Open

Define unit test to ensure that telemetry is not enabled in source-built SDK dotnet/source-build#5067

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support OfficialBuildId for SB #48327

Support OfficialBuildId for SB #48327

mthalman commented Apr 9, 2025

Copilot AI left a comment

ViktorHofer commented Apr 10, 2025

ViktorHofer Apr 10, 2025 •

edited

Loading

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025 •

edited

Loading

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

NikolaMilosavljevic Apr 10, 2025

mthalman Apr 11, 2025

ViktorHofer Apr 10, 2025

mthalman Apr 11, 2025

mthalman commented Apr 10, 2025

ViktorHofer commented Apr 10, 2025

MichaelSimons Apr 10, 2025

mthalman Apr 11, 2025

MichaelSimons commented Apr 10, 2025

ViktorHofer left a comment

MichaelSimons Apr 15, 2025

mmitche left a comment

mthalman commented Apr 16, 2025

ViktorHofer commented Apr 17, 2025 •

edited

Loading

mthalman commented Apr 17, 2025

		<!-- TODO: Remove this import and the EnableDevBuildAsDefaultForSourceOnly when dev builds are the default for source only.
		https://github.com/dotnet/source-build/issues/4922 -->

	+ <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease' and '$(SemanticVersioningV1)' != 'true'">$(_PreReleaseLabel)$(_SemVerLabelSeparator)final</VersionSuffix>
	+ <VersionSuffix Condition="'$(DotNetFinalVersionKind)' == 'prerelease'">$(_PreReleaseLabel)$(_SemVerLabelSeparator)final</VersionSuffix>

Support OfficialBuildId for SB #48327

Support OfficialBuildId for SB #48327

Conversation

mthalman commented Apr 9, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

ViktorHofer commented Apr 10, 2025

ViktorHofer Apr 10, 2025 • 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

ViktorHofer Apr 10, 2025 • 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

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

Choose a reason for hiding this comment

mthalman commented Apr 10, 2025

ViktorHofer commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons commented Apr 10, 2025

ViktorHofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmitche left a comment

Choose a reason for hiding this comment

mthalman commented Apr 16, 2025

ViktorHofer commented Apr 17, 2025 • edited Loading

mthalman commented Apr 17, 2025

ViktorHofer Apr 10, 2025 •

edited

Loading

ViktorHofer Apr 10, 2025 •

edited

Loading

ViktorHofer commented Apr 17, 2025 •

edited

Loading