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

Add RID values for s390x #52907

Merged
merged 1 commit into from
Jun 2, 2021
Merged

Add RID values for s390x #52907

merged 1 commit into from
Jun 2, 2021

Conversation

uweigand
Copy link
Contributor

  • Add all supported s390x Linux distributions to runtimeGroups.props

  • Update generated files and test cases

@ericstj
Copy link
Member

ericstj commented May 19, 2021

Is s390x part of our supported matrix now? We recently did some work to make it easier to build on new architectures and platforms without adding them to the graph. Trying to understand if that's good enough here, or if there's a need to more officially support this architecture. cc @tmds @richlander @jkotas

@ericstj ericstj requested a review from jkotas May 19, 2021 17:00
@ericstj
Copy link
Member

ericstj commented May 19, 2021

I found #34195 where @jkotas mentioned this path, so I'm assuming this is agreed. I'd like his approval for merge.

@jkotas
Copy link
Member

jkotas commented May 19, 2021

Is s390x part of our supported matrix now?

It is community-supported, same as e.g. Tizen. It is not Microsoft-supported.

</RuntimeGroup>

<RuntimeGroup Include="alpine">
<Parent>linux-musl</Parent>
<Architectures>x64;arm;arm64</Architectures>
<Architectures>x64;arm;arm64;s390x</Architectures>
Copy link
Member

Choose a reason for hiding this comment

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

Is there really alpine for s390x ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we've been supporting alpine for a few years now, see e.g.
https://wiki.alpinelinux.org/wiki/S390x
https://alpinelinux.org/downloads/

Copy link
Member

Choose a reason for hiding this comment

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

@uweigand is there a need for anything other than "rhel"?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind getting s390x support on Fedora too. I think it's better to keep it as general as possible.

@tmds
Copy link
Member

tmds commented May 19, 2021

We recently did some work to make it easier to build on new architectures and platforms without adding them to the graph.

@ericstj based on that, I think we can leave out the versioned rids from this PR (like rhel.7-s390x and rhel.7.0-s390x). Is that correct?

@ericstj
Copy link
Member

ericstj commented May 19, 2021

I think we can leave out the versioned rids from this PR (like rhel.7-s390x and rhel.7.0-s390x). Is that correct?

Possibly, the one thing we get by adding them to the graph that we don't get by just building for those architectures is the ability for an SDK to build a self-contained application for that RID from another platform. For example: suppose I want to build a self-contained application for rhel.7-z390x from my Windows machine with the Microsoft-provided dotnet SDK. The only way that will work correctly is if the RID is added to the checked-in RID graph.

@tmds
Copy link
Member

tmds commented May 19, 2021

@ericstj based on that, I think we can leave out the versioned rids from this PR (like rhel.7-s390x and rhel.7.0-s390x). Is that correct?

@ericstj I think we may be even able to leave out "rhel", right?

So we need to add linux-s390, and linux-musl-s390 only?

I think we should give that a try, and maybe fix some issues we bump into along the way.

@uweigand
Copy link
Contributor Author

So the initial reason I had to add RIDs was that otherwise the loader would sometimes fail to find the appropriate runtime version of dependent libraries. For example, some test cases have "unix" versions of some libraries, and while the loader would know the current RID is "linux-s390x" even if that is not in the graph, it would fail to understand that "linux-s390x" is a version of "linux" is a version of "unix". Adding "linux-s390x" to the RID graph fixes this.

Now whether we absolutely need all the distro-specific RIDs, I'm not fully sure. I simply added all Linux distros that currently support s390x, for consistency with the other architectures. I don't fully understand the nuances what difference the distro-specific (or even version-specific) RIDs make, so I certainly appreciate guidance here.

@tmds
Copy link
Member

tmds commented May 19, 2021

@uweigand the rid graph has required everything to be specified up front. This has bitten us a couple of times when we tried to build .NET on a distro that was not yet in the graph (like a new version of Fedora). To improve, @ericstj implemented #50818 to add rids as part of the build. I think it means we only need to add linux-s390, and linux-musl-s390 here.

@uweigand
Copy link
Contributor Author

@uweigand the rid graph has required everything to be specified up front. This has bitten us a couple of times when we tried to build .NET on a distro that was not yet in the graph (like a new version of Fedora). To improve, @ericstj implemented #50818 to add rids as part of the build. I think it means we only need to add linux-s390, and linux-musl-s390 here.

OK, I'll give that a try. Thanks!

* Add Linux on the s390x architecture to runtimeGroups.props

* Update generated files
@uweigand
Copy link
Contributor Author

@uweigand the rid graph has required everything to be specified up front. This has bitten us a couple of times when we tried to build .NET on a distro that was not yet in the graph (like a new version of Fedora). To improve, @ericstj implemented #50818 to add rids as part of the build. I think it means we only need to add linux-s390, and linux-musl-s390 here.

OK, I'll give that a try. Thanks!

Seems to work for me so far. Patch updated.

@ericstj
Copy link
Member

ericstj commented May 19, 2021

@uweigand are you testing on an SDK that was built with #50818 in place, where s390x was specified in a RID to add (for example by passing this in as part of the TargetRID)? If so then I'd expect the shared framework built would have the necessary RID hierarchy generated at build.

@uweigand
Copy link
Contributor Author

@uweigand are you testing on an SDK that was built with #50818 in place, where s390x was specified in a RID to add (for example by passing this in as part of the TargetRID)? If so then I'd expect the shared framework built would have the necessary RID hierarchy generated at build.

Ah, good point! I'll need to rebuild the host SDK as well and retry.

@ericstj
Copy link
Member

ericstj commented May 19, 2021

Ah, good point! I'll need to rebuild the host SDK as well and retry.

Give that a try without any changes (including this PR). If you have the fix for #50818 and build for linux-s390x, that should produce a shared framework that "knows" that linux-s390x imports unix. I'm still willing to take this PR if we think it makes things easier, but I'd like to understand what's minimally required.

@tmds
Copy link
Member

tmds commented May 20, 2021

This definitely is more easy on the eyes.

I think we want this minimal version to work. We may hit some bumps, because this hasn't been used in practice yet.

@uweigand
Copy link
Contributor Author

Ah, good point! I'll need to rebuild the host SDK as well and retry.

Give that a try without any changes (including this PR). If you have the fix for #50818 and build for linux-s390x, that should produce a shared framework that "knows" that linux-s390x imports unix. I'm still willing to take this PR if we think it makes things easier, but I'd like to understand what's minimally required.

I've now confirmed the following:

  • With this "minimal" version applied, everything seems to work fine, just the same as with the original full RID set.
  • Without this minimal version applied, even on current mainline which includes Add support for adding RIDs to runtime.json during build #50818, I still see failures because libraries under "unix" are not pulled in.

If the latter is actually supposed to be working, I guess there's still a bug somewhere. Can you elaborate how this is supposed to work in detail?

@ericstj
Copy link
Member

ericstj commented May 20, 2021

I still see failures because libraries under "unix" are not pulled in.

Can you elaborate on where you see those failures? Here's what I would expect to happen:

  1. When building dotnet/runtime you're doing so either from some other architecture or you've modified the dotnet CLI manually to run on your new target architecture.
  2. When building dotnet/runtime you are specifying the new OutputRID (are you specifying DotNetBuildFromSource?)
  • You can check on the generated runtime.json here to ensure it has the new RIDs: artifacts\obj\Microsoft.NETCore.Platforms\*\runtime.json
  1. The resultant shared framework deps file should list the new RIDs, permitting the host to understand the relationship when running tests: artifacts\bin\testhost\*\shared\Microsoft.NETCore.App\6.0.0\Microsoft.NETCore.App.deps.json
  2. The same should be true for the runtime-packs and shared framework layouts produced by installer projects (essentially all the
    Microsoft.NETCore.App.deps.json files produced by the build ought to include your new RIDs).

I suspect we're missing some connection along the way here.

@uweigand
Copy link
Contributor Author

What I'm doing is more or less the following:

  1. On a Linux x64 machine, I'm cross-building a dotnet CLI for use on s390x. I'm doing this by mirroring the process how the official installer tarballs are build, i.e. I'm building the runtime, sdk, aspnetcore, and then installer projects, where each consumes the output generated by one of the former (might be either a tarball or some nuget packages). Finally, I end up with a dotnet-sdk-6.0.100-preview.3.21201.24-linux-s390x.tar.gz tarball.
  2. I move this tarball over to my Linux s390x machine, unpack it somewhere, and point my PATH to the dotnet binary that's in there. This gives me a working dotnet CLI running natively on s390x.
  3. On my Linux s390x machine working natively, using that CLI, I'm building the runtime repository and running the tests. This is done simply via ./build.sh and ./build.sh libs.tests --test. One of those tests fails as it doesn't find its library in a unix subdirectory. If I apply the patch in this PR to that runtime repository before building, that test succeeds instead.

I am not (yet) using the DotNetBuildFromSource method. (Going forward I'm planning to use this to do RPM package builds, but so far I did not try to get this to work.) Also, I am not manually setting OutputRID anywhere.

If I do not have this patch applied, then I'm not seeing linux-s390x (or any RID involving s390x) in any of the files you mention, neither in those included in the installer tarball I created via cross-compiling, nor in the files created by the native build of the runtime. When I have this patch applied to the runtime repo I'm building natively, the linux-s390x will then show up in the following set of files:

artifacts/bin/Microsoft.NETCore.Platforms/Debug/runtime.json.dgml
artifacts/bin/Microsoft.NETCore.Platforms.Tests/net6.0-Debug/runtime.json
artifacts/bin/Microsoft.NETCore.Platforms.Tests/net6.0-Debug/runtimeGroups.props
artifacts/bin/testhost/net6.0-Linux-Debug-s390x/shared/Microsoft.NETCore.App/6.0.0/Microsoft.NETCore.App.deps.json
artifacts/obj/Microsoft.NETCore.App.Bundle/Debug/net6.0/linux-s390x/output/shared/Microsoft.NETCore.App/6.0.0-dev/Microsoft.NETCore.App.deps.json
artifacts/obj/Microsoft.NETCore.App.Runtime/Debug/net6.0/linux-s390x/Microsoft.NETCore.App.deps.json
artifacts/obj/Microsoft.NETCore.App.Runtime/Debug/net6.0/linux-s390x/output/shared/Microsoft.NETCore.App/6.0.0-dev/Microsoft.NETCore.App.deps.json

What does OutputRID do, and where should I be setting it? During the cross-build of the CLI, or during the native build of the runtime? Note that in both places, the build system appears to be detecting the architecture just fine; during the cross-build I specify /p:TargetArchitecture=s390x, and the first line of the build output is actually:
__DistroRid: linux-s390x
And during the native build it detects the architecture it is running on automatically, and also gives the same __DistroRid output.

@tmds
Copy link
Member

tmds commented May 25, 2021

What does OutputRID do, and where should I be setting it?

@uweigand you can specify OutputRID and DotNetBuildFromSource as arguments to build.sh, e.g. /p:OutputRid=xxxx.
OutputRID should default to TargetRID which should be derived from /etc/os-release/. I don't think there is a need for you to set this.

DotNetBuildFromSource triggers some different behavior from the build, you should set it to true: /p:DotNetBuildFromSource=true.

With this "minimal" version applied, everything seems to work fine, just the same as with the original full RID set.

@ericstj I think this means the PR works as expected, and it is probably good to merge?

@tmds
Copy link
Member

tmds commented May 31, 2021

@ericstj is this good to merge?

@ericstj
Copy link
Member

ericstj commented Jun 1, 2021

Apologies for the delay, I missed the earlier reply.

What does OutputRID do, and where should I be setting it? During the cross-build of the CLI, or during the native build of the runtime? Note that in both places, the build system appears to be detecting the architecture just fine; during the cross-build I specify /p:TargetArchitecture=s390x, and the first line of the build output is actually:
__DistroRid: linux-s390x

I think this means that the output RID is being set, after this I'd expect the s390x to show up in the Microsoft.NETCore.Platforms package.

, nor in the files created by the native build of the runtime.

Given your process this makes sense. We only enable build time generation of the runtime.json when DotNetBuildFromSource is set:

<!-- When building from source, ensure the RID we're building for is part of the RID graph -->
<AdditionalRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">$(AdditionalRuntimeIdentifiers);$(OutputRID)</AdditionalRuntimeIdentifiers>

I think I determined another missing piece, which is that the shared-framework build is not consuming the generated runtime.json, which would cause tests to fail as was originally mentioned. I've opened #53550 for this and also mentioned the desire to support the non-source-build-new RID scenario.

@ericstj is this good to merge?

Given the minimal impact, I'm OK merging it to unblock. I would really like us to be in the place where we don't need to do this for new distros / archs that aren't part of official build matrix.

@ericstj ericstj merged commit 9ab81f7 into dotnet:main Jun 2, 2021
@tmds
Copy link
Member

tmds commented Jun 2, 2021

I would really like us to be in the place where we don't need to do this for new distros / archs that aren't part of official build matrix.

We didn't need to add the distros, which is already a huge improvement. 🎉

@uweigand uweigand deleted the s390x-rid branch June 2, 2021 10:30
@uweigand
Copy link
Contributor Author

uweigand commented Jun 2, 2021

Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants