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

Enable Alpine ARM support #41982

Merged
merged 4 commits into from
Sep 10, 2020
Merged

Enable Alpine ARM support #41982

merged 4 commits into from
Sep 10, 2020

Conversation

janvorli
Copy link
Member

@janvorli janvorli commented Sep 8, 2020

This change enables Alpine ARM support in the lab build scripts,
packages and RID graph.

This change enables Alpine ARM support in the lab build scripts,
packages and RID graph
@ghost
Copy link

ghost commented Sep 8, 2020

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

@ViktorHofer ViktorHofer requested a review from safern September 9, 2020 08:35
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Overall looks good. However this is not adding any builds to either official builds or PR/CI.

@janvorli
Copy link
Member Author

janvorli commented Sep 9, 2020

@safern I have not realized this before, but @trylek mentioned it yesterday too. I am going to update this PR with changes in the runtime.yml (I hope I understand it correctly that that is where we define what gets built in official builds and PR/CI). But I am quite confused by the seemingly random selection of OSes / architectures for various templates in that file. Do you have a guidance on where it would make sense to add the Linux_musl_arm? I've started by trying to put it everywhere where I've seen Linux_musl_arm64, but I am not sure if that's the right approach.

@safern
Copy link
Member

safern commented Sep 9, 2020

runtime.yml is for PR/CI -- runtime-official.yml is for official builds.

Do you have a guidance on where it would make sense to add the Linux_musl_arm? I've started by trying to put it everywhere where I've seen Linux_musl_arm64, but I am not sure if that's the right approach.

In runtime-official.yml that should be enough. For runtime.yml we should be more clinical because we don't have much resources to run tests on arm in all PRs. What I would suggest for PR/CI is to add this new platform wherever we have linux_musl_arm64 push the changes and by looking at the diff it would be easier to give feedback.

@janvorli
Copy link
Member Author

janvorli commented Sep 9, 2020

Thanks @safern, I've just added a new commit that updates runtime.yml and runtime-official.yml.

@safern
Copy link
Member

safern commented Sep 9, 2020

runtime-official.yml looks good.

runtime.yml is just adding build legs. Do we want to run libraries tests on linux_musl_arm against a checked runtime?

- Linux_musl_arm64

@safern
Copy link
Member

safern commented Sep 9, 2020

Also, do we know what kind of test coverage we need for linux_musl_arm?

@janvorli
Copy link
Member Author

janvorli commented Sep 9, 2020

Ah, I've missed that one. I'll add it in a second.
As for test coverage, it seems that we can use the same we have for Linux-musl-arm64. But I don't have a strong opinion there and I am not sure what is our reasoning behind test matrix for platforms we have right now.

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM from the yml changes.

@janvorli
Copy link
Member Author

janvorli commented Sep 9, 2020

@safern I can see that coreclr, libraries and installer builds for arm musl have succeeded, but library tests are failing early with this error:

./RunTests.sh: line 161: /root/helix/work/correlation/dotnet: No such file or directory

Do you have any idea what can be wrong? Is it possible that it is some kind of chicken and egg issue?

One of the test logs is here:
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-41982-merge-16b4c390e366456f83/Microsoft.Extensions.Configuration.CommandLine.Tests/console.d3067d68.log?sv=2019-02-02&se=2020-09-29T18%3A12%3A23Z&sr=c&sp=rl&sig=33ZU3C7EONbfYcvMy4tH4rS0g0dk6dFogdTW56XQMps%3D

@trylek
Copy link
Member

trylek commented Sep 9, 2020

@janvorli - I think we should take a closer look at this bit:

IncludeDotNetCli: false # optional -- true will download a version of the .NET CLI onto the Helix machine as a correlation payload; requires DotNetCliPackageType and DotNetCliVersion

@safern
Copy link
Member

safern commented Sep 9, 2020

Could it be that the dotnet host doesn't have support for Linux_musl_arm leg yet?

We consume the host package produced from dotnet/runtime. So what we should probably do is merge this without running tests and then once we produce a new host package with musl_arm support update the package version and add test runs.

@janvorli
Copy link
Member Author

janvorli commented Sep 9, 2020

I don't know where it was supposed to get the dotnet executable from. I was thinking that it would come from the installer build and test leg. But I guess you are most likely right and the package needs to be produced first.

@safern
Copy link
Member

safern commented Sep 9, 2020

Yeah it comes from a Package, we don't yet use the live built host. We have an active issue to do that.

@janvorli
Copy link
Member Author

Ok, let me disable the tests for now.

@janvorli
Copy link
Member Author

@ericstj do the RID related changes look good to you?

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Lgtm

@janvorli janvorli merged commit d992e0c into dotnet:master Sep 10, 2020
@janvorli janvorli deleted the enable-arm-musl branch September 10, 2020 13:36
directhex added a commit to directhex/runtime that referenced this pull request Sep 11, 2020
janvorli added a commit to janvorli/runtime that referenced this pull request Oct 1, 2020
This change ports to release/5.0 enabling Alpine ARM support
in the lab build scripts packages and RID graph. It enables
CI and official builds.
janvorli added a commit that referenced this pull request Oct 8, 2020
* Port enable Alpine ARM support (#41982)

This change ports to release/5.0 enabling Alpine ARM support
in the lab build scripts packages and RID graph. It enables
CI and official builds.

* Port of #42096
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

4 participants