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

[NativeAOT] Improving HelloiOS sample for NativeAOT to be testable with runtime packs #86652

Merged
merged 21 commits into from
Jun 8, 2023

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented May 23, 2023

Description

This PR improves testing NativeAOT iOS build integration by adjusting the build of HelloiOS sample.
The list of changes:

  • introducing an option to build HelloiOS sample with NativeAOT ios runtime packs (see bellow invocation instructions)
  • reusing LinkerArg items populated by NativeAOT targets
  • improving the way AppleAppBuilder passes extra linker arguments to CMake (this avoids weird duplication of linker flags when extra linker arguments are passed along through target_link_libraries)

Instructions

  • To build/run the app without runtime packs run the following command on a clean checkout
    make world TARGET_OS=ios DEPLOY_AND_RUN=true -C src/mono/sample/iOS-NativeAOT
  • To build/run the app with runtime packs run the following command on a clean checkout:
    make world TARGET_OS=ios DEPLOY_AND_RUN=true USE_RUNTIME_PACKS=true -C src/mono/sample/iOS-NativeAOT
    • This requires dotnet sdk version 8.0.100-preview.5+ and should be reverted once the testing is done

Current state

Scenario: USE_RUNTIME_PACKS=false on a ios-arm64 device

  • building
  • running

Scenario: USE_RUNTIME_PACKS=true on a ios-arm64 device

  • building
  • running

Final note


Contributes to #80911

@ivanpovazan ivanpovazan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) os-ios Apple iOS area-NativeAOT-coreclr labels May 23, 2023
@ivanpovazan ivanpovazan self-assigned this May 23, 2023
@ivanpovazan
Copy link
Member Author

/cc: @akoeplinger @kotlarmilos

@ghost
Copy link

ghost commented May 23, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR is currently marked as draft and should be used just for testing and receiving feedback.

Description

On the long run, it will improve testing NativeAOT iOS build integration by adjusting the build of HelloiOS sample.
The list of changes:

  • introducing an option to build HelloiOS sample with NativeAOT ios runtime packs (see bellow invocation instructions)
  • reusing LinkerArg items populated by NativeAOT targets
  • improving the way AppleAppBuilder passes extra linker arguments to CMake (this avoids weird duplication of linker flags when extra linker arguments are passed along to target_link_libraries)

Instructions

  • To build/run the app without runtime packs run the following command on a clean checkout
    make world TARGET_OS=ios DEPLOY_AND_RUN=true -C src/mono/sample/iOS-NativeAOT
  • To build/run the app with runtime packs run the following command on a clean checkout:
    make world TARGET_OS=ios DEPLOY_AND_RUN=true USE_RUNTIME_PACKS=true -C src/mono/sample/iOS-NativeAOT
    • This requires dotnet sdk version 8.0.100-preview.5+ and should be reverted once the testing is done

Current state

Scenario: USE_RUNTIME_PACKS=false on a ios-arm64 device

  • building
  • running

Scenario: USE_RUNTIME_PACKS=true on a ios-arm64 device

  • building
  • running crashes on startup -> I am currently investigating the stacktrace

Final note


Contributes to #80911

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

NO-MERGE, os-ios, area-NativeAOT-coreclr

Milestone: -

@ghost
Copy link

ghost commented May 23, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR is currently marked as draft and should be used just for testing and receiving feedback.

Description

On the long run, it will improve testing NativeAOT iOS build integration by adjusting the build of HelloiOS sample.
The list of changes:

  • introducing an option to build HelloiOS sample with NativeAOT ios runtime packs (see bellow invocation instructions)
  • reusing LinkerArg items populated by NativeAOT targets
  • improving the way AppleAppBuilder passes extra linker arguments to CMake (this avoids weird duplication of linker flags when extra linker arguments are passed along to target_link_libraries)

Instructions

  • To build/run the app without runtime packs run the following command on a clean checkout
    make world TARGET_OS=ios DEPLOY_AND_RUN=true -C src/mono/sample/iOS-NativeAOT
  • To build/run the app with runtime packs run the following command on a clean checkout:
    make world TARGET_OS=ios DEPLOY_AND_RUN=true USE_RUNTIME_PACKS=true -C src/mono/sample/iOS-NativeAOT
    • This requires dotnet sdk version 8.0.100-preview.5+ and should be reverted once the testing is done

Current state

Scenario: USE_RUNTIME_PACKS=false on a ios-arm64 device

  • building
  • running

Scenario: USE_RUNTIME_PACKS=true on a ios-arm64 device

  • building
  • running crashes on startup -> I am currently investigating the stacktrace

Final note


Contributes to #80911

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

NO-MERGE, os-ios, area-NativeAOT-coreclr

Milestone: -

@ivanpovazan
Copy link
Member Author

ivanpovazan commented May 24, 2023

@MichalStrehovsky it seems b412b6b introduced a regression for building and running the HelloiOS sample with NativeAOT on a device (in both debug and release configuration).

I have confirmed this by checking out the commit and building and running the app before and after I revert the commit in question.
When the commit is included the app crashes on startup with the following stack trace:
https://gist.github.com/ivanpovazan/b9a0eae347a76ea39cf1f943b801a8e1

There is a simple repro which builds everything and runs the app via: make world TARGET_OS=ios DEPLOY_AND_RUN=true USE_RUNTIME_PACKS=true -C src/mono/sample/iOS-NativeAOT
However, if you do not have mac device at hand here is the repro package for ILC:
2828501054_Program.zip

Finally, I will also try to investigate further why this started failing.

PS Just for reference, when we merge this and Filip's PR we should be almost there to setup CI testing so these kind of regressions are caught early on.

@MichalStrehovsky
Copy link
Member

I have confirmed this by checking out the commit and building and running the app before and after I revert the commit in question.

Hmm, I don't see anything that would explain why there would be a problem on iDevices after this. Note that the commit in question requires a synchronized update in libRuntime, CoreLib, System.Private.TypeLoader, and the AOT compiler (a fundamental data structure was changed that is understood by all of these). I'd expect the stack you're seeing if one of these was stale on your machine.

@filipnavara
Copy link
Member

I'd expect the stack you're seeing if one of these was stale on your machine.

It uses runtime packs from a specific .NET package but the ILCompiler is still the live one, right, @ivanpovazan?

@ivanpovazan
Copy link
Member Author

ivanpovazan commented May 25, 2023

@MichalStrehovsky thanks for checking this, I think that is the problem as I updated global.json to a specific version, which means it probably picks up runtime packs before the commit that causes the discrepancy

@filipnavara yeah exactly ^ will try with a newer version of sdk

@ivanpovazan
Copy link
Member Author

Using newer sdk solves the issue as noted by: #86652 (comment)

@ivanpovazan ivanpovazan changed the title [WIP] Improving HelloiOS sample for NativeAOT to be testable with runtime packs [NativeAOT] Improving HelloiOS sample for NativeAOT to be testable with runtime packs May 25, 2023
@ivanpovazan ivanpovazan marked this pull request as ready for review May 25, 2023 10:07
@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

Many jobs seem to be timing out which is being tracked here: #76454
I will wait to have a healthy state on CI to rerun and verify this can be merged safely.

@filipnavara
Copy link
Member

#76454

dotnet/arcade#13774 was resolved. Time to retry?

@MichalStrehovsky
Copy link
Member

Extra-platforms will not succeed without #87203. They've been failing for a couple days during build stage (not a single test gets executed).

@filipnavara
Copy link
Member

Extra-platforms will not succeed without #87203.

Right. I wanted to point that out too but my fish memory managed to forget it :-)

@ivanpovazan
Copy link
Member Author

/azp run runtime,runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@filipnavara
Copy link
Member

I guess a rebase/merge is necessary since the builds use branch commits and not merge commits in the AzDO configuration?

@ivanpovazan
Copy link
Member Author

I guess a rebase/merge is necessary since the builds use branch commits and not merge commits in the AzDO configuration?

Sorry, I am not sure I understand what could be the problem of just rerunning the pipelines?

@filipnavara
Copy link
Member

filipnavara commented Jun 7, 2023

Sorry, I am not sure I understand what could be the problem of just rerunning the pipelines?

That you won't get the commit 6e34b5d that was merged to main and fixed the S.T.Json tests that blocked all the NativeAOT pipelines in the build phase.

@ivanpovazan
Copy link
Member Author

Sorry, I am not sure I understand what could be the problem of just rerunning the pipelines?

That you won't get the commit 6e34b5d that was merged to main and fixed the S.T.Json tests that blocked all the NativeAOT pipelines in the build phase.

Looking at the checkout before the build at: https://dev.azure.com/dnceng-public/public/_build/results?buildId=298056&view=logs&j=d282b611-679c-5014-3fe6-3a931ade305f&t=7b0acb1d-0623-531c-f6fb-76c84e37bf81&l=677
isn't
HEAD is now at 35aba423fd Merge c9d5543d3aa59c7a5b6a3056b18aea291a67b0f4 into 6e34b5d4e9b16321f37c108fea3aa7e4e04b495a making sure it takes the last-current tip of dotnet/main

@filipnavara
Copy link
Member

filipnavara commented Jun 7, 2023

Sorry for false alarm then. It's an option in the AzDO workflows and some other dotnet repositories build the (unmerged) branch (and I would swear that dotnet/runtime did as well).

@akoeplinger
Copy link
Member

akoeplinger commented Jun 7, 2023

Sorry for false alarm then. It's an option in the AzDO workflows and some other dotnet repositories build the (unmerged) branch (and I would swear that dotnet/runtime did as well).

It was like that a looong time ago but we use the merged-with-main state nowadays.

You'll see it in the checkout step where it says HEAD is now at 35aba423fd Merge c9d5543d3aa59c7a5b6a3056b18aea291a67b0f4 into 6e34b5d4e9b16321f37c108fea3aa7e4e04b495a

@filipnavara
Copy link
Member

filipnavara commented Jun 7, 2023

It was like that a looong time ago

Fine, you can call me grandpa Filip then.

(Also, the dotnet/installer repo didn't do that - #83695 (comment) - which was not that long time ago)

@ivanpovazan
Copy link
Member Author

Even with the 6e34b5d fix it seems we still hit time-outs in several jobs. Any suggestions on how to proceed with this?

@MichalStrehovsky
Copy link
Member

Even with the 6e34b5d fix it seems we still hit time-outs in several jobs. Any suggestions on how to proceed with this?

I'm fine merging with these timeouts for NativeAOT. I didn't look at/consider any of the Mono legs though.

@SamMonoRT
Copy link
Member

Even with the 6e34b5d fix it seems we still hit time-outs in several jobs. Any suggestions on how to proceed with this?

I have kicked off a re-run of the runtime and extra-platforms lanes - you should possibly have a newer set of results by the time you are at your desk tomorrow.

@steveisok
Copy link
Member

Separately, I think it would be a good idea to add a nativeaot only pipeline so we don't have to kick off the kitchen sink w/ runtime-extra-platforms. This would be similar to /azp run runtime-ioslike.

/cc @akoeplinger

@ivanpovazan
Copy link
Member Author

Thank you all very much for your help and assistance.
I went through the latest CI results, all the failures seem unrelated.
Additionally, I have opened tracking issues for noticed failures.

@ivanpovazan ivanpovazan merged commit 2835110 into dotnet:main Jun 8, 2023
@ivanpovazan ivanpovazan deleted the hellios-packs branch June 9, 2023 12:13
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2023
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.

7 participants