Use arcade power source build infra #58651

Anipik · 2021-09-03T19:41:33Z

Related to #54573

dotnet-issue-labeler · 2021-09-03T19:41:36Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Anipik · 2021-09-08T21:07:00Z

cc @ViktorHofer & @wfurt to look at the change.

ViktorHofer · 2021-09-09T17:53:00Z

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

@@ -128,7 +128,7 @@
  </ItemGroup>

  <!-- Support for deploying msquic -->
-  <ItemGroup Condition="'$(TargetsWindows)' == 'true' and
+  <ItemGroup Condition="'$(_hostOS)' == 'windows' and


That's an internal property. Don't we have a better one to use? cc @am11

Would also be good to add a comment explaining why TargetsWindows can't be used here. I don't know that either yet.

Note that Targets{OSName} properties in top level Directory.Build.props do not get set for src/libraries, we set them in eng/targetframeworksuffix.props. It would be nice to get rid of whole SkipInferTargetOSName mechanism and just use the top-level Directory.Build.props, unless it is impossible to consolidate (which I don't think is the case).

i can add the comment. but the explaination is
The non portable build triggers all config build which build this project for net6.0-windows on linux, hence targetsWIndows is true but the runtime is linux so all the BinPlaceDir are linux directories and thee file gets binplaced with the linux runtime.
i think we want to binplace it in the runtime when we arre building on windows, i added @wfurt to verify that

comment would be always good. We should package this only for Windows runtime - regardless where this is build. if _hostOS is the right variables I'm fine with that.

That's an internal property.

Correct, properties with _ indicate "do no use outside of this file" and can break in the future. We have managed to avoid using these internal properties elsewhere. Since this is the first use-case of HostOS outside of Directory.Build.props, we can (unconditionally) define <HostOS>$(_hostOS)</HostOS> between these two lines:

runtime/Directory.Build.props

Lines 20 to 21 in 1cd5033

<_hostOS Condition="'$(TargetOS)' == 'Browser'">Browser</_hostOS>

<TargetOS Condition="'$(TargetOS)' == ''">$(_hostOS)</TargetOS>

and then in the following lines of Directory.Build.props and here, use $(HostOS).

Why can't we use TargetOS here instead?

I think it's because of the non-portable build which @Anipik alluded above: #58651 (comment)
But is this the only place in the entire build infra where it is applicable? If not, what is the alternative way of solving it, maybe we should use that approach to keep it consistent?

But the sourcebuild leg sets the OS to Linux in the yml, why would the TargetOS be "Windows" then?

runtime/eng/pipelines/common/platform-matrix.yml

Line 191 in 3e6d492

osGroup: Linux

TargetsWindows = true is true in the innerbuilds when we dispatch innerbuild from net6.0-windows.

ViktorHofer · 2021-09-09T17:55:07Z

eng/pipelines/common/global-build-job.yml

+      - template: /eng/common/templates/steps/source-build.yml
+        parameters:
+          platform:
+            buildScript: $(_sclEnableCommand) $(Build.SourcesDirectory)$(dir)build$(scriptExt)


Is that the right command to invoke? cc @MichaelSimons

I think so. This is just the build script, the template defaults to specify all of the correct args.

And is the _sclEnableCommand still needed?

runtime/eng/pipelines/common/global-build-job.yml

Line 100 in d841f5e

value: scl enable llvm-toolset-7.0 --

yes otherwise we get clang failures.

MichaelSimons

With these changes, will the Microsoft.SourceBuild.Intermediates.* packages get published during internal CI runs?

Anipik · 2021-09-09T18:10:25Z

With these changes, will the Microsoft.SourceBuild.Intermediates.* packages get published during internal CI runs?

This was the first step, i was thinking of adding it a separate change but i think i can push the change here only

Anipik · 2021-09-09T18:49:09Z

https://dev.azure.com/dnceng/internal/_build/results?buildId=1352976&view=logs&j=5cab1c58-8e18-5cb5-2651-4639bac8ae0f

corressponding internal build

MichaelSimons

I don't have an in-depth knowledge of the runtime pipelines but the source-build specific parts LGTM.

Directory.Build.props

src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props

ericstj · 2021-09-10T20:22:17Z

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

@@ -128,7 +128,8 @@
  </ItemGroup>

  <!-- Support for deploying msquic -->
-  <ItemGroup Condition="'$(TargetsWindows)' == 'true' and
+  <!-- Native msquic assemblies should only be binplaced while running on windows. -->
+  <ItemGroup Condition="'$(HostOS)' == 'windows' and


This is a bit of conflation. Nothing about building on a Windows machine means that these files are present. Targeting is the right concept. Can we instead just exclude this from source build? Since presumably the reason this isn't available is because we aren't building the dependent package as part of source build.

I'm actually surprised that the PackageReference worked but this didn't. What failed that led you to add this?

I'm actually surprised that the PackageReference worked but this didn't. What failed that led you to add this?

sourcebuild leg failed.

Of course something failed, how did it fail and why?

Can we instead just exclude this from source build?

SourceBuild leg if run on windows will not contain this file in the targeting pack.

Targeting is the right concept.

why is the targeting the right concept here, we are binplacing the file in the runtime folders, shouldnt we check the runtime we are running on before binplacing ?

Of course something failed, how did it fail and why?

the missing fileVersion error.

shouldnt we check the runtime we are running on before binplacing ?

The runtime we're running on has nothing to do with the product we are building, though it is sometimes related. We could technically build all of libraries for Linux from a windows machine (and we used to do so at the start of .NETCore).

BinPlacing is supposed to involve a calculation on "best applicable configuration". Typically we only binplace when the configuration being built is the best for a given target. https://github.com/dotnet/arcade/blob/22d90b34fe210ca9f85ea1896eda29f2de55f826/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/BinPlace.targets#L122

It looks like this project is bypassing that by directly defining BinPlaceDir items. In that case, this condition needs to do the work that would have been done by ChooseBestTargetFrameworksTask: the concept shouldn't be "host OS" but should be "target runtime for the vertical". It looks like this should be TargetOS.

I'm still dubious about MSQuic packagereference though. @MichaelSimons does this package even get built in source build repos? Shouldn't we exclude the PackageReference?

I'm still dubious about MSQuic packagereference though. @MichaelSimons does this package even get built in source build repos? Shouldn't we exclude the PackageReference?

It does not get produced by source-build today. Additionally system.net.msquic.transport is showing up as a prebuilt. Excluding the PackageReference seems like the good approach for source-build.

The runtime we're running on has nothing to do with the product we are building, though it is sometimes related. We could technically build all of libraries for Linux from a windows machine

i thought this applied to managed assemblies, i thought we cant build the native assemblies cross platform ?

ericstj · 2021-09-11T02:17:39Z

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

@@ -101,7 +101,8 @@
    <PackageReference Include="System.Net.MsQuic.Transport"
                      Version="$(SystemNetMsQuicTransportVersion)"
                      PrivateAssets="all"
-                      GeneratePathProperty="true" />
+                      GeneratePathProperty="true" 
+                      Condition="'$(DotNetBuildFromSource)' != 'true'" />


Should this same condition be applied to the target which consumes the file from this package?

its used for interop calls mostly, no specific target uses this file

ericstj

LGTM modulo the one comment about the target condition

ericstj · 2021-09-13T16:50:36Z

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

@@ -129,7 +130,7 @@

  <!-- Support for deploying msquic -->
  <ItemGroup Condition="'$(TargetsWindows)' == 'true' and
-                        ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'x86')">
+                        ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'x86') and '$(DotNetBuildFromSource)' != 'true'">
    <BinPlaceDir Include="$(MicrosoftNetCoreAppRuntimePackNativeDir)" ItemName="NativeBinPlaceItem" />


Not for this PR, but something to consider moving forward is that this fails silently if that quic package changes it's shape. It might be good to avoid a wildcard here. We should be explicit about things which we end up shipping in the shared framework.

Anipik · 2021-09-14T20:25:12Z

Failure is a known xunit bug,

Anipik · 2021-09-14T20:25:26Z

/backport to release/6.0

github-actions · 2021-09-14T20:25:42Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1235139018

Anipik changed the title ~~[WIP] Use arcade power source build infra~~ Use arcade power source build infra Sep 3, 2021

Anipik requested review from ViktorHofer, MichaelSimons and ericstj September 3, 2021 21:01

marek-safar added the area-Infrastructure label Sep 6, 2021

ViktorHofer reviewed Sep 9, 2021

View reviewed changes

MichaelSimons reviewed Sep 9, 2021

View reviewed changes

MichaelSimons approved these changes Sep 9, 2021

View reviewed changes

am11 reviewed Sep 10, 2021

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

ericstj reviewed Sep 10, 2021

View reviewed changes

src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props Outdated Show resolved Hide resolved

ericstj reviewed Sep 10, 2021

View reviewed changes

Anirudh Agnihotry added 2 commits September 10, 2021 14:40

add yml files

329bbf5

add file version and condition package reference on source build

6143407

ericstj reviewed Sep 11, 2021

View reviewed changes

ericstj approved these changes Sep 11, 2021

View reviewed changes

runfoapp bot mentioned this pull request Sep 13, 2021

TLS1.3 networking tests are flaky on Windows 11 #58927

Closed

to avoid copying the file from the cache

d948056

ericstj reviewed Sep 13, 2021

View reviewed changes

ericstj approved these changes Sep 13, 2021

View reviewed changes

Anipik merged commit 0585160 into dotnet:main Sep 14, 2021

github-actions bot mentioned this pull request Sep 14, 2021

[Tell-mode][release/6.0] Use arcade power source build infra #59108

Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Anipik deleted the sb branch November 10, 2021 18:57

	<_hostOS Condition="'$(TargetOS)' == 'Browser'">Browser</_hostOS>
	<TargetOS Condition="'$(TargetOS)' == ''">$(_hostOS)</TargetOS>

Use arcade power source build infra #58651

Use arcade power source build infra #58651

Conversation

Anipik commented Sep 3, 2021 • edited Loading

dotnet-issue-labeler bot commented Sep 3, 2021

Anipik commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anipik Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Sep 10, 2021 • 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

MichaelSimons left a comment

Choose a reason for hiding this comment

Anipik commented Sep 9, 2021 • edited Loading

Anipik commented Sep 9, 2021

MichaelSimons left a comment

Choose a reason for hiding this comment

ericstj Sep 10, 2021 • 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

Anipik Sep 10, 2021 • 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

Anipik Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anipik commented Sep 14, 2021

Anipik commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

Anipik commented Sep 3, 2021 •

edited

Loading

Anipik Sep 9, 2021 •

edited

Loading

am11 Sep 9, 2021 •

edited

Loading

ViktorHofer Sep 10, 2021 •

edited

Loading

Anipik commented Sep 9, 2021 •

edited

Loading

ericstj Sep 10, 2021 •

edited

Loading

Anipik Sep 10, 2021 •

edited

Loading

Anipik Sep 12, 2021 •

edited

Loading