Skip to content
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

Microsoft.NETCore.App.Crossgen2.sfxproj in source-build #74721

Closed
tmds opened this issue Aug 28, 2022 · 30 comments
Closed

Microsoft.NETCore.App.Crossgen2.sfxproj in source-build #74721

tmds opened this issue Aug 28, 2022 · 30 comments
Labels
area-Infrastructure-coreclr source-build Issues relating to dotnet/source-build
Milestone

Comments

@tmds
Copy link
Member

tmds commented Aug 28, 2022

This project publishes crossgen as a self-contained app.

For source-build this needs to use the runtime that got built from the runtime repo.

The builds also needs to be aware of the rid that source-build added via AdditionalRuntimeIdentifiers by using #69455.

cc @ericstj @omajid @MichaelSimons @jkotas @ViktorHofer

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 28, 2022
@ghost
Copy link

ghost commented Aug 28, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

This project publishes crossgen as a self-contained app.

For source-build this needs to use the runtime that got built from the runtime repo.

The builds also needs to be aware of the rid that source-build added via AdditionalRuntimeIdentifiers by using #69455.

cc @ericstj @omajid @MichaelSimons @jkotas @ViktorHofer

Author: tmds
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@jkotas
Copy link
Member

jkotas commented Aug 28, 2022

For source-build this needs to use the runtime that got built from the runtime repo.

It may be easiest to publish it as framework dependent app for source-build.

@smasher164 smasher164 added this to the 7.0.0 milestone Aug 31, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 31, 2022
@ericstj ericstj added the source-build Issues relating to dotnet/source-build label Sep 12, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

This project publishes crossgen as a self-contained app.

For source-build this needs to use the runtime that got built from the runtime repo.

The builds also needs to be aware of the rid that source-build added via AdditionalRuntimeIdentifiers by using #69455.

cc @ericstj @omajid @MichaelSimons @jkotas @ViktorHofer

Author: tmds
Assignees: -
Labels:

area-Infrastructure-coreclr, source-build

Milestone: 7.0.0

@ViktorHofer
Copy link
Member

@hoyosjs, @trylek, @jkoritzinsky (@agocke) just changed labels as this isn't related to the libraries infrastructure. Can you please take a look?

@tmds
Copy link
Member Author

tmds commented Sep 14, 2022

For source-build this needs to use the runtime that got built from the runtime repo.

When we're source-building .NET, runtime gets built twice: once as runtime-portable, and then as (non-portable) runtime.

When the non-portable runtime build happens, it is using packages that got source-built by runtime-portable. So all output artifacts from runtime were built from source, and we meet the built-from-source requirement.

The runtime-portable is taking up a large chunk of the build time, and it would be a huge improvement if we can eliminate it.

To meet the built-from-source requirement, the Microsoft.NETCore.App.Crossgen2 package may only contain things that were built from source. We can't use a pre-built Microsoft.NETCore.App.Runtime.<rid> package and should change to use artifacts that are built as part of runtime.
Then runtime can produce built-from-source artifacts in a single pass.

We can then eliminate the runtime-portable build by making builds use the non-portable packages built by runtime (like #75597 and dotnet/aspnetcore#43937)

@hoyosjs
Copy link
Member

hoyosjs commented Sep 20, 2022

Is this needed for .NET 7? From what I see this depends on #69455 which is not merged. We are already past RC2 snap.

Once that's in, I wonder if we need a task to update KnownFrameworkReference et al to account for AdditionalRuntimeIdentifiers in RuntimePackRuntimeIdentifiers and have the runtime packs be produced in a place visible to it.

@tmds
Copy link
Member Author

tmds commented Sep 21, 2022

Is this needed for .NET 7? From what I see this depends on #69455 which is not merged. We are already past RC2 snap.

I think we should set our goal to .NET 7 if that is feasible for everyone involved.

source-build's timing doesn't make it feasible to change something before rc.

From what I see this depends on #69455 which is not merged.

In this case, the rid we're publishing would be an exact match with a known rid.
I don't know if it needs to be in the graph for this match to work ... may be.

Once that's in, I wonder if we need a task to update KnownFrameworkReference et al to account for AdditionalRuntimeIdentifiers in RuntimePackRuntimeIdentifiers and have the runtime packs be produced in a place visible to it.

I don't know about the Task. In #75597 I'm making some changes so source-build can pick up non-portable rids from a previous build.

It would be good if we could eliminate the known references to things we shouldn't be using when DotNetBuildFromSource == true. That would be a good guard for the built-from-source requirement.

cc @ericstj

@ericstj
Copy link
Member

ericstj commented Sep 21, 2022

We are already past RC2 snap.

Infrastructure changes are OK to go into 7.0 so long as they stay constrained to infrastructure and feed into the source-build requirements. We can take them to Tactics if we think there is some product risk - typically we don't and instead work to mitigate the risk through some PR validation (manual checks, test cases, etc). I closed out that PR since I'm not actively working on it and my time for making fixes (especially those that others are dependent on) is pretty small at the moment. I think it makes more sense for that to be driven by folks who need it and can test the end-to-end with the change.

@hoyosjs
Copy link
Member

hoyosjs commented Sep 21, 2022

I don't know about the Task.

Sorry, meant target.

@tmds
Copy link
Member Author

tmds commented Sep 22, 2022

@ViktorHofer can we use eng/targetingpacks.targets to use what got built while compiling crossgen2.csproj from Microsoft.NETCore.App.Crossgen2.sfxproj?

@ViktorHofer
Copy link
Member

ViktorHofer commented Sep 22, 2022

Yes, that would be the right extension point to use. Out-of-band libraries under src/libraries and other projects like the mono samples use that as well.

@tmds
Copy link
Member Author

tmds commented Sep 26, 2022

@ViktorHofer, can you (or someone else that is familiar) look into this?

@ViktorHofer
Copy link
Member

Do you mean reviewing the PR or enabling the targetingpack.targets infrastructure for crossgen sfxproj?

@tmds
Copy link
Member Author

tmds commented Sep 27, 2022

Enabling targetpack.targets infra for sfxproj.

I took a shot but didn't get far.

I first built runtime repo ./build.sh /p:DotnetBuildFromSource=true.

Then I tried building just the sfxproj by running dotnet pack Microsoft.NETCore.App.Crossgen2.sfxproj but I get a ton of build errors that look like: Predefined type Xyz is not defined or imported. This is before I've made any changes. So I don't know how to build just this project without building the entire repo.

I guess the fix involves importing targetingpacks.targets at the right spot. I don't know where that is.

Also, maybe we can remove some of the known packs and references, before adding the built ones, to be sure the prebuilts don't get used.

    <KnownFrameworkReference Remove="Microsoft.NETCore.App" />
    <KnownCrossgen2Pack Remove="Microsoft.NETCore.App.Crossgen2" />
    <KnownFrameworkReference Remove="Microsoft.AspNetCore.App" />
    <KnownAppHostPack Remove="Microsoft.NETCore.App" />

@ViktorHofer
Copy link
Member

Crossgen2 is owned by the CLR team and @MichalStrehovsky afaik. It would be good to first agree on the direction. Do we want to publish framework dependent or self-contained in source-build? What are the components in play? Is it crossgen2.csproj and Microsoft.NETCore.App.Crossgen2.sfxproj? I hope that Michal or others can help further with this as I'm really short on additional resources right now.

@MichalStrehovsky
Copy link
Member

Crossgen2 is owned by the CLR team and @MichalStrehovsky afaik

It's owned by @dotnet/crossgen-contrib. I look at stuff but don't make decisions and I'm not on the team that owns it.

Do we need to build the sfxproj in the first place? Crossgen2 is on the same plan as NativeAOT - if you do PublishReadyToRun=true, we grab crossgen2 from a NuGet package (dotnet/source-build#2885 (comment)). It doesn't come from the SDK. Do we need to build the NuGet package if it's not going to be published anyway (because the Microsoft-built one will be used)?

@tmds
Copy link
Member Author

tmds commented Sep 27, 2022

The crossgen2 package gets used by the ASP.NET Core build.

The goal is to ensure all artifacts produced by a single build of runtime (which are then used by other repos, like ASP.NET Core, and also as prebuilts for successive builds of .NET) are built from source.

For .NET 7, these packages are only consumed while building .NET itself and they are not made available for end-users. So it is possible to publish them different if that makes it easier to meet the built-from-source requirement.

(For .NET 8+, there is an open issue on source-build repo for source-built SDKs to also be capable of delivering these features.)

It would be nice if we can do this for .NET 7. It allows to omit building runtime twice in source-build which reduces total build-time on x64 by 25%, and by 45% on arm64 (from dotnet/installer#14549 (comment)).

My feeling is that using targetingpacks.targets we are close to doing this, but I don't know how to wire it up.

@tmds
Copy link
Member Author

tmds commented Sep 29, 2022

@ViktorHofer @MichalStrehovsky can someone take a look at this?

@tmds
Copy link
Member Author

tmds commented Sep 29, 2022

fyi, this is what my latest attempt looks like:

diff --git a/eng/targetingpacks.targets b/eng/targetingpacks.targets
index 744a0cd20b4..b4d35aab5f7 100644
--- a/eng/targetingpacks.targets
+++ b/eng/targetingpacks.targets
@@ -26,6 +26,7 @@
 
   <!-- Add Known* items if the SDK doesn't support the TargetFramework yet. -->
   <ItemGroup Condition="'$(UseLocalTargetingRuntimePack)' == 'true'">
+    <KnownFrameworkReference Remove="$(LocalFrameworkOverrideName)" />
     <KnownFrameworkReference Include="$(LocalFrameworkOverrideName)"
                              DefaultRuntimeFrameworkVersion="$(ProductVersion)"
                              LatestRuntimeFrameworkVersion="$(ProductVersion)"
@@ -36,6 +37,7 @@
                              TargetingPackName="$(LocalFrameworkOverrideName).Ref"
                              TargetingPackVersion="$(ProductVersion)"
                              Condition="'@(KnownFrameworkReference)' == '' or !@(KnownFrameworkReference->AnyHaveMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))" />
+    <KnownRuntimePack Remove="$(LocalFrameworkOverrideName)" />
     <KnownRuntimePack Include="$(LocalFrameworkOverrideName)"
                       TargetFramework="$(NetCoreAppCurrent)"
                       RuntimeFrameworkName="$(LocalFrameworkOverrideName)"
@@ -44,12 +46,14 @@
                       RuntimePackRuntimeIdentifiers="linux-arm;linux-armv6;linux-arm64;linux-musl-arm64;linux-bionic-arm64;linux-loongarch64;linux-musl-x64;linux-bionic-x64;linux-x64;osx-x64;rhel.6-x64;win-arm;win-arm64;win-x64;win-x86;linux-musl-arm;osx-arm64;maccatalyst-x64;maccatalyst-arm64;browser-wasm;ios-arm64;ios-arm;iossimulator-arm64;iossimulator-x64;iossimulator-x86;tvos-arm64;tvossimulator-arm64;tvossimulator-x64;android-arm64;android-arm;android-x64;android-x86"
                       RuntimePackLabels="Mono"
                       Condition="'@(KnownRuntimePack)' == '' or !@(KnownRuntimePack->AnyHaveMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))"/>
+    <KnownAppHostPack Remove="$(LocalFrameworkOverrideName)" />
     <KnownAppHostPack Include="$(LocalFrameworkOverrideName)"
                       AppHostPackNamePattern="$(LocalFrameworkOverrideName).Host.**RID**"
                       AppHostPackVersion="$([MSBuild]::ValueOrDefault('$(_AppHostBaselinePackVersion)', '$(ProductVersion)'))"
                       AppHostRuntimeIdentifiers="linux-arm;linux-armv6;linux-arm64;linux-musl-arm64;linux-bionic-arm64;linux-loongarch64;linux-musl-x64;linux-bionic-x64;linux-x64;osx-x64;rhel.6-x64;tizen.4.0.0-armel;tizen.5.0.0-armel;win-arm;win-arm64;win-x64;win-x86;linux-musl-arm;osx-arm64"
                       TargetFramework="$(NetCoreAppCurrent)"
                       Condition="'@(KnownAppHostPack)' == '' or !@(KnownAppHostPack->AnyHaveMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))" />
+    <KnownCrossgen2Pack Remove="$(LocalFrameworkOverrideName).Crossgen2" />
     <KnownCrossgen2Pack Include="$(LocalFrameworkOverrideName).Crossgen2"
                         TargetFramework="$(NetCoreAppCurrent)"
                         Crossgen2PackNamePattern="$(LocalFrameworkOverrideName).Crossgen2.**RID**"
@@ -87,6 +91,14 @@
                        Condition="'$(UsePackageDownload)' == 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Runtime'))" />
       <PackageReference Remove="@(PackageReference)"
                         Condition="'$(UsePackageDownload)' != 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Runtime'))" />
+      <PackageDownload Remove="@(PackageDownload)"
+                       Condition="'$(UsePackageDownload)' == 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Crossgen2'))" />
+      <PackageReference Remove="@(PackageReference)"
+                        Condition="'$(UsePackageDownload)' != 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Crossgen2'))" />
+      <PackageDownload Remove="@(PackageDownload)"
+                       Condition="'$(UsePackageDownload)' == 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Host'))" />
+      <PackageReference Remove="@(PackageReference)"
+                        Condition="'$(UsePackageDownload)' != 'true' and $([System.String]::Copy('%(Identity)').StartsWith('$(LocalFrameworkOverrideName).Host'))" />
     </ItemGroup>
   </Target>
 
diff --git a/src/coreclr/tools/aot/crossgen2/crossgen2.csproj b/src/coreclr/tools/aot/crossgen2/crossgen2.csproj
index 12864a2341e..bd45dd2dd41 100644
--- a/src/coreclr/tools/aot/crossgen2/crossgen2.csproj
+++ b/src/coreclr/tools/aot/crossgen2/crossgen2.csproj
@@ -13,6 +13,7 @@
     <RuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">$(PackageRID)</RuntimeIdentifiers>
     <SelfContained>false</SelfContained>
     <SelfContained Condition="'$(RunningPublish)' == 'true'">true</SelfContained>
+    <UseLocalTargetingRuntimePack>true</UseLocalTargetingRuntimePack>
   </PropertyGroup>
 
   <Import Project="crossgen2.props" />
@@ -28,6 +29,7 @@
   </PropertyGroup>
 
   <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />
+  <Import Project="$(RepositoryEngineeringDir)targetingpacks.targets" />
 
   <PropertyGroup Condition="'$(NativeAotSupported)' == 'true'">
     <IlcToolsPath>$(CoreCLRILCompilerDir)</IlcToolsPath>

And dotnet pack Microsoft.NETCore.App.Crossgen2.sfxproj gives me:

/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018: The "ResolveRuntimePackAssets" task failed unexpectedly. [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]
/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018: System.NullReferenceException: Object reference not set to an instance of an object. [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]
/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018:    at Microsoft.NET.Build.Tasks.ResolveRuntimePackAssets.ExecuteCore() [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]
/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018:    at Microsoft.NET.Build.Tasks.TaskBase.Execute() [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]
/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]
/home/tmds/repos/runtime/.dotnet/sdk/7.0.100-preview.7.22377.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(440,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [/home/tmds/repos/runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]

@MichalStrehovsky
Copy link
Member

@ViktorHofer @MichalStrehovsky can someone take a look at this?

@mangod9 I Cc'd @dotnet/crossgen-contrib but there's no volunteer, could you nominate one? I assume this is blocking source build in .NET 7, but I don't know anything about source build or "packs".

@mangod9
Copy link
Member

mangod9 commented Sep 30, 2022

Is this required for 7? @trylek @AntonLapounov should be able to help here.

@tmds
Copy link
Member Author

tmds commented Sep 30, 2022

Strictly speaking it is not required as we are capable of building .NET 7.

We should still aim to fix this for .NET 7 as it enables reducing source-build's build time by 25% and more, see dotnet/installer#14549 (comment).

@tmds
Copy link
Member Author

tmds commented Oct 4, 2022

@trylek @AntonLapounov should be able to help here.

@trylek @AntonLapounov can one of you ptal?

@AntonLapounov
Copy link
Member

Are you seeing that failure in the ResolveRuntimePackAssets task when doing the following sequence of commands with the diff given above? I have no prior knowledge of that part of SDK; however, it should not be difficult to debug this.

./build.sh /p:DotnetBuildFromSource=true
dotnet pack Microsoft.NETCore.App.Crossgen2.sfxproj

@tmds
Copy link
Member Author

tmds commented Oct 10, 2022

I'll try to capture what happens when source-build runs this.

@ViktorHofer @AntonLapounov it doesn't look like the top-level verbosity ripples down to the MSBuild tasks. How can I get more verbose logging of what happens in these MSBuild tasks:

<MSBuild Projects="$(RepoRoot)src/coreclr/tools/aot/crossgen2/crossgen2.csproj"
Targets="Restore"
Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())
;RunningPublish=true
;RuntimeIdentifier=$(PackageRID)
;CoreCLRArtifactsPath=$(CoreCLRArtifactsPath)
;R2ROverridePath=$(MSBuildThisFileDirectory)ReadyToRun.targets" />
<MSBuild Projects="$(RepoRoot)src/coreclr/tools/aot/crossgen2/crossgen2.csproj"
Targets="Publish;PublishItemsOutputGroup"
Properties="RunningPublish=true
;RuntimeIdentifier=$(PackageRID)
;CoreCLRArtifactsPath=$(CoreCLRArtifactsPath)
;R2ROverridePath=$(MSBuildThisFileDirectory)ReadyToRun.targets">
<Output TaskParameter="TargetOutputs"
ItemName="_RawCrossgenPublishFiles" />
</MSBuild>

@tmds
Copy link
Member Author

tmds commented Oct 10, 2022

@AntonLapounov if you want to take a stab at this.

You can perform a build like this:

./build.sh /p:DotNetBuildFromSource=true --outputrid banana.10-x64 /p:RuntimeOS=linux /p:AdditionalRuntimeIdentifierParent=linux

The Crossgen2 package should be built using Microsoft.NETCore.App.Runtime.banana.10-x64 package, instead of the pre-built Microsoft.NETCore.App.Runtime.linux-x64 package.

@tmds
Copy link
Member Author

tmds commented Oct 10, 2022

Maybe this is already consuming the runtime pack that got built?

What does this do:

<ResolvedRuntimePack Update="Microsoft.NETCore.App.Runtime.$(RuntimeIdentifier)">
<PackageDirectory>$(MicrosoftNetCoreAppRuntimePackDir)</PackageDirectory>
</ResolvedRuntimePack>

And, what is the contents of MicrosoftNetCoreAppRuntimePackDir? Is this a package that got extracted, or was this built from source?

cc @agocke @am11

@agocke
Copy link
Member

agocke commented Oct 10, 2022

Yes, that pulls the runtime pack that just got built

@am11
Copy link
Member

am11 commented Oct 10, 2022

Btw, <NativeAotSupported property is set in two places with different conditions:

<NativeAotSupported Condition="('$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'OSX') and ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64')">true</NativeAotSupported>

<NativeAotSupported Condition="'$(DotNetBuildFromSource)' == 'true'">false</NativeAotSupported>

maybe it's better to merge the condition in crossgen2 into the top-level Subsets.props to avoid any unexpected build state. It is basically controlling "after compiling crossgen2 assembly, whether to publish crossgen2 as nativeaot app (NativeAotSupported=true) or a singlefileapp (NativeAotSupported=false)".

@tmds
Copy link
Member Author

tmds commented Oct 11, 2022

Yes, that pulls the runtime pack that just got built

Great! I can just close this then.

@tmds tmds closed this as completed Oct 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr source-build Issues relating to dotnet/source-build
Projects
None yet
Development

No branches or pull requests