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

Enabled source-building with mono runtime on any architecture. #14792

Merged
merged 4 commits into from Nov 15, 2022

Conversation

tmds
Copy link
Member

@tmds tmds commented Oct 19, 2022

Adds an argument to the top-level build script to use the mono runtime, and sets the flags needed for the different repos to work with mono.

This fixes dotnet/source-build#3076 and also enables building .NET on any architectures (including x64, and arm64) with mono runtime.

@crummel @MichaelSimons ptal

cc @uweigand @omajid @janani66

Adds an argument to the top-level build script to use the mono
runtime, and sets the flags needed for the different repos
to work with mono.
@tmds tmds requested a review from a team as a code owner October 19, 2022 13:29
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.

These changes are too late for GA. I would be supportive of taking them in 7.0 servicing if the patches are backported/signed off by the product teams.

eng/SourceBuild.props Show resolved Hide resolved
@omajid
Copy link
Member

omajid commented Oct 19, 2022

I can confirm this fixes source-build for me on s390x and ppc64le.

@tmds
Copy link
Member Author

tmds commented Oct 21, 2022

I've synced the sdk patch with dotnet/sdk#28674, and it now prints an informative error message to the user the feature is not supported.

These changes are too late for GA. I would be supportive of taking them in 7.0 servicing if the patches are backported/signed off by the product teams.

We need these patches for the mono-based builds. We can apply them ourselves if they are not included.

Others will be interested in having them also, like for Alpine (dotnet/source-build#3020). cc @ayakael

@ayakael
Copy link
Contributor

ayakael commented Oct 21, 2022

I'd also have armv6 be considered for mono build as well, as it is supported only on that backend.

@tmds
Copy link
Member Author

tmds commented Oct 26, 2022

I'll re-target this PR to main, and create PRs for the patches at the end of this week.

Like with PR that eliminates portable runtime, we can request backports once installer main is working.

Copy link
Contributor

@crummel crummel left a comment

Choose a reason for hiding this comment

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

This looks good to me so far, I'll check back in after the retargeting. Thanks Tom!

@tmds
Copy link
Member Author

tmds commented Nov 14, 2022

Close/Open to re-trigger CI.

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.

LGTM w/one minor comment.

@@ -11,6 +11,7 @@ usage() {
echo " --run-smoke-test don't build; run smoke tests"
echo " --with-packages <dir> use the specified directory of previously-built packages"
echo " --with-sdk <dir> use the SDK in the specified directory for bootstrapping"
echo " --use-mono-runtime output uses the mono runtime"
Copy link
Member

Choose a reason for hiding this comment

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

Please alphabetize.

@tmds
Copy link
Member Author

tmds commented Nov 18, 2022

@tmds
Copy link
Member Author

tmds commented Nov 30, 2022

The sdk PR got the 7.0.3 milestone yesterday.

We need to backport this PR to 7.0 too to complete the feature.

cc @crummel @MichaelSimons

@MichaelSimons
Copy link
Member

@tmds - I'm not making the connection on what needs to be backported. The 7.0.3 milestone includes the 7.0.103 SDK.

@tmds
Copy link
Member Author

tmds commented Nov 30, 2022

This PR (#14792) should be backported to 7.0.

@MichaelSimons
Copy link
Member

This PR (#14792) should be backported to 7.0.

But it looks like this PR was merged into release/7.0.1xx. What am I missing?

@tmds
Copy link
Member Author

tmds commented Nov 30, 2022

Oops, I thought this PR was targeting main. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants