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

Adding HelloiOS sample for NativeAOT #82249

Merged
merged 10 commits into from
Feb 24, 2023
Merged

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented Feb 16, 2023

This PR introduces necessary changes to build and run a sample application with NativeAOT on iOS-like platforms.
The application is intended to be used by developers who work on enabling NativeAOT on iOS-like platforms and can serve as PoC.

What is included

  • AppleAppBuilder adaptations to support NativeAOT-specific configuration
  • Sample project configured for building with NativeAOT and a makefile used for building the dependencies and the application
  • README.md which explains how to build/run the application

Takeaways


Contributes to: #80911

@ivanpovazan ivanpovazan self-assigned this Feb 16, 2023
@ivanpovazan ivanpovazan changed the title Adding HelloiOS sample for NativeAOT WIP: Adding HelloiOS sample for NativeAOT Feb 16, 2023
@ivanpovazan ivanpovazan added the os-ios Apple iOS label Feb 16, 2023
@ghost
Copy link

ghost commented Feb 16, 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 introduces necessary changes to build and run a sample application with NativeAOT on iOS-like platforms.

What is included

  • AppleAppBuilder adaptations to support NativeAOT-specific configuration
  • Sample project configured for building with NativeAOT and a makefile used for building the dependencies and the application
  • README.md which explains how to build/run the application

Takeaways

  • The application is currently located in src/mono/sample/iOS-NativeAOT directory, which is not ideal and will be changed in the future. I wanted to avoid interrupting the repo tree and believe that for now (in this initial NativeAOT on iOS support) the location is good enough. Later, once we have more complete NativeAOT on iOS support, we should move the sample into a tests directory where it should become a functional test. Nevertheless, I am open for suggestions where to place the application.
  • The PR is marked as a draft, as it depends on: Update NativeAOT build integration targets to include ioslike platforms #82086 which includes changes introduced in the last commit of this PR (should be reverted once merged).
  • The app has been locally tested on ios-arm64/iossimulator-arm64/maccatalyst-arm64

Fixes #80911

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost
Copy link

ghost commented Feb 16, 2023

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

Issue Details

This PR introduces necessary changes to build and run a sample application with NativeAOT on iOS-like platforms.

What is included

  • AppleAppBuilder adaptations to support NativeAOT-specific configuration
  • Sample project configured for building with NativeAOT and a makefile used for building the dependencies and the application
  • README.md which explains how to build/run the application

Takeaways

  • The application is currently located in src/mono/sample/iOS-NativeAOT directory, which is not ideal and will be changed in the future. I wanted to avoid interrupting the repo tree and believe that for now (in this initial NativeAOT on iOS support) the location is good enough. Later, once we have more complete NativeAOT on iOS support, we should move the sample into a tests directory where it should become a functional test. Nevertheless, I am open for suggestions where to place the application.
  • The PR is marked as a draft, as it depends on: Update NativeAOT build integration targets to include ioslike platforms #82086 which includes changes introduced in the last commit of this PR (should be reverted once merged).
  • The app has been locally tested on ios-arm64/iossimulator-arm64/maccatalyst-arm64

Fixes #80911

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

os-ios, area-NativeAOT-coreclr

Milestone: -

@ivanpovazan
Copy link
Member Author

ivanpovazan commented Feb 17, 2023

Failures are related

@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

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan ivanpovazan marked this pull request as ready for review February 20, 2023 09:39
@ivanpovazan ivanpovazan changed the title WIP: Adding HelloiOS sample for NativeAOT Adding HelloiOS sample for NativeAOT Feb 20, 2023
@@ -0,0 +1,55 @@
# NativeAOT iOS sample app
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it under mono folder?

Copy link
Member Author

@ivanpovazan ivanpovazan Feb 20, 2023

Choose a reason for hiding this comment

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

I agree the location is not ideal as I wrote in the description, I wanted to avoid interrupting the repo tree and think that until we have more complete support with NativeAOT it might stay like this, as these samples are generally used as PoC. Later, once we have more complete NativeAOT on iOS support, we should move the sample into a tests directory where it should become a functional test. Nevertheless, I am open for suggestions where to place the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@filipnavara filipnavara Feb 20, 2023

Choose a reason for hiding this comment

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

There's also https://github.com/dotnet/runtime/tree/main/src/samples which may be better than the mono location. I assume the desire is to keep it in the same repository to allow quick iteration of AppleAppBuilder changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MichalStrehovsky what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion. If the sample is not building/testing, it will go stale and stop working quickly. This sample is digging into too many unstable internals to be placed in the dotnet/samples repo.

I don't know about the src/samples directory. It looks like it's something we imported from runtimelab. It's unclear if intentionally; can't find any docs for what it is. If I try to dotnet build the project in it, it doesn't seem to build.

It's not clear to me who the target audience for the sample is. I don't have an opinion on where it should be.

Copy link
Member Author

@ivanpovazan ivanpovazan Feb 21, 2023

Choose a reason for hiding this comment

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

It's not clear to me who the target audience for the sample is.

The target audience, for now, are developers contributing to NativeAOT on iOS work and the sample serves as a verification method to locally test a change (or series of changes).

On the other hand, the sample is also a "proof" that with the current state of dotnet/runtime main, one can build an iOS-like application targeting different platforms with NativeAOT.

Once we have runtime packages ready, they will be referenced instead of referencing internals explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

What I was trying to say is that if this is not building/running, it will go stale and might as well live in a personal repo of one of the devs. It will likely get regularly broken now or in the future. If it's building and running, it would qualify as a test and would go to src/tests. I'm not firm on any of this but I was asked for an opinion :)

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 understand, and I am grateful for your input! :)
I was just describing the reasoning behind it.
Another thing I forgot the mention, is that the Mono sample, living in: https://github.com/dotnet/runtime/tree/main/src/mono/sample/iOS, is built regularly in size/performance regression tracking, so was thinking that in the future the same approach can be used for NativeAOT as well. This would prevent the sample to go stale.

Copy link
Member

Choose a reason for hiding this comment

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

We had very similar discussion when src/mono/sample was added initially. The problem is that we have overloaded meaning of samples.

  • Product samples that customers can copy&paste and they just work. These samples live in dotnet/samples repo. Samples under src/mono/sample or src/samples are not that. They depend on the rest of the repo, internal details and they are not generally something we would like to see replicated in uncontrolled way.
  • Basic apps that people working on the dotnet/runtime repo find convenient for ad-hoc testing and that are sometimes used for other purposes, like perf testing. They are simple end-to-end tests more than anything else. We use multiple conventions for these projects. src\coreclr\tools\aot\ILCompiler\repro is another example of this kind of convenience project. I think it is confusing that we call these samples.

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member

@ivanpovazan @akoeplinger should there be a nativeaot pipeline to avoid running the kitchen sink?

@ivanpovazan
Copy link
Member Author

@ivanpovazan @akoeplinger should there be a nativeaot pipeline to avoid running the kitchen sink?

Yes, maybe not required at the moment, but would be good to have some way of verifying the changes in the future (I suppose maccatalyst as a test platform?).

The reason I ran runtime-extra-platforms here, is to verify I did not break anything in AppleAppBuilder for Mono.

Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

Minor suggestions, LGTM.

src/mono/sample/iOS-NativeAOT/Makefile Show resolved Hide resolved
src/mono/sample/iOS-NativeAOT/Program.csproj Outdated Show resolved Hide resolved
src/mono/sample/iOS-NativeAOT/Program.csproj Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/util.m Outdated Show resolved Hide resolved
@kotlarmilos
Copy link
Member

tvos-arm64 failure is dotnet/arcade#11683.

@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

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

Failures are unrelated.

@ivanpovazan ivanpovazan mentioned this pull request Feb 24, 2023
2 tasks
@ivanpovazan ivanpovazan merged commit 78f8010 into dotnet:main Feb 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 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.

8 participants