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

Support building a mono-based .NET Runtime on x64 #68424

Merged
merged 1 commit into from
May 3, 2023

Conversation

omajid
Copy link
Member

@omajid omajid commented Apr 22, 2022

We now have some architectures (eg, s390x and ppc64le) that produce a .NET runtime that looks and feels like any other .NET runtime, except it's using Mono instead of CoreCLR under the hood.

However, it's only possible to produce this Mono-based .NET runtime on s390x/ppc64le. That makes it hard to test this set up on other architectures. Issues that affect the mono build on all architectures - such as #66594 become harder to fix and verify because of this unnecessary architecture requirement.

Fix that by adding a flag to the top-level build.sh (and also to the msbuild projects) to make it possible to produce a .NET runtime with Mono on any platform.

The default configuration is unchanged: we still produced CoreCLR-based .NET runtime on x64 and a Mono-based runtime on s390x/ppc64le.

Fixes: #62440

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 22, 2022
@omajid
Copy link
Member Author

omajid commented Apr 22, 2022

cc @directhex

@ghost
Copy link

ghost commented Apr 23, 2022

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

Issue Details

We now have some architectures (eg, s390x) that produce a .NET runtime that looks and feels like any other .NET runtime, except it's using Mono instead of CoreCLR under the hood.

However, it's only possible to produce this Mono-based .NET runtime on s390x. That makes it hard to test this set up on other architectures. Issues that affect the mono build on all architectures - such as #66594 become harder to fix and verify because of this unnecessary architecture requirement.

Fix that by adding a flag to the top-level build.sh (and also to the msbuild projects) to make it possible to produce a .NET runtime with Mono on any platform.

The default configuration is unchanged: we still produced CoreCLR-based .NET runtime on x64 and a Mono-based runtime on s390x.

Fixes: #62440

Author: omajid
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

Comment on lines 29 to 30
<PrimaryRuntimeFlavor Condition="'$(PrimaryRuntimeFlavor)' == '' and ('$(TargetArchitecture)' == 's390x' or '$(TargetArchitecture)' == 'armv6')">Mono</PrimaryRuntimeFlavor>
<PrimaryRuntimeFlavor Condition="'$(PrimaryRuntimeFlavor)' == ''">CoreCLR</PrimaryRuntimeFlavor>
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify the condition by splitting it across a few lines.

<DefaultPrimaryRuntimeFlavor>CoreCLR<DefaultPrimaryRuntimeFlavor/>
<DefaultPrimaryRuntimeFlavor Condition="'$(TargetArchitecture)' == 's390x'">Mono</DefaultPrimaryRuntimeFlavor>
<DefaultPrimaryRuntimeFlavor Condition="'$(TargetArchitecture)' == 'armv6'">Mono</DefaultPrimaryRuntimeFlavor>
<PrimaryRuntimeFlavor Condition="'$(PrimaryRuntimeFlavor)' == ''">$(DefaultPrimaryRuntimeFlavor)</PrimaryRuntimeFlavor>

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Can you please document the PrimaryRuntimeFlavor switch? Especially, I would be interested in a section that explains when to use PrimaryRuntimeFlavor vs RuntimeFlavor.

@omajid
Copy link
Member Author

omajid commented Apr 25, 2022

Thanks for the feeback!

Can you please document the PrimaryRuntimeFlavor switch?

FWIW, this PR doesn't add it; the property already existed. We do make it possible for users to override it. The eng/Subsets.props documents it like this already:

  <!-- Determine the primary runtime flavor. This is usually CoreCLR, except on
       platforms (like s390x) where only Mono is supported. The primary runtime
       flavor is used to decide when to build the hosts and installers. -->

And the --help also adds:

  echo "  -primaryRuntime                 Primary Runtime: CoreCLR or Mono. Unlike -runtimeFlavor, this produces a .NET-like"
  echo "                                  runtime (including tools, apphosts, etc) but uses either Mono or CoreCLR as the"
  echo "                                  actual runtime. Useful for building .NET on architectures without CoreCLR support,"
  echo "                                  or emulating the build for those architectures on x64."

What sort of detail are you looking for?

What would be a good place to document it?

@omajid
Copy link
Member Author

omajid commented Apr 25, 2022

Thanks to @tmds' comment (#62440 (comment)), I have learned that we don't need to adjust PrimaryRuntimeFlavor in the props file, we can simply pass in the correct value via msbuild property flag through build.sh - it will always override the property value defined in the props file. Is there any reason to add the conditionals to the .props file? I guess it makes things more explicit?

eng/build.sh Outdated
@@ -38,6 +38,10 @@ usage()
echo " [Default: Debug]"
echo " -runtimeFlavor (-rf) Runtime flavor: CoreCLR or Mono."
echo " [Default: CoreCLR]"
echo " -primaryRuntime Primary Runtime: CoreCLR or Mono. Unlike -runtimeFlavor, this produces a .NET-like"
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a corresponding change in build.ps1 for Windows?

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, we do. I need to learn Windows/Powershell :'(

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 don't have access to a Windows environment to fully test my changes, but I have used the powershell docker container image and Set-PSDebug -Trace 1 to confirm that

./eng/build.ps1 -primaryRuntime Mono -v d

Results in:

DEBUG:    1+  >>>> & "/runtime/eng/common/build.ps1" /p:TargetArchitecture=x64 -restore -build /p:PrimaryRuntimeFlavor=mono -verbosity d -configuration Debug

@ghost
Copy link

ghost commented May 23, 2022

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

Issue Details

We now have some architectures (eg, s390x) that produce a .NET runtime that looks and feels like any other .NET runtime, except it's using Mono instead of CoreCLR under the hood.

However, it's only possible to produce this Mono-based .NET runtime on s390x. That makes it hard to test this set up on other architectures. Issues that affect the mono build on all architectures - such as #66594 become harder to fix and verify because of this unnecessary architecture requirement.

Fix that by adding a flag to the top-level build.sh (and also to the msbuild projects) to make it possible to produce a .NET runtime with Mono on any platform.

The default configuration is unchanged: we still produced CoreCLR-based .NET runtime on x64 and a Mono-based runtime on s390x.

Fixes: #62440

Author: omajid
Assignees: -
Labels:

area-Infrastructure-mono, community-contribution

Milestone: -

@omajid omajid force-pushed the mono-x64-primary-runtime-flavor branch from 4ab4e75 to 6fbb9de Compare May 30, 2022 14:55
@ViktorHofer
Copy link
Member

There's a merge conflict in the PR. Apart from that, looks good.

@omajid omajid force-pushed the mono-x64-primary-runtime-flavor branch from 6fbb9de to b49c0f0 Compare May 30, 2022 16:05
eng/build.sh Outdated
echo " -primaryRuntime Primary Runtime: CoreCLR or Mono. Unlike -runtimeFlavor, this produces a .NET-like"
echo " runtime (including tools, apphosts, etc) but uses either Mono or CoreCLR as the"
echo " actual runtime. Useful for building .NET on architectures without CoreCLR support,"
echo " or emulating the build for those architectures on x64."
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I still don't like this name. I had rather had an argument that captures this produces a .NET runtime.

We now have:

To build a .NET-like runtime that uses CoreCLR on x64:

./build.sh

To build mono on x64:

./build.sh -runtimeFlavor Mono

To build a .NET-like runtime that uses mono on x64:

./build.sh -runtimeFlavor Mono -primaryRuntime Mono

Undefined behavior:

./build.sh -primaryRuntime Mono

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Do we really need a separate argument or could we say if you explicitly specify a runtimeFlavor then make that the primary runtime?

Copy link
Member

Choose a reason for hiding this comment

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

looks like this PR stalled a bit, what's our current thinking here?

Copy link
Member Author

@omajid omajid Aug 30, 2022

Choose a reason for hiding this comment

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

I am not sure what the way forward here is.

As far as a I can tell, ./build.sh -primaryRuntime Mono is the same as ./build.sh -runtimeFlavor Mono: you get mono and nothing else.

I see @tmds's point about making things more complex, but I am not sure what the desired way forward is.

I think making runtimeFlavor the primary runtime is the opposite of what we want. We want to be able to produce a .NET runtime (what this PR was calling primaryRuntime) where the underlying runtime (aka runtimeFlavor) is mono, not CoreCLR.

Copy link
Member

@tmds tmds Aug 31, 2022

Choose a reason for hiding this comment

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

Suggestion: add a flag named --mono-runtime with description Use the Mono runtime instead of the CoreCLR runtime..

The build scrip can use this to set PrimaryRuntimeFlavor/RuntimeFlavor as needed.

For the build script user, I think this provides a user-friendly option.

Copy link
Member

Choose a reason for hiding this comment

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

@akoeplinger what do you think about the suggestion?

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 like the idea of --mono-runtime! I think "runtime" is a little ambiguous, though - I think we are building the .NET runtime with the Mono VM (instead of the Mono runtime with the Mono VM).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I think we could shorten it to just --mono.

Copy link
Member

Choose a reason for hiding this comment

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

Let's use the same name as source-build: --use-mono-runtime.

https://github.com/dotnet/dotnet/blob/2fc4462a30e08fe5eccb6dbc271a4297d2bba75f/build.sh#L12

@steveisok
Copy link
Member

@omajid @tmds @akoeplinger This has been open for a while. Outside of resolving conflicts, what else needs resolved?

@omajid
Copy link
Member Author

omajid commented Feb 10, 2023

Outside of resolving conflicts, what else needs resolved?

I need to update this PR to use the new argument name that we have converged on.

@akoeplinger
Copy link
Member

@omajid let me know if you need help resolving the conflicts :)

@lewing
Copy link
Member

lewing commented Apr 11, 2023

This is the oldest open pr now :)

We now have some architectures (eg, s390x) that produce a .NET runtime
that looks and feels like any other .NET runtime, except it's using
Mono instead of CoreCLR under the hood.

However, it's only possible to produce this Mono-based .NET runtime on a
select few architectures. That makes it hard to test this set up on
other architectures.  Issues that affect the mono build on all
architectures - such as dotnet#66594 become harder to fix and verify because
of this unnecessary architecture requirement.

Fix that by adding a flag to the top-level build.sh (and also to the
msbuild projects) to make it possible to produce a .NET runtime with
Mono on any platform.

The default configuration is unchanged: we still produced CoreCLR-based
.NET runtime on x64 and a Mono-based runtime on arm32,ppc64le,s390x.

Fixes: dotnet#62440
@omajid omajid force-pushed the mono-x64-primary-runtime-flavor branch from b49c0f0 to 25fdd52 Compare April 11, 2023 16:47
@omajid omajid closed this Apr 11, 2023
@omajid omajid reopened this Apr 11, 2023
@omajid
Copy link
Member Author

omajid commented Apr 12, 2023

I have updated the PR. Anyone willing to review it?

@@ -360,6 +361,11 @@ while [[ $# > 0 ]]; do
shift 2
;;

-usemonoruntime)
Copy link
Member

Choose a reason for hiding this comment

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

The vmr option is --use-mono-runtime. Can you use the exact same name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! The rest of the options in the runtime build.sh use a completely different convention, though (eg outputrid, keepnativesymbols). Whatever I go with, it will look out of place with the other convention :(

Since this option lives in build.sh, I tried to keep it consistent with the other options in build.sh.

I am happy to change it if it's okay with the runtime maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that a "keeping it with current runtime convention sounds reasonable to me" or a "making it match the option in vmr sounds reasonable to me" ? 😄

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine leaving out the dashes. I didn't realize you had done it to be consistent with the other arguments.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the current state too.

@lewing lewing requested a review from steveisok April 26, 2023 02:13
@marek-safar
Copy link
Contributor

@directhex @akoeplinger could you please help landing this PR

@akoeplinger akoeplinger merged commit fc22ee9 into dotnet:main May 3, 2023
169 of 170 checks passed
@akoeplinger
Copy link
Member

Thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-mono 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.

support building mono on x64 as it gets built on s390x
9 participants