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

Update NativeAOT build integration targets to include ioslike platforms #82086

Merged
merged 21 commits into from
Feb 20, 2023

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Feb 14, 2023

This PR updates NativeAOT build integration targets to include ioslike platforms, and contributes to dotnet/sdk#30068.

/cc: @ivanpovazan @akoeplinger

@ghost
Copy link

ghost commented Feb 14, 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 updates NativeAOT build integration targets to include ios and iossimulator, and contributes to dotnet/sdk#30068.

/cc: @ivanpovazan @akoeplinger

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-NativeAOT-coreclr

Milestone: -

@kotlarmilos kotlarmilos changed the title Update NativeAOT build integration targets to include ios and iossimulator Update NativeAOT build integration targets to include ioslike platforms Feb 15, 2023
@kotlarmilos kotlarmilos marked this pull request as ready for review February 15, 2023 12:18
…iler.SingleEntry.targets

Co-authored-by: Filip Navara <filip.navara@gmail.com>
kotlarmilos and others added 2 commits February 15, 2023 16:17
…e.targets

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
…iler.SingleEntry.targets

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
…iler.SingleEntry.targets

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@@ -23,13 +23,20 @@ The .NET Foundation licenses this file to you under the MIT license.
<IlcBuildTasksPath Condition="'$(IlcBuildTasksPath)' == ''">$(MSBuildThisFileDirectory)..\tools\netstandard\ILCompiler.Build.Tasks.dll</IlcBuildTasksPath>
<TargetOS Condition="$(RuntimeIdentifier.StartsWith('win'))">windows</TargetOS>
<TargetOS Condition="$(RuntimeIdentifier.StartsWith('osx'))">osx</TargetOS>
<TargetOS Condition="$(RuntimeIdentifier.StartsWith('maccatalyst'))">maccatalyst</TargetOS>
Copy link
Member

Choose a reason for hiding this comment

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

Should we use here the same principle as above with: <OSIdentifier Condition="'$(OSIdentifier)' == '' and $(RuntimeIdentifier.StartsWith('win'))">win</OSIdentifier> in the "first-match-wins" approach, and include '$(TargetOS)' == '' condition in this case as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, otherwise it won't select the first match.

Copy link
Member

@am11 am11 Feb 17, 2023

Choose a reason for hiding this comment

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

Up until now, these are not meant to be set from the outside, so we should also remove it from OSIdentifier since there is no use-case for it. We try to limit unnecessary extension points which become hard to maintain later and to justify their removal during refactoring. I am planning to look into deduplicating these two lists as we really need a single list of hardcoded platforms for shipping targets (ideally we shouldn't be even needing that and directly rely on SDK provided properties...).

Copy link
Member

Choose a reason for hiding this comment

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

I understand your concern and agree that if these properties are not meant to be extensible, the conditions should be removed (in both places).
However, if this is going to be changed in the near future in any case, can we agree on the implementation for this PR, so we can close it?
@jkotas 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.

can we agree on the implementation for this PR, so we can close it?

Sure, I'll follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas what do you think?

I agree that this can use refactoring that can be done as a follow up (not required for this PR).

<NativeFramework Include="CoreFoundation" />
<NativeFramework Include="CryptoKit" />
<NativeFramework Include="Foundation" />
<NativeFramework Include="Security" />
<NativeFramework Include="GSS" />
<!-- The library builds don't reference the GSS API on tvOS builds. -->
<NativeFramework Condition="'$(TargetOS)' != 'tvos' and '$(TargetOS)' != 'tvossimulator'" Include="GSS" />
Copy link
Member

Choose a reason for hiding this comment

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

nit: Use $(TargetOS.StartsWith('tvos'))

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 wasn't sure about negating the condition (didn't find it in the repository). Would $(!TargetOS.StartsWith('tvos')) work?

Copy link
Member

Choose a reason for hiding this comment

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

I find the explicit version a bit easier to read. !$(TargetOS.StartsWith('tvos')) (! first) should work but I didn't try it.

Copy link
Member

Choose a reason for hiding this comment

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

I also find the explicit version easier to reason about.

@ivanpovazan
Copy link
Member

FWIW: Failures are unrelated

@kotlarmilos
Copy link
Member Author

Failure looks relevant. Seems that 407edf3 conditioned out libraries related to msquic.

@ivanpovazan
Copy link
Member

Failure looks relevant. Seems that 407edf3 conditioned out libraries related to msquic.

Seeing it here as well: https://github.com/dotnet/runtime/pull/82249/checks?check_run_id=11417387284

@kotlarmilos
Copy link
Member Author

[QUIC] Missing libmsquic on tested platforms #81901

Tracking issue: #81901

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

lgtm!
@MichalStrehovsky is there anything else that would prevent us from merging this ?

@@ -63,11 +63,13 @@ The .NET Foundation licenses this file to you under the MIT license.

<ItemGroup>
<NetCoreAppNativeLibrary Include="System.Native" />
<NetCoreAppNativeLibrary Include="System.Globalization.Native" Condition="'$(StaticICULinking)' != 'true'" />
<!-- FIXME: The library is currently not available for iOS-like platforms -->
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue we could link to?

Copy link
Member

@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.

Just created and linked: #82389

@kotlarmilos kotlarmilos merged commit 06dd126 into dotnet:main Feb 20, 2023
@kotlarmilos kotlarmilos deleted the feature/nativeaot-targets-ios branch February 20, 2023 09:09
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 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