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.

Sign up for GitHub

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

Jump to bottom

Replace IlcHostArch with SDK-computed value #82645

Merged
merged 12 commits into from
Feb 27, 2023
Merged

Replace IlcHostArch with SDK-computed value #82645

merged 12 commits into from
Feb 27, 2023

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 24, 2023

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 24, 2023
@jkotas jkotas requested a review from agocke February 24, 2023 21:38
@ghost
Copy link

ghost commented Feb 24, 2023

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

Issue Details

null

Author: am11
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr
Milestone: -

@am11 am11 force-pushed the feature/nativeaot/build-integration branch from 728efe2 to bf36e14 Compare February 24, 2023 23:22
@am11 am11 marked this pull request as ready for review February 25, 2023 01:21
@am11 am11 force-pushed the feature/nativeaot/build-integration branch from 54d2b7a to 269de39 Compare February 25, 2023 02:02
@am11 am11 force-pushed the feature/nativeaot/build-integration branch from 59dd15d to ae475ce Compare February 25, 2023 03:49
jkotas
jkotas approved these changes Feb 25, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. @agocke Any additional feedback?

@jkotas jkotas mentioned this pull request Feb 25, 2023
Closed
@am11 am11 requested a review from jkotas February 26, 2023 15:07
jkotas
jkotas approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

I would like @agocke to take a look as well.

@agocke
Copy link
Member

agocke commented Feb 27, 2023

Looking...

agocke
agocke reviewed Feb 27, 2023
View reviewed changes
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
<_indexOfHyphen>$(_targetOSPkg.IndexOf('-'))</_indexOfHyphen>
<_targetOS Condition="'$(_indexOfHyphen)' &gt; -1">$(_targetOSPkg.SubString(0, $(_indexOfHyphen)))</_targetOS>
<_targetOS Condition="'$(_indexOfHyphen)' == -1">$(_targetOSPkg)</_targetOS>
<_hostOS>$(NETCoreSdkPortableRuntimeIdentifier.SubString(0, $(NETCoreSdkPortableRuntimeIdentifier.LastIndexOf('-'))))</_hostOS>
Copy link
Member

Choose a reason for hiding this comment

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

This is an 'internal' variable so I'm not really concerned with naming, but FWIW I don't think of linux-musl as an OS by itself, I think of it as OSxLibc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alpine to linux-musl is same as Android to linux-bionic; which is our existing convention:

runtime/Directory.Build.props

Line 194 in eccbe90

<_portableOS Condition="'$(_runtimeOS)' == 'linux-musl' or $(_runtimeOS.StartsWith('alpine'))">linux-musl</_portableOS>
Each linux distro considers itself as an OS. Though, there is no strict definition where we should draw the line (kernel, kernel+libc or kernel+libc+userland etc.).

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
<!-- Determine _targetArchitecture from RuntimeIdentifier -->
<_targetArchitecture>$(RuntimeIdentifier.SubString($([MSBuild]::Add($(RuntimeIdentifier.LastIndexOf('-')), 1))))</_targetArchitecture>

<RuntimeIlcPackageName>runtime.$(_targetOS)-$(_targetArchitecture).Microsoft.DotNet.ILCompiler</RuntimeIlcPackageName>
<_hostPackageName>runtime.$(_hostOS)-$(_hostArchitecture).Microsoft.DotNet.ILCompiler</_hostPackageName>
Copy link
Member

Choose a reason for hiding this comment

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

On non-Windows, this is just runtime.$(NETCoreSdkPortableRuntimeIdentifier).Microsoft.DotNet.ILCompiler, right?

Ideally, I'd like to avoid string-splitting the RIDs at all, so it would be nice if there were pre-existing variables for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me this is more readable in the context of next line:

_hostPackageName=_hostOS+_hostArchitecture
_targetPackageName=_targetOS+_targetArchitecture

src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets Show resolved Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets
Text="Cross-OS native compilation is not supported." />

<Error Condition="'$(DisableUnsupportedError)' != 'true' and !($(RuntimeIdentifier.EndsWith('x64')) or $(RuntimeIdentifier.EndsWith('arm64')))"
<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(_targetArchitecture)' != 'x64' and '$(_targetArchitecture)' != 'arm64'"
Copy link
Member

Choose a reason for hiding this comment

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

Is this line necessary. In fact, is it even reachable? Or do we fail during package restore since a linux-x86 runtime pack doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need to revisit these error cases / validations at some point, out of scope for this PR though.

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets
Text="Native compilation does not support targeting $(RuntimeIdentifier) yet." />

<Error Condition="'$(DisableUnsupportedError)' != 'true' and !('$(IlcHostArch)' == 'x64' or '$(IlcHostArch)' == 'arm64')"
<Error Condition="'$(DisableUnsupportedError)' != 'true' and !('$(_hostArchitecture)' == 'x64' or '$(_hostArchitecture)' == 'arm64')"
Copy link
Member

Choose a reason for hiding this comment

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

Also not sure what this is accomplishing. It seems like we should be able to run anywhere with a linker and a package definition.

Copy link
Member

Choose a reason for hiding this comment

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

Similar to my complaint above these checks need to be revisited. They predate the .NET SDK integration with the PublishAot property, and many of the checks are either outdated or outright useless. Unfortunately it requires some effort to fix and test all the combinations so I don't think it should block this PR [which just renames the variables within the existing checks].

@agocke
Copy link
Member

agocke commented Feb 27, 2023

LGTM. Continued uncomfortableness at parsing RIDs, which is something we've explicitly said not to do, but I don't see a better option in some of these cases.

@agocke agocke merged commit 3bbf29e into dotnet:main Feb 27, 2023
@agocke agocke mentioned this pull request Feb 28, 2023
Merged
@kant2002 kant2002 mentioned this pull request Mar 21, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@filipnavara filipnavara filipnavara approved these changes

@jkotas jkotas jkotas approved these changes

@agocke agocke agocke approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees
No one assigned
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@am11 @agocke @filipnavara @jkotas