Do not set CrossBuild to true when building runtime tests for Android #55641

fanyang-mono · 2021-07-14T13:27:24Z

ghost · 2021-07-14T13:27:30Z

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

Issue Details

Author:	fanyang-mono
Assignees:	-
Labels:	`area-Infrastructure-mono`
Milestone:	-

ghost · 2021-07-14T13:27:55Z

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

Issue Details

Author:	fanyang-mono
Assignees:	-
Labels:	`area-Infrastructure`
Milestone:	-

fanyang-mono · 2021-07-14T21:59:58Z

This change makes FreeBSD build failing though. Need to investigate further.

ViktorHofer · 2021-07-22T07:22:07Z

cc @am11

am11 · 2021-07-22T09:13:56Z

Strange that it didn't showed up in CI runs. Was it excluded for some other reason?
PackageRID is the RID to restore package. OutputRid is the RID to create shipping package.

If changing package RID is fixing the "Building Targeting Pack" issue on Android, while all other platforms are fine, I think we should find the reason why Android with CrossBuild=true is expecting package restore RID to be same as target, rather than host?

am11 · 2021-07-22T09:59:58Z

Does deleting this override line fix Android test build:

runtime/src/tests/run.proj

Line 651 in b937677

    
           <MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>

?

fanyang-mono · 2021-07-23T14:15:17Z

Does deleting this override line fix Android test build:

runtime/src/tests/run.proj

Line 651 in b937677

<MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>

?

No the failure happened before reaching to this point.

fanyang-mono · 2021-07-23T14:17:10Z

Strange that it didn't showed up in CI runs. Was it excluded for some other reason?
PackageRID is the RID to restore package. OutputRid is the RID to create shipping package.

If changing package RID is fixing the "Building Targeting Pack" issue on Android, while all other platforms are fine, I think we should find the reason why Android with CrossBuild=true is expecting package restore RID to be same as target, rather than host?

The Android arm64 lane only run on rolling build. That's why it didn't fail for the PR checks.

am11 · 2021-07-23T15:49:31Z

The Android arm64 lane only run on rolling build. That's why it didn't fail for the PR checks.

Is rolling build different than the official build? We did ran official build in that PR.

No

Alright, then this particular leg of Android is different than anything else we have in PR, official and unofficial platform matrix. We are not passing -cross and explicitly setting /CrossBuild=false, when it is clearly cross-compilation on Ubuntu for Android. I think we should try to fix this leg's configuration, rather than changing the meaning of things again.

… to true

fanyang-mono · 2021-07-23T16:43:10Z

Is rolling build different than the official build? We did ran official build in that PR.
Rolling build is the build against main, which ran a couple times per day. It is not triggered as often as for every PR.

Alright, then this particular leg of Android is different than anything else we have in PR, official and unofficial platform matrix. We are not passing -cross and explicitly setting /CrossBuild=false, when it is clearly cross-compilation on Ubuntu for Android. I think we should try to fix this leg's configuration, rather than changing the meaning of things again.

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true. I updated the condition for _packageRID to accommodate this scenario.

ViktorHofer · 2021-07-23T16:46:37Z

Is rolling build different than the official build? We did ran official build in that PR.

Unfortunately yes but fortunately they are public builds. Rolling builds are ones that are triggered when a PR is merged, they run on the main branch. Because of the low number of hardware available for certain configurations we can't run all configurations per PR which is why some only run on rolling builds. We usually only have about 10-15 rolling builds per day (as those are batched together as well).

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true

Why is PortableBuild=true passed in at all? It's already true by default:

runtime/Directory.Build.props

Line 111 in e60882e

fanyang-mono · 2021-07-23T17:00:41Z

Why is PortableBuild=true passed in at all?

It was set at
https://github.com/dotnet/runtime/blob/main/src/tests/build.sh#L362
and
https://github.com/dotnet/runtime/blob/main/src/tests/build.sh#L400

The runtime test infrastructure is a complicated beast. Actually, I don't know exactly why it PortableBuild=true needs to be passed in explicitly. Maybe we don't need to. Even thought it was not passed in explicitly, PortableBuild will still be true. And the problem that this PR is trying to solve will still be there.

am11 · 2021-07-23T17:02:20Z

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true. I updated the condition for _packageRID to accommodate this scenario.

Ah, ok. I was looking at (https://dev.azure.com/dnceng/public/_build/results?buildId=1237622&view=logs&j=1fbc174f-133f-57a8-dcec-0de15ad033b4&t=827c0caa-5f6d-5c33-e064-1c275c207b92) Build Product step and there we have:

/__w/1/s/build.sh -ci -arch arm64 -os Android -s mono+libs -c Release /p:CrossBuild=false

I don't know how to explain it based on what we understood. If we are cross compiling then /p:CrossBuild should be true (or just pass -cross) and ROOTFS_DIR environment variable should be set. Similarly, in test build step:

/__w/1/s/src/tests/build.sh /p:LibrariesConfiguration=Release -ci -excludemonofailures os Android arm64 /p:RuntimeVariant= Release

should have -cross flag set rather than setting it somewhere in the middle of the build? Then we won't be needing one-off workarounds like

runtime/src/tests/run.proj

Line 651 in b937677

    
           <MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>

if something is not working with ROOTFS_DIR and -cross (which has been the cross build way in runtime/coreclr/corefx/core-setup repos for a while now), we can investigate it and fix to make it work.

am11

Thank you! Looks great. :)

Android cross-build seems a bit different than other platforms, as while it is a cross build, we don't set -cross arguments (and set ROOTFS_DIR environment variable) for it. It is probably to workaround something, but IMHO, we should try to fix the reason of workaround at some point, so we could cross-build mono as well as coreclr for Android alike (#32800).

src/tests/build.sh

Co-authored-by: Adeel Mujahid <[email protected]>

fanyang-mono · 2021-07-29T13:41:09Z

Thank you! Looks great. :)

Android cross-build seems a bit different than other platforms, as while it is a cross build, we don't set -cross arguments (and set ROOTFS_DIR environment variable) for it. It is probably to workaround something, but IMHO, we should try to fix the reason of workaround at some point, so we could cross-build mono as well as coreclr for Android alike (#32800).

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

am11 · 2021-07-29T14:36:01Z

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

You have added an exception in test build script which reads "do not consider Android's cross build as typical cross-build".
So whatever workaround'ish reason it entails is what I was referring to. -cross is neither passed to Android's product build nor test build, while it's clearly a cross-build.

fanyang-mono · 2021-07-29T17:54:07Z

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

You have added an exception in test build script which reads "do not consider Android's cross build as typical cross-build".
So whatever workaround'ish reason it entails is what I was referring to. -cross is neither passed to Android's product build nor test build, while it's clearly a cross-build.

Ah, gotcha!

fanyang-mono · 2021-07-30T15:52:58Z

@am11 If this change looks good to you, do you mind approving it?

am11 · 2021-07-30T15:56:26Z

@ViktorHofer if you could give a green light on this too that would be great. :)

ghost · 2021-07-30T16:13:33Z

Hello @fanyang-mono!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

fanyang-mono added 2 commits July 14, 2021 09:20

Fix _packageRID

f6cfa5e

Hack to enable arm64 run for PR

9092d57

dotnet-issue-labeler bot added the area-Infrastructure-mono label Jul 14, 2021

fanyang-mono added area-Infrastructure and removed area-Infrastructure-mono labels Jul 14, 2021

karelz mentioned this pull request Jul 22, 2021

Test failure: System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled #41078

Closed

Dealing with the case when both PortableBuild and CrossBuild were set…

0844825

… to true

karelz mentioned this pull request Jul 27, 2021

Safe handle use after close in System.Net.Security.Tests #46770

Closed

Do not set CrossBuild to true when building for Android

8065a69

am11 reviewed Jul 29, 2021

View reviewed changes

src/tests/build.sh Outdated Show resolved Hide resolved

Update src/tests/build.sh

2d913a3

Co-authored-by: Adeel Mujahid <[email protected]>

Revert qualification hack

963d731

fanyang-mono changed the title ~~Fix _packageRID~~ Do not set CrossBuild to true when building runtime tests for Android Jul 29, 2021

fanyang-mono marked this pull request as ready for review July 29, 2021 19:20

fanyang-mono requested review from naricc and trylek July 29, 2021 19:21

fanyang-mono mentioned this pull request Jul 30, 2021

Consolidate Android cross-build setup for mono and coreclr #56622

Open

am11 approved these changes Jul 30, 2021

View reviewed changes

fanyang-mono requested a review from ViktorHofer July 30, 2021 16:13

fanyang-mono added the auto-merge label Jul 30, 2021

ViktorHofer approved these changes Jul 30, 2021

View reviewed changes

ghost merged commit 6fb0d44 into dotnet:main Jul 30, 2021

ghost locked as resolved and limited conversation to collaborators Aug 29, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not set CrossBuild to true when building runtime tests for Android #55641

Do not set CrossBuild to true when building runtime tests for Android #55641

fanyang-mono commented Jul 14, 2021 •

edited

Loading

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

fanyang-mono commented Jul 14, 2021

ViktorHofer commented Jul 22, 2021

am11 commented Jul 22, 2021

am11 commented Jul 22, 2021

fanyang-mono commented Jul 23, 2021

fanyang-mono commented Jul 23, 2021

am11 commented Jul 23, 2021 •

edited

Loading

fanyang-mono commented Jul 23, 2021

ViktorHofer commented Jul 23, 2021

fanyang-mono commented Jul 23, 2021 •

edited

Loading

am11 commented Jul 23, 2021 •

edited

Loading

am11 left a comment •

edited

Loading

fanyang-mono commented Jul 29, 2021

am11 commented Jul 29, 2021

fanyang-mono commented Jul 29, 2021

fanyang-mono commented Jul 30, 2021

am11 commented Jul 30, 2021

ghost commented Jul 30, 2021

Do not set CrossBuild to true when building runtime tests for Android #55641

Do not set CrossBuild to true when building runtime tests for Android #55641

Conversation

fanyang-mono commented Jul 14, 2021 • edited Loading

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

fanyang-mono commented Jul 14, 2021

ViktorHofer commented Jul 22, 2021

am11 commented Jul 22, 2021

am11 commented Jul 22, 2021

fanyang-mono commented Jul 23, 2021

fanyang-mono commented Jul 23, 2021

am11 commented Jul 23, 2021 • edited Loading

fanyang-mono commented Jul 23, 2021

ViktorHofer commented Jul 23, 2021

fanyang-mono commented Jul 23, 2021 • edited Loading

am11 commented Jul 23, 2021 • edited Loading

am11 left a comment • edited Loading

Choose a reason for hiding this comment

fanyang-mono commented Jul 29, 2021

am11 commented Jul 29, 2021

fanyang-mono commented Jul 29, 2021

fanyang-mono commented Jul 30, 2021

am11 commented Jul 30, 2021

ghost commented Jul 30, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

fanyang-mono commented Jul 14, 2021 •

edited

Loading

am11 commented Jul 23, 2021 •

edited

Loading

fanyang-mono commented Jul 23, 2021 •

edited

Loading

am11 commented Jul 23, 2021 •

edited

Loading

am11 left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.