-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
+327
−16
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e0b8ca3
Integrate AppleAppBuilder with NativeAOT runtime
ivanpovazan 53baa49
Add HelloiOS sample variant for NativeAOT
ivanpovazan 433f8df
Adding a README.md
ivanpovazan ba174d2
Bump the version displayed on the screen when running HelloiOS app
ivanpovazan 65b08e3
Use util.m with Mono as well and fix the build
ivanpovazan c09b462
Use NativeDependencies property instead to align with AndroidAppBuild…
ivanpovazan b369a36
Adding fixes according to review feedback
ivanpovazan b621221
Adding fixes according to review feedback
ivanpovazan d42f3e3
Adding more details into the app README.md file to clarify the applic…
ivanpovazan d69ba2e
Markdown cleanup
ivanpovazan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW NativeAOT keeps samples here: https://github.com/dotnet/samples/tree/main/core/nativeaot
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.