Pass TargetRid and SourceBuildNonPortable to the native scripts by tmds · Pull Request #74504 · dotnet/runtime

tmds · 2022-08-24T16:07:31Z

ghost · 2022-08-24T16:07:43Z

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

Issue Details

The native build script uses the portablebuild argument when calculating the distro rid.

Author:	tmds
Assignees:	-
Labels:	`area-Infrastructure-coreclr`
Milestone:	-

tmds · 2022-08-25T14:05:20Z

This PR passes the TargetRid and PortableBuild arguments to the native scripts.
The native scripts take these into account now when calculating __DistroRid.
When source-build passes a TargetRid that gets used as a __DistroRid.
Previously PortableBuild was ignored and assumed to be portable causing __DistroRid to always be the portable rid.

I've simplified init-distro-rid.sh a bit by only calling initNonPortableDistroRid for non-portable builds.

corehost managed and native now both use __DistroRid to determine the output directory.

…ative script.

tmds · 2022-08-26T13:47:54Z

This is now working.

When I run the ./build.sh command and pass in an unknown rid --packagerid fedora.40-x64 everything in the default set builds, with the exception of Microsoft.NETCore.App.Crossgen2.sfxproj which is using the PackageRID:

runtime/src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Crossgen2.sfxproj

Lines 32 to 48 in 7aef48a

 <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> 

Using /p:AdditionalRuntimeIdentifiers=fedora.40-x64 it should be possible to make Microsoft.NETCore.Platforms.csproj generate a package that can be used to let the this project know about the fedora.40-x64 rid. I'm don't know how to do this in practice. @ericstj do you have some suggestions?

This PR is up for review.

ericstj · 2022-08-26T18:37:21Z

generate a package that can be used to let the this project know about the fedora.40-x64 rid.

Looks like this is trying to publish crossgen2 as a self-contained application. I would imagine it needs #69455 and then needs to set BundledRuntimeIdentifierGraphFile. Still unclear to me if that's enough since I'm not sure how that project fetches the runtime that it's trying to link in.

tmds · 2022-08-28T06:47:56Z

Thanks @ericstj. I've created #74721 to look further into Microsoft.NETCore.App.Crossgen2.sfxproj for source-build.

am11

Few suggestions, otherwise looks good.

tmds · 2022-08-31T08:05:03Z

@am11 thanks for reviewing! I've addressed your feedback.

tmds · 2022-09-13T12:43:57Z

Is this good to merge?

tmds · 2022-09-20T11:14:38Z

@dotnet/runtime-infrastructure is this good to merge?

tmds · 2022-09-26T04:31:46Z

Can you merge this?

This enables building runtime repo for source-build on platforms where the target rid is not yet known in the rid graph.

ViktorHofer · 2022-09-26T10:17:44Z

Thanks for the work so far Tom. Unfortunately as already I mentioned, I'm not the right person to sign-off this change and I just asked the CLR team again to take a look.

trylek

I apologize, I'm not really an expert in this area. To my limited knowledge the change looks good but I'm reluctant to approve it, I'd definitely suggest finding a reviewer who's more familiar with source build.

tmds · 2022-09-27T06:18:08Z

If the Build Linux x64 release SourceBuild passes, that means this is in a good state.

@MichaelSimons can you review/approve from source-build perspective?

I've just extended SourceBuild.props so it figures out the appropriate RuntimeOS instead of taking it as an argument.
This needs to do a CI run.

ViktorHofer · 2022-09-27T08:33:10Z

@am11 @MichalStrehovsky can you please take a look? Am11 hope that's OK to ask you for a review but if I remember correctly, you made the original changes around RuntimeOS/TargetArch/TargetRid/... and have the most expertise in that area.

tmds · 2022-09-27T08:34:52Z

+
+ <!-- BaseOS is an expected known rid in the graph that TargetRid is compatible with.
+ It's used to add TargetRid in the graph if the parent can't be detected. -->
+ <BaseOS>$(RuntimeOS)</BaseOS>


TargetRid, RuntimeOS, and BaseOS should ripple down from source-build.
TargetRid names what is built.. The build will add this rid to graph. If it can't find a parent (for example, alpine.4.15-x64 finds alpine-x64 as the parent) then BaseOS is used as a parent. For example, if Orange Linux is a musl based version of Linux, BaseOS should be linux-musl.
RuntimeOS is the rid of the prebuilt SDK and artifacts that get used during the build. source-build/installer should set this to match the bootstrap SDK's NETCoreSdkRuntimeIdentifier.

MichalStrehovsky · 2022-09-27T08:57:56Z

@am11 @MichalStrehovsky can you please take a look? Am11 hope that's OK to ask you for a review but if I remember correctly, you made the original changes around RuntimeOS/TargetArch/TargetRid/... and have the most expertise in that area.

I can only be authoritative for the diff in src/coreclr/tools/aot/ILCompiler/ILCompiler.csproj. But this is an area that went through like 5 different values in the past year (used to be defined as __DistroRid, then ToolsRid, then Crossgen2PackageRid, then OutputRid, and now with this change PackageRid). I don't know what any of those mean. As long as the CI passes and we're still generating ARM64 bits when crossbuilding ARM64 from x64 (which would not be covered by the CI), that particular diff LGTM.

tmds · 2022-09-27T12:31:11Z

 <PropertyGroup>
 <OutputPath>$(RuntimeBinDir)ilc/</OutputPath>
- <RuntimeIdentifier>$(OutputRid)</RuntimeIdentifier>
+ <RuntimeIdentifier>$(PackageRID)</RuntimeIdentifier>


@MichalStrehovsky I think this may be breaking to the cross-compilation scenario.
OutputRid is linux-arm64 and PackageRID is linux-x64.
I see there is ILCompiler.csproj, and ILCompiler_crossarch.csproj.
This first, I guess, is used for creating a NuGet package (host: arm64 -> target: arm64)?
I think the latter is the one that is used on the build host (host: x64 -> target: arm64)?
Is that right?
I don't understand when/how ILCompiler gets packed. Where does this happen?

It gets packed by a special project file pkgproj src/installer/pkg/projects/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.pkgproj, which is ultimately using $(OutputRid) (via properties inheritance).
This change is correct. The reason we haven't gotten any complaints before is because we are not (yet) cross-compiling ILCompiler in a way that HostOS and TargetOS differ (the first part of RID...).

@am11 does Microsoft.DotNet.ILCompiler.pkgproj consume files built by ILCompiler.csproj?
If so, for what rid were these files built? OutputRid or PackageRID?

Microsoft.DotNet.ILCompiler.pkgproj consume files built by ILCompiler.csproj
It collects the files from various location and packs them together in a zip (.nupkg) file. In this case, PackageRID is only used to restore the packages when building the managed assembly (we don't have proeprocessor conditions based on RuntimeIdentifier, it's all MSIL or #if TARGET_{OS_or_ARCH} in the code). OutputRid is used in pkgprojs or passed to cmake to decide which bits to build and pack.

Sounds like we won't break anything then.
From what you said, I wonder: does ILCompiler.csproj need a RuntimeIdentifier?

does ILCompiler.csproj need a RuntimeIdentifier?
Good question. As I understood it; RuntimeIdentifer during the restore step is used to decide which runtime.{rid}.xyz for a given package to restore, if the package has native assets. Here, ILCompiler.props has multiple PackageReference and ProjectReference, it is likely that the resolved dependency graph of this project (under obj directory) has such package-with-native-assets which is requiring the RuntimeIdentifier to be specified.

am11 · 2022-09-27T13:14:38Z

Am11 hope that's OK to ask you for a review

@ViktorHofer, I approved this PR three weeks ago. :)

MichaelSimons

I approve from a source-build perspective.

jkoritzinsky

LGTM. I've also approved the Arcade PR

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

tmds · 2022-09-28T06:04:55Z

@ViktorHofer or someone else, can you merge this please?

ViktorHofer · 2022-09-28T09:48:00Z

Unrelated, but there was a segfault when invoking msbuild:

/__w/1/s/.dotnet/sdk/7.0.100-preview.7.22377.5/MSBuild.dll /nologo -maxcpucount /m -verbosity:m /v:minimal /bl:/__w/1/s/artifacts/log/Checked/ToolsetRestore.binlog /clp:Summary /clp:ErrorsOnly;NoSummary /nr:false /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=true /p:__ToolsetLocationOutputFile=/__w/1/s/artifacts/toolset/8.0.0-beta.22469.1.txt /t:__WriteToolsetLocation /warnaserror /__w/1/s/artifacts/toolset/restore.proj
/__w/1/s/eng/common/tools.sh: line 474: 512 Segmentation fault (core dumped) "$_InitializeBuildTool" "$@"
Build failed with exit code 139. Check errors above.
Finishing: Build and generate native prerequisites

Unfortunately I couldn't find a dump in the artifacts payload. Configuration: CoreCLR Product Build Linux x86 checked

ghost added area-Infrastructure-coreclr Only use for closed issues community-contribution Indicates that the PR has been added by a community member labels Aug 24, 2022

tmds changed the title ~~SourceBuild: pass portable argument via the build script arg.~~Pass TargetRid and SourceBuildNonPortable to the native scriptsAug 25, 2022

tmds mentioned this pull request Aug 25, 2022
corehost: align artifacts dir used by managed and native build. #74498
Closed

tmds force-pushed the sb_portable branch 3 times, most recently from b887874 to 45c616bCompare August 26, 2022 11:09

Pass SourceBuild TargetRid and SourceBuildPortable args through the n…
45a8ea8
…ative script.

tmds force-pushed the sb_portable branch from 45c616b to 45a8ea8Compare August 26, 2022 11:13

This was referenced Aug 30, 2022
Add Fedora 38 RID #74372
Merged
Ensure shared framework consumes source-built RID graph #53550
Closed

ViktorHofer requested a review from am11 August 30, 2022 14:53

tmds requested review from ViktorHofer and ericstj August 30, 2022 15:48

am11 reviewed Aug 30, 2022
View reviewed changes

Comment threadeng/build.sh Outdated
Comment threadeng/build.sh Outdated

tmds added 2 commits August 31, 2022 09:14

Rename -packagerid to -distrorid.
9d2af1d

Fix init-distro-rid.sh.
a619594

Use OutputRid instead of PackageRid.
599ef82

tmds commented Aug 31, 2022
View reviewed changes

Comment threadeng/build.sh Outdated

am11 reviewed Aug 31, 2022
View reviewed changes

Comment threadsrc/native/corehost/corehost.proj Outdated

tmds added 4 commits August 31, 2022 11:54

Drop unnecessary Condition.
f39f890

Try passing --distrorid as OutputRid.
3da8276

NativeExport: use PackageRID for AppHostRuntimeIdentifier.
2fdda4a

Rename --distrorid to --outputrid.
d89911f

tmds mentioned this pull request Sep 9, 2022
source-build.yml: add runtimeOS parameter. dotnet/arcade#10782
Merged

tmds mentioned this pull request Sep 15, 2022
Add support for banana linux dotnet/source-build#297
Closed
4 tasks

Merge branch 'main' into sb_portable
502c373

trylek reviewed Sep 26, 2022
View reviewed changes

tmds force-pushed the sb_portable branch from 61feae6 to 6606103Compare September 27, 2022 06:22

SourceBuild.props: derive RuntimeOS from NETCoreSdkRuntimeIdentifier.
0b228a7

tmds force-pushed the sb_portable branch from 6606103 to 0b228a7Compare September 27, 2022 06:23

Undo using NETCoreSdkRuntimeIdentifier.
5aa074a

ViktorHofer requested a review from am11 September 27, 2022 08:31

tmds commented Sep 27, 2022
View reviewed changes

MichaelSimons approved these changes Sep 27, 2022
View reviewed changes

jkoritzinsky approved these changes Sep 27, 2022
View reviewed changes

Comment threadeng/pipelines/common/global-build-job.yml

Update eng/pipelines/common/global-build-job.yml
1965c29
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

ViktorHofer merged commit 58c3d52 into dotnet:mainSep 28, 2022

tmds mentioned this pull request Sep 29, 2022
Eliminate portable runtime build. dotnet/installer#14549
Merged

ayakael mentioned this pull request Oct 17, 2022
[release/6.0] Add alpine to CI for source-build dotnet/installer#13074
Merged

ayakael mentioned this pull request Oct 26, 2022
[release/6.0] Pass TargetRid and SourceBuildNonPortable to the native scripts (backport of #74504) #77508
Merged

ghost locked as resolved and limited conversation to collaborators Oct 28, 2022

Uh oh!

Conversation

tmds commented Aug 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 24, 2022

Uh oh!

tmds commented Aug 25, 2022

Uh oh!

tmds commented Aug 26, 2022

Uh oh!

ericstj commented Aug 26, 2022

Uh oh!

tmds commented Aug 28, 2022

Uh oh!

am11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmds commented Aug 31, 2022

Uh oh!

Uh oh!

Uh oh!

tmds commented Sep 13, 2022

Uh oh!

tmds commented Sep 20, 2022

Uh oh!

tmds commented Sep 26, 2022

Uh oh!

ViktorHofer commented Sep 26, 2022

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

tmds commented Sep 27, 2022

Uh oh!

ViktorHofer commented Sep 27, 2022

Uh oh!

tmdsSep 27, 2022

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Sep 27, 2022

Uh oh!

tmdsSep 27, 2022

Choose a reason for hiding this comment

Uh oh!

am11Sep 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmdsSep 27, 2022

Choose a reason for hiding this comment

Uh oh!

am11Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

tmdsSep 27, 2022

Choose a reason for hiding this comment

Uh oh!

am11Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

am11 commented Sep 27, 2022

Uh oh!

MichaelSimons left a comment

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmds commented Sep 28, 2022

Uh oh!

ViktorHofer commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

tmds commented Aug 24, 2022•
edited
Loading

am11Sep 27, 2022•
edited
Loading