Fix crossgen2 build on Tizen #55789
Fix crossgen2 build on Tizen #55789
Conversation
ghost
added
the
|
@am11 Thank you so much!!! :) |
clamp03
left a comment
clamp03
left a comment
There was a problem hiding this comment.
I checked that it works well for armel build
terrajobst
added
the
community-contribution
|
@jkotas Could you review this PR? It fixes Tizen armel build fail. Thank you. |
| <_runtimeOS Condition="$(_runtimeOS.StartsWith('tizen'))">linux</_runtimeOS> | ||
| <_runtimeOS Condition="'$(PortableBuild)' == 'true'">$(_portableOS)</_runtimeOS> | ||
|
|
||
| <_packageOS Condition="'$(CrossBuild)' == 'true'">$(_hostOS.ToLowerInvariant)</_packageOS> |
There was a problem hiding this comment.
What is the difference between portableOS, runtimeOS and packageOS? I think it can use some comments.
There was a problem hiding this comment.
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.
| <PackageRID Condition="'$(PackageRID)' == ''">$(_runtimeOS)-$(TargetArchitecture)</PackageRID> | ||
| <PackageRID>$(_packageOS)-$(TargetArchitecture)</PackageRID> | ||
|
|
||
| <!-- Crossgen2 does not support armel, so we will use arm instead. --> |
There was a problem hiding this comment.
I believe that crossgen2 is able to produce armel binaries, no? What does it mean that crossgen2 does not support armel?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
crossgen2 just does not have package name with armel, it uses arm architecture name for both soft and hard fp.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Are we using that property anywhere else than in crossgen2.csproj? If not, would it make sense to move this code over there?
There was a problem hiding this comment.
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.
ViktorHofer
left a comment
ViktorHofer
left a comment
There was a problem hiding this comment.
Aside from a few questions LGTM. Thanks a lot @am11
|
@ViktorHofer Can it be merged? ARMEL BUILD is broken for over 20 days. I want to proceed ARMEL Build CI (#56281) after ARMEL build works well. Thank you. |
|
@am11 is there anything else left here to do? Otherwise after re-triggering CI this should be good to merge. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run dotnet-linker-tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-dev-innerloop |
|
/azp run runtime-staging |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@ViktorHofer, it is ready from my point of view. The goal is to have a flat list of |
|
Thanks a lot @am11 |
5 participants
Fixes the Tizen/armel regression from 566b53a; we had a special case for armel crossgen2 build which was missed.