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

[WIP] source-build: don't skip building Native Aot artifacts. #88520

Closed
wants to merge 4 commits into from

Conversation

tmds
Copy link
Member

@tmds tmds commented Jul 7, 2023

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2023
@ghost
Copy link

ghost commented Jul 7, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to dotnet/source-build#1215.

cc @jkotas @MichaelSimons @premun @omajid @ashnaga

Author: tmds
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr

Milestone: -

@@ -136,6 +136,9 @@ The .NET Foundation licenses this file to you under the MIT license.
<LinkerArg Include="-static-pie" Condition="'$(StaticExecutable)' == 'true' and '$(PositionIndependentExecutable)' != 'false'" />
<LinkerArg Include="-dynamiclib" Condition="'$(_IsApplePlatform)' == 'true' and '$(NativeLib)' == 'Shared'" />
<LinkerArg Include="-shared" Condition="'$(_IsApplePlatform)' != 'true' and '$(NativeLib)' == 'Shared'" />
<!-- source-build links directly with openssl (-lssl -lcrypto) -->
<LinkerArg Include="-lssl" />
<LinkerArg Include="-lcrypto" />
Copy link
Member Author

@tmds tmds Jul 7, 2023

Choose a reason for hiding this comment

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

This relates to #66859.

I've unconditionally added these flags because there is no condition that tells us if the build links with OpenSSL (source-build non-portable) or loads it dynamically (dlopen).

In the latter case the flags are not needed. Perhaps they don't cause issues either (on platforms where .NET dlopens OpenSSL).

@@ -136,6 +136,9 @@ The .NET Foundation licenses this file to you under the MIT license.
<LinkerArg Include="-static-pie" Condition="'$(StaticExecutable)' == 'true' and '$(PositionIndependentExecutable)' != 'false'" />
<LinkerArg Include="-dynamiclib" Condition="'$(_IsApplePlatform)' == 'true' and '$(NativeLib)' == 'Shared'" />
<LinkerArg Include="-shared" Condition="'$(_IsApplePlatform)' != 'true' and '$(NativeLib)' == 'Shared'" />
<!-- source-build links directly with openssl (-lssl -lcrypto) -->
Copy link
Member

Choose a reason for hiding this comment

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

I assume that you will want to have libicu in this list as well. Is that correct?

I do not think that we want to pass in these libraries to the linker by default. We will need to have a new property for non-portable build output and set it as necessary.

What do you want the default for dotnet publish to be in distro-provided dotnet? Do you want it to produce the non-portable output default or portable output by default? This is general question for all targets, not specific to native AOT. Does it produce portable or non-portable output today?

Copy link
Member

@am11 am11 Jul 8, 2023

Choose a reason for hiding this comment

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

In the first PropertyGroup, we can explicitly define an internal property:

<_NonPortableBuild Condition="'$(NETCoreSdkRuntimeIdentifier)' != '$(NETCoreSdkPortableRuntimeIdentifier)'">true</_NonPortableBuild>

After that, make decisions based on that property: Condition="'$(_NonPortableBuild)' == 'true'"; without locking down to linux.

Another problem with the current approach is that it disregards static / customization options: https://github.com/dotnet/runtime/blob/60799cc5705fce35d04612dc24bdba46ed5a4f0e/src/coreclr/nativeaot/docs/compiling.md#using-statically-linked-icu

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you want the default for dotnet publish to be in distro-provided dotnet? Do you want it to produce the non-portable output default or portable output by default? This is general question for all targets, not specific to native AOT. Does it produce portable or non-portable output today?

Since non-portable and portable have different traits, we want this to be a deliberate choice by the user.

Similar to choosing between the .NET bundled runtime packs or the portable Microsoft NuGet packages, the UX for this is the user specify the non-portable rid (instead of the portable rid).

We should then add the linker flags based on the selected runtime.

Copy link
Member Author

@tmds tmds Jul 8, 2023

Choose a reason for hiding this comment

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

I assume that you will want to have libicu in this list as well. Is that correct?

I didn't need to add it to get this compiled on my machine, and ldd doesn't show it on my Fedora .NET .so files.
source-build may be using icu in the same way as the portable builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to choosing between the .NET bundled runtime packs or the portable Microsoft NuGet packages, the UX for this is the user specify the non-portable rid (instead of the portable rid).

I'm taking a look at the packages involved in a native aot publish using the preview 5 SDK.
I see Microsoft.DotNet.ILCompiler which is the tooling, and runtime.linux-x64.Microsoft.DotNet.ILCompiler which contains the runtime.

For source-build, the first should be identical to the Microsoft one.
And for the second, we want to produce a non-portable rid package, for example: runtime.fedora.38-x64.Microsoft.DotNet.ILCompiler

The extra linker flags describe what is in the second package, so we should include them in that package when it gets built from source.

The first package then needs a mechanism to pick those up.

@jkotas does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

The split between RID neutral and RID specific packages is only a download size optimization. It is not designed to allow composing RID specific packages of different origin.

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 think the SDK finds the rid specific package explicitly based on the information from BundledVersions.props which we control for source-build:

    <KnownILCompilerPack Include="Microsoft.DotNet.ILCompiler"
                         TargetFramework="net8.0"
                         ILCompilerPackNamePattern="runtime.**RID**.Microsoft.DotNet.ILCompiler"
                         ILCompilerPackVersion="8.0.0-preview.5.23280.8"
                         ILCompilerRuntimeIdentifiers="linux-musl-x64;linux-x64;win-x64;linux-arm;linux-arm64;linux-musl-arm;linux-musl-arm64;osx-arm64;osx-x64;win-arm;win-arm64;win-x86"
                         />

Copy link
Member

Choose a reason for hiding this comment

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

Yes and ILCompilerRuntimeIdentifiers has hardcoded finite lists of RIDs. What happens for RIDs outside this hardcoded list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and ILCompilerRuntimeIdentifiers has hardcoded finite lists of RIDs. What happens for RIDs outside this hardcoded list?

For some of the lists it will use the runtime graph to find the most appropriate rid from the finite set.

It's clear some changes are needed for the SDK, Microsoft.DotNet.ILCompiler, and runtime.<rid>.Microsoft.DotNet.ILCompiler to work together when built from source.

I'm going to avoid the linker flags issue for now by making the build use the openssl portable dlopen when source-build and nativeaot are combined, and see what else comes up.

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'm going to avoid the linker flags issue for now by making the build use the openssl portable dlopen when source-build and nativeaot are combined, and see what else comes up.

Added 3024212 for this.

@tmds
Copy link
Member Author

tmds commented Jul 11, 2023

@MichaelSimons the source-build leg fails on detecting the prebuilt from llvm-project. What should I do about it?

.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23328.2/tools/SourceBuild/AfterSourceBuild.proj(68,5): error : (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) 1 new pre-builts discovered! Detailed usage report can be found at /__w/1/s/artifacts/source-build/self/prebuilt-report/baseline-comparison.xml.
See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
runtime.linux-x64.Microsoft.NETCore.Runtime.ObjWriter.16.0.5-alpha.1.23354.1

@tmds
Copy link
Member Author

tmds commented Aug 16, 2023

@MichaelSimons the source-build leg fails on detecting the prebuilt from llvm-project. What should I do about it?

@MichaelSimons can you provide some guidance?

@MichaelSimons
Copy link
Member

@MichaelSimons the source-build leg fails on detecting the prebuilt from llvm-project. What should I do about it?

@MichaelSimons can you provide some guidance?

One (and only one) of the dependencies in the Version.Details.xml file need to be marked as source-build.

When building the repo for source-build, the infrastructure will retrieve the source-build intermediate package for the specified version from the dependent repo and prepopulate the package cache with the inner packages from that repo's source-build leg. Those pre-cached packages are then the known set of package that can be used and not flagged as prebuilts.

@mateusrodrigues
Copy link
Member

Hi! I'd like to get a discussion going on source mapping the LLVM source code that doesn't get built by a Linux source build in a way that it doesn't end up landing in the source tarball that's shipped by Linux distros.

The thinking behind this is that, even though LLVM in its entirety wouldn't get built as part of the Linux source-build process, the code would still be present in the source package. This would require us to do thorough license and security scans in that code when, in Canonical's case, doing a Main Inclusion Request (moving the package from universe to main in our archive), which is what we're doing for all .NET LTS releases (.NET 8 being on of them).

@@ -48,8 +48,7 @@

<!-- CoreDisTools are used in debugging visualizers. -->
<IncludeCoreDisTools Condition="'$(Configuration)' != 'Release' and '$(CrossHostArch)' == ''">true</IncludeCoreDisTools>
<!-- source-build doesn't use ObjWriter for the ILCompiler. the end-user will end up pulling Microsoft-built bits for NativeAOT anyway. -->
<IncludeObjWriter Condition="'$(DotNetBuildFromSource)' != 'true'">true</IncludeObjWriter>
<IncludeObjWriter>true</IncludeObjWriter>
Copy link
Member

Choose a reason for hiding this comment

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

Could you do one more level of constant propagation on this and also delete the property?

@agocke
Copy link
Member

agocke commented Oct 16, 2023

Moving this one to draft for now. I think there are a lot of missing pieces that we need to import.

@agocke agocke marked this pull request as draft October 16, 2023 20:05
@ghost ghost closed this Nov 15, 2023
@ghost
Copy link

ghost commented Nov 15, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants