Skip to content

Fix crossgen2 build on Tizen #55789

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

Merged
ViktorHofer merged 1 commit into from
Jul 30, 2021
Merged

Fix crossgen2 build on Tizen #55789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@

<_runtimeOS Condition="$(_runtimeOS.StartsWith('tizen'))">linux</_runtimeOS>
<_runtimeOS Condition="'$(PortableBuild)' == 'true'">$(_portableOS)</_runtimeOS>

<_packageOS Condition="'$(CrossBuild)' == 'true'">$(_hostOS.ToLowerInvariant)</_packageOS>
Copy link
Member

@jkotas jkotas Jul 21, 2021

Choose a reason for hiding this comment

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

What is the difference between portableOS, runtimeOS and packageOS? I think it can use some comments.

Copy link
Member Author

@am11 am11 Jul 21, 2021

Choose a reason for hiding this comment

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

These are the intermediate private properties. I think we should document the four public properties (which are in PascalCasing, without underscore), and also explain the process of calculation that is inherently complex.

<_packageOS Condition="'$(_packageOS)' == '' and '$(PortableBuild)' == 'true'">$(_portableOS)</_packageOS>
<_packageOS Condition="'$(_packageOS)' == ''">$(_runtimeOS)</_packageOS>
</PropertyGroup>

<PropertyGroup Label="CalculateRID">
Expand All @@ -162,10 +166,11 @@
<MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(PortableBuild)' != 'true' and '$(_portableOS)' == 'linux'">linux-$(_hostArch)</MicrosoftNetCoreIlasmPackageRuntimeId>
<MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(_toolRuntimeRID)</MicrosoftNetCoreIlasmPackageRuntimeId>

<_packageRID Condition="'$(PortableBuild)' == 'true'">$(_portableOS)-$(TargetArchitecture)</_packageRID>
<_packageRID Condition="'$(CrossBuild)' == 'true'">$(_hostOS.ToLowerInvariant)-$(TargetArchitecture)</_packageRID>
<PackageRID Condition="'$(PackageRID)' == ''">$(_packageRID)</PackageRID>
<PackageRID Condition="'$(PackageRID)' == ''">$(_runtimeOS)-$(TargetArchitecture)</PackageRID>
<PackageRID>$(_packageOS)-$(TargetArchitecture)</PackageRID>

<!-- Crossgen2 does not support armel, so we will use arm instead. -->
Copy link
Member

@jkotas jkotas Jul 21, 2021

Choose a reason for hiding this comment

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

I believe that crossgen2 is able to produce armel binaries, no? What does it mean that crossgen2 does not support armel?

Copy link
Member

@jkotas jkotas Jul 21, 2021

Choose a reason for hiding this comment

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

If it is some sort of crossgen2 specific hack, I think it would be better to have it in crossgen2.csproj like before this change.

Copy link
Member Author

@am11 am11 Jul 21, 2021

Choose a reason for hiding this comment

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

crossgen2 just does not have package name with armel, it uses arm architecture name for both soft and hard fp.

Copy link
Member

@ViktorHofer ViktorHofer Jul 22, 2021

Choose a reason for hiding this comment

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

Do you know what the reason for that is? Is it just to minimize build/package targets?

<Crossgen2PackageRID Condition="'$(TargetArchitecture)' == 'armel'">$(_packageOS)-arm</Crossgen2PackageRID>
<Crossgen2PackageRID Condition="'$(TargetArchitecture)' != 'armel'">$(PackageRID)</Crossgen2PackageRID>
Comment on lines +172 to +173
Copy link
Member

@ViktorHofer ViktorHofer Jul 22, 2021

Choose a reason for hiding this comment

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

Are we using that property anywhere else than in crossgen2.csproj? If not, would it make sense to move this code over there?

Copy link
Member Author

@am11 am11 Jul 22, 2021

Choose a reason for hiding this comment

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

The idea was to gather all kinds of RID resolving code in one place and avoid any one-off exception. Otherwise it starts with one exception and then goes haywire. 😁

MicrosoftNetCoreIlasmPackageRuntimeId is also similar RID which we have defined above.


<OutputRid Condition="'$(OutputRid)' == ''">$(PackageRID)</OutputRid>
<OutputRid Condition="'$(PortableBuild)' == 'true'">$(_portableOS)-$(TargetArchitecture)</OutputRid>
Expand Down
6 changes: 1 addition & 5 deletions src/coreclr/tools/aot/crossgen2/crossgen2.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@
<OutputPath>$(RuntimeBinDir)/crossgen2</OutputPath>
<!-- The default value for macOS is false -->
<UseAppHost>true</UseAppHost>

<TargetArchitectureAppHost>$(TargetArchitecture)</TargetArchitectureAppHost>
<TargetArchitectureAppHost Condition="'$(TargetArchitectureAppHost)'=='armel'">arm</TargetArchitectureAppHost>

<AppHostRuntimeIdentifier>$(PackageRID)</AppHostRuntimeIdentifier>
<AppHostRuntimeIdentifier>$(Crossgen2PackageRID)</AppHostRuntimeIdentifier>
</PropertyGroup>
<Import Project="crossgen2.props" />
</Project>