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

Eliminate portable runtime build. #14549

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 20, 2022

@tmds tmds requested a review from a team as a code owner September 20, 2022 06:19
@@ -50,7 +50,7 @@
<SourceBuiltTarballName>$(OutputPath)$(SourceBuiltArtifactsTarballName).$(installerOutputPackageVersion).tar.gz</SourceBuiltTarballName>
</PropertyGroup>

<Exec Command="tar --numeric-owner --exclude='Microsoft.SourceBuild.Intermediate.*.nupkg' --exclude='runtime.$(TargetRid).*.nupkg' -czf $(SourceBuiltTarballName) *.nupkg *.props SourceBuildReferencePackages/"
<Exec Command="tar --numeric-owner --exclude='Microsoft.SourceBuild.Intermediate.*.nupkg' -czf $(SourceBuiltTarballName) *.nupkg *.props SourceBuildReferencePackages/"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is to include ilasm for the non-portable rid.

@@ -0,0 +1,337 @@
From 4e92a776a0bb0b8a4ab02ad4e436352594b7eab2 Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<MicrosoftNETCoreAppHostPackageVersion>$(MicrosoftNETCoreAppHostwinx64PackageVersion)</MicrosoftNETCoreAppHostPackageVersion>
<MicrosoftNETCoreAppRuntimePackageVersion>$(MicrosoftNETCoreAppRuntimewinx64PackageVersion)</MicrosoftNETCoreAppRuntimePackageVersion>
- <MicrosoftAspNetCoreAppRuntimePackageVersion>$(MicrosoftAspNetCoreAppRuntimewinx64PackageVersion)</MicrosoftAspNetCoreAppRuntimePackageVersion>
+ <MicrosoftAspNetCoreAppRuntimePackageVersionBuilt>$(MicrosoftAspNetCoreAppRuntimewinx64PackageVersion)</MicrosoftAspNetCoreAppRuntimePackageVersionBuilt>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this to deal with https://github.com/dotnet/installer/issues/14492. This renames the property so source-build doesn't override it. There are better ways to fix this. I noticed the issue after I wrote this patch.

@@ -0,0 +1,67 @@
From 39d69b410eff3e6a22aa3a25489263a35c7da453 Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmds
Copy link
Member Author

tmds commented Sep 20, 2022

I ran this locally using Microsoft's portable sdk.
Then I took the output from the build (non-portable sdk and assets) and made another build.

This built fine with the exception of fsharp which fails on the second build with:

    FSharp.Build -> /home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/artifacts/bin/FSharp.Build/Proto/netstandard2.0/FSharp.Build.dll
    FSharp.Compiler.Service -> /home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/artifacts/bin/FSharp.Compiler.Service/Proto/netstandard2.0/FSharp.Compiler.Service.dll
FSC : error : no entrypoint specified in executable binary [/home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/src/fsc/fscProject/fsc.fsproj::TargetFramework=net7.0] [/home/tmds/tarball61/Tools/source-built/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##vso[task.logissue type=error;sourcepath=FSC;linenumber=0;columnnumber=0;code=;]no entrypoint specified in executable binary [/home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/src/fsc/fscProject/fsc.fsproj::TargetFramework=net7.0%5D
    fsc -> /home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/artifacts/bin/fsc/Proto/net7.0/fsc.dll
FSC : error : no entrypoint specified in executable binary [/home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/src/fsi/fsiProject/fsi.fsproj::TargetFramework=net7.0] [/home/tmds/tarball61/Tools/source-built/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##vso[task.logissue type=error;sourcepath=FSC;linenumber=0;columnnumber=0;code=;]no entrypoint specified in executable binary [/home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/src/fsi/fsiProject/fsi.fsproj::TargetFramework=net7.0%5D
    fsi -> /home/tmds/tarball61/src/fsharp/artifacts/source-build/self/src/artifacts/bin/fsi/Proto/net7.0/fsi.dll

I have no clue what is causing this or to get better debug info.
It could be that two successive builds without these changes also have the issue. I have not tried.
Can someone help me figure out what the issue is?

@tmds
Copy link
Member Author

tmds commented Sep 20, 2022

This built fine with the exception of fsharp which fails on the second build with:

The issue is not caused by these changes. I've created dotnet/source-build#3030.

@MichaelSimons
Copy link
Member

@tmds - This is a fantastic contribution I've been wanting for some time. Can you clarify your intent on if you are trying to get this into 7.0 or 8.0? I ask because the contributing changes in the repos like runtime and aspnetcore are going into main and this PR is going into 7.0.

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the FSharp issue is resolved (dotnet/source-build#3030), I'd like to trigger the internal CI on this change to validate everything builds cleanly rebuilding the tarball with the SDK and artifacts from these changes.

@tmds
Copy link
Member Author

tmds commented Sep 20, 2022

Can you clarify your intent on if you are trying to get this into 7.0 or 8.0? I ask because the contributing changes in the repos like runtime and aspnetcore are going into main and this PR is going into 7.0.

I'm targeting the main branches to get feedback from the repo owners, and here I'm using 7.0 to validate.

We can decide if we want to do this 7.0 or later depending on the risk. We could also make it opt-in.

It builds, but it isn't finished yet. I think the crossgen2 package that gets built from runtime no longer meets the built-from-source requirement. I've created dotnet/runtime#74721 for that.

Once the FSharp issue is resolved (dotnet/source-build#3030), I'd like to trigger the internal CI on this change to validate everything builds cleanly rebuilding the tarball with the SDK and artifacts from these changes.

Is this a CI job that will build installer twice? One starting with the prebuilts + portable SDK, and then a second build with the output of the first build?

@MichaelSimons
Copy link
Member

We can decide if we want to do this 7.0 or later depending on the risk. We could also make it opt-in.

Sounds good.

Is this a CI job that will build installer twice? One starting with the prebuilts + portable SDK, and then a second build with the output of the first build?

Correct

@@ -0,0 +1,37 @@
From 43f51a115a8f20b0c1374afa180c2a67db2e357e Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dotnet/sdk#28005.

@ayakael
Copy link

ayakael commented Sep 26, 2022

In testing this against 7.0.100-rc1 on Alpine Linux, build of aspnetcore fails with:

Could not locate the package or project for "Microsoft.NETCore.App.Runtime.alpine.3.17-x64"

I reckon this is due to the initial prebuilt bootstrap being portable. Aspnetcore should be able to buld with portable runtime despite targeting non-portable.

edit Ignore this, I made a mistake when porting this pull request to the Alpine building script. It works now.

@ayakael
Copy link

ayakael commented Sep 26, 2022

Incidently this fixes musl support on installer, as the issues had to do entirely with portable builds in source-build. I'm doing more tests, but it looks like it would make #13410 unnecessary.

@ayakael
Copy link

ayakael commented Sep 27, 2022

Finished tests for dotnet7 rc1 on Alpine Linux. Substantial build time savings:

  • x86_64: 89 min to 66 min
  • aarch64: 181 min to 97 min
  • armv7: tbd, but given current build time of 8 hours, going down to 3 would be terrific.

Very happy with this change, and would be happy to see it included in net7.0.

@tmds
Copy link
Member Author

tmds commented Sep 28, 2022

The tarball build fails with:

2022-09-28T19:24:52.6807149Z     Build FAILED.
2022-09-28T19:24:52.6807585Z     
2022-09-28T19:24:52.6811751Z     /tarball/src/runtime/artifacts/source-build/self/package-cache/microsoft.dotnet.apicompat.task/7.0.100-rc.1.22429.2/build/Microsoft.NET.ApiCompat.ValidatePackage.targets(32,5): error CP0002: Member 'Microsoft.Extensions.Caching.Memory.PostEvictionDelegate.PostEvictionDelegate(object, System.IntPtr)' exists on lib/netstandard2.0/Microsoft.Extensions.Caching.Abstractions.dll but not on lib/net7.0/Microsoft.Extensions.Caching.Abstractions.dll [/tarball/src/runtime/artifacts/source-build/self/src/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/Microsoft.Extensions.Caching.Abstractions.csproj]
2022-09-28T19:24:52.6816093Z         0 Warning(s)
2022-09-28T19:24:52.6816602Z         1 Error(s)

This seems related to dotnet/sdk#25566.

@tmds
Copy link
Member Author

tmds commented Sep 29, 2022

The tarball build fails with:

I had accidentally removed disabling package validation which caused the build failure.

@@ -0,0 +1,266 @@
From 65dba1ea70461a99f585daebed67a3ad52b3f48c Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dotnet/runtime#74504 (merged on runtime main).

@@ -0,0 +1,127 @@
From cd2f0ffb287251c26f03850d1a596abafb99d5c7 Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dotnet/runtime#76068 (merged on runtime main).

@@ -0,0 +1,52 @@
From 2775cf0d524f7a31f8cad3405b3e111056b345e3 Mon Sep 17 00:00:00 2001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmds
Copy link
Member Author

tmds commented Sep 29, 2022

This now builds without the portable runtime.

There is still an issue with the crossgen2 package that gets built by runtime.

This contains a self-contained app. Self-contained apps get built using artifacts from microsoft.netcore.app.runtime.<rid> package. So the package that now comes out of runtime has prebuilt binaries in it.

It should be possible to use the microsoft.netcore.app.runtime.<rid> that got built as part of the runtime build, but I haven't been able to make that work.
There is an open issue for this: dotnet/runtime#74721.

@tmds
Copy link
Member Author

tmds commented Sep 30, 2022

Besides the patches needed to eliminate the portable runtime build, I've included patches that enable source-build to build on distros (and distro versions) that are not yet known in the runtime graph (dotnet/runtime#74504 and dotnet/runtime#76068).

I've included them in this PR to see if they work well with the other RID-related changes.

@tmds
Copy link
Member Author

tmds commented Nov 8, 2022

I'll look into making the backport PRs.

We also still need a proper fix for dotnet/source-build#3121.

@ayakael
Copy link

ayakael commented Nov 8, 2022

I'll look into making the backport PRs.

We also still need a proper fix for dotnet/source-build#3121.

Indeed. I've been using your fix, that is to say changing the name of MicrosoftAspNetCoreAppRuntimePackageVersion to MicrosoftAspNetCoreAppRuntimePackageVersionBuilt. In fact, it is what I backported to release/6.0.1xx in #14816. It was my understanding that this is the fix we're putting forward.

ayakael added a commit to ayakael/installer that referenced this pull request Nov 10, 2022
@tmds
Copy link
Member Author

tmds commented Nov 14, 2022

crummel pushed a commit to ayakael/installer that referenced this pull request Nov 15, 2022
ayakael added a commit to ayakael/installer that referenced this pull request Nov 15, 2022
ayakael added a commit to ayakael/installer that referenced this pull request Nov 16, 2022
@tmds
Copy link
Member Author

tmds commented Nov 18, 2022

@tmds
Copy link
Member Author

tmds commented Dec 1, 2022

installer: to be backported: #14938, #14549

@crummel @MichaelSimons are you looking into backporting these to 7.0? Is there something I can do to help?

@MichaelSimons
Copy link
Member

This was picked up in 7.0.1 servicing (internally). It will be released in Dec servicing.

@MichaelSimons
Copy link
Member

The runtime and aspnetcore changes that were backported flowed in and required the installer changes.

@tmds
Copy link
Member Author

tmds commented Dec 14, 2022

@MichaelSimons this change is now part of tag/v7.0.101-source-build, but not part of release/7.0.1xx. Is that expected? Will it become part of the 7.0.1xx release branch?

@MichaelSimons
Copy link
Member

Is that expected?

Yes it is. The internal servicing changes made for 7.0.101 changes aren't getting mirror publicly because of the tight timeline for the January release because of the holiday vacations. The changes will get mirrored during the 7.0.102 release in January. I apologize for any inconveniences this causes.

ayakael added a commit to ayakael/installer that referenced this pull request Dec 16, 2022
ayakael added a commit to ayakael/installer that referenced this pull request Jan 10, 2023
ayakael added a commit to ayakael/installer that referenced this pull request Jan 11, 2023
@tmds
Copy link
Member Author

tmds commented Jan 11, 2023

The changes will get mirrored during the 7.0.102 release in January.

The changes are part of the 7.0.102 tag, will they be mirrored automatically to the 7.0.1xx and 7.0.2xx branches?

@MichaelSimons
Copy link
Member

will they be mirrored automatically to the 7.0.1xx and 7.0.2xx branches?

Yes.

ayakael added a commit to ayakael/installer that referenced this pull request Jan 13, 2023
ayakael added a commit to ayakael/installer that referenced this pull request Jan 13, 2023
crummel pushed a commit to ayakael/installer that referenced this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants