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

Revive #48505 #54914

Merged
merged 7 commits into from
Jul 6, 2021
Merged

Revive #48505 #54914

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0fff6f3
Revive #48505
ViktorHofer Jun 29, 2021
01975b9
Update build.proj
ViktorHofer Jun 29, 2021
bbfbb07
Update crossgen2.csproj
ViktorHofer Jun 29, 2021
a3af296
Update src/coreclr/tools/aot/crossgen2/crossgen2.csproj
ViktorHofer Jun 30, 2021
9b11763
Update build.sh
ViktorHofer Jun 30, 2021
9986925
Update src/tests/build.sh
ViktorHofer Jun 30, 2021
7d7e19d
Use OutputRid when creating .nupkgs, instead of PackageRID
am11 Jul 4, 2021
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
53 changes: 17 additions & 36 deletions Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@
</PropertyGroup>

<PropertyGroup Label="CalculateTargetOS">
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('OSX'))">OSX</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('FREEBSD'))">FreeBSD</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('NETBSD'))">NetBSD</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('ILLUMOS'))">illumos</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('SOLARIS'))">Solaris</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSUnixLike())">Linux</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('WINDOWS'))">windows</TargetOS>
<_hostOS>Linux</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('OSX'))">OSX</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('FREEBSD'))">FreeBSD</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('NETBSD'))">NetBSD</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('ILLUMOS'))">illumos</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('SOLARIS'))">Solaris</_hostOS>
<_hostOS Condition="$([MSBuild]::IsOSPlatform('WINDOWS'))">windows</_hostOS>
<_hostOS Condition="'$(TargetOS)' == 'Browser'">Browser</_hostOS>
<TargetOS Condition="'$(TargetOS)' == ''">$(_hostOS)</TargetOS>
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

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

from what I understood _hostOS is the OS where the build runs right? in that case Browser doesn't make sense here (I assume we have some other wrong assumption about this somewhere else so this is needed right now).

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Let's untangle this when we eliminate remaining properties which are conflicting with each other.

Copy link
Member

@am11 am11 Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think in case of browser, value of _hostOS is used in PackageRID below. For other platforms, it is also used to calculate OutputRid (through _portableOS) and MicrosoftNetCoreIlasmPackageRuntimeId when cross-building.

<TargetsMobile Condition="'$(TargetOS)' == 'iOS' or '$(TargetOS)' == 'iOSSimulator' or '$(TargetOS)' == 'MacCatalyst' or '$(TargetOS)' == 'tvOS' or '$(TargetOS)' == 'tvOSSimulator' or '$(TargetOS)' == 'Android' or '$(TargetOS)' == 'Browser'">true</TargetsMobile>
</PropertyGroup>

Expand Down Expand Up @@ -114,16 +116,13 @@
the build system to use browser/ios/android as the _runtimeOS for produced package RIDs. -->
<_runtimeOS Condition="'$(TargetsMobile)' == 'true'">$(TargetOS.ToLowerInvariant())</_runtimeOS>

<_runtimeOSVersionIndex>$(_runtimeOS.IndexOfAny(".-0123456789"))</_runtimeOSVersionIndex>
<_runtimeOSFamily Condition="'$(_runtimeOSVersionIndex)' != '-1'">$(_runtimeOS.SubString(0, $(_runtimeOSVersionIndex)))</_runtimeOSFamily>

<_portableOS>linux</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'linux-musl'">linux-musl</_portableOS>
<_portableOS Condition="$([MSBuild]::IsOSPlatform('OSX'))">osx</_portableOS>
<_portableOS Condition="'$(_runtimeOSFamily)' == 'win' or '$(_runtimeOS)' == 'win' or '$(TargetOS)' == 'windows'">win</_portableOS>
<_portableOS Condition="'$(_runtimeOSFamily)' == 'FreeBSD'">freebsd</_portableOS>
<_portableOS Condition="'$(_runtimeOSFamily)' == 'illumos'">illumos</_portableOS>
<_portableOS Condition="'$(_runtimeOSFamily)' == 'Solaris'">solaris</_portableOS>
<_portableOS Condition="'$(_hostOS)' == 'OSX'">osx</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'win' or '$(TargetOS)' == 'windows'">win</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'FreeBSD' or '$(TargetOS)' == 'FreeBSD'">freebsd</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'illumos' or '$(TargetOS)' == 'illumos'">illumos</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'Solaris' or '$(TargetOS)' == 'Solaris'">solaris</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'Browser'">browser</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'maccatalyst'">maccatalyst</_portableOS>
<_portableOS Condition="'$(_runtimeOS)' == 'ios'">ios</_portableOS>
Expand All @@ -134,16 +133,12 @@

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

<!-- support cross-targeting by choosing a RID to restore when running on a different machine that what we're build for -->
<_portableOS Condition="'$(TargetOS)' == 'Unix' and '$(_runtimeOSFamily)' != 'osx' and '$(_runtimeOSFamily)' != 'FreeBSD' and '$(_runtimeOS)' != 'linux-musl' and '$(_runtimeOSFamily)' != 'illumos' and '$(_runtimeOSFamily)' != 'Solaris'">linux</_portableOS>
</PropertyGroup>

<PropertyGroup Label="CalculateRID">
<_toolRuntimeRID Condition="'$(CrossBuild)' == 'true'">$(_hostOS.ToLowerInvariant)-$(_hostArch)</_toolRuntimeRID>
<_toolRuntimeRID Condition="'$(BuildingInsideVisualStudio)' == 'true'">$(_runtimeOS)-x64</_toolRuntimeRID>
<_toolRuntimeRID Condition="'$(_toolRuntimeRID)' == ''">$(_runtimeOS)-$(_hostArch)</_toolRuntimeRID>
<!-- We build linux-musl-arm on a ubuntu container, so we can't use the toolset build for alpine runtime. We need to use portable linux RID for our toolset in order to be able to use it. -->
<_toolRuntimeRID Condition="'$(_runtimeOS)' == 'linux-musl' and $(TargetArchitecture.StartsWith('arm')) and !$(_hostArch.StartsWith('arm'))">linux-x64</_toolRuntimeRID>

<!-- There are no WebAssembly tools, so use the default ones -->
<_toolRuntimeRID Condition="'$(_runtimeOS)' == 'browser'">linux-x64</_toolRuntimeRID>
Expand All @@ -163,26 +158,12 @@
<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>

<_outputRID Condition="'$(TargetOS)' == 'windows'">win-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'OSX'">osx-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'Linux'">linux-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'FreeBSD'">freebsd-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'NetBSD'">netbsd-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'illumos'">illumos-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'Solaris'">solaris-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'MacCatalyst'">maccatalyst-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'iOS'">ios-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'iOSSimulator'">iossimulator-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'tvOS'">tvos-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'tvOSSimulator'">tvossimulator-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'Android'">android-$(TargetArchitecture)</_outputRID>
<_outputRID Condition="'$(TargetOS)' == 'Browser'">browser-$(TargetArchitecture)</_outputRID>

<OutputRid Condition="'$(OutputRid)' == ''">$(PackageRID)</OutputRid>
<OutputRid Condition="'$(PortableBuild)' == 'true'">$(_outputRID)</OutputRid>
<OutputRid Condition="'$(PortableBuild)' == 'true'">$(_portableOS)-$(TargetArchitecture)</OutputRid>
</PropertyGroup>

<PropertyGroup Label="CalculateTargetOSName" Condition="'$(SkipInferTargetOSName)' != 'true'">
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/crossgen2/crossgen2.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<TargetArchitectureAppHost>$(TargetArchitecture)</TargetArchitectureAppHost>
<TargetArchitectureAppHost Condition="'$(TargetArchitectureAppHost)'=='armel'">arm</TargetArchitectureAppHost>

<AppHostRuntimeIdentifier>$(_runtimeOS)-$(TargetArchitectureAppHost)</AppHostRuntimeIdentifier>
<AppHostRuntimeIdentifier>$(PackageRID)</AppHostRuntimeIdentifier>
Comment on lines 8 to +10
Copy link
Member

Choose a reason for hiding this comment

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

ARMEL build is broken because target architecture is not updated to arm for crossgen2.
Please fix it. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@clamp03 can you please open a new issue for that? @am11 do you have time to look at the regression?

Copy link
Member

@am11 am11 Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

armel is another community supported platform, which gets broken time to time. For community related things to sustain in the continuous development (for the busy repo like runtime), there should be a CI leg IMO (as we added one last year for GCC build validation). I have the next batch of changes in progress, and will fix it in in that batch soon. Meanwhile, you can apply a local patch and get by it (and please consider adding CI leg as it won't be the last armel breakage otherwise).

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @jkotas @safern regarding the armel CI protection discussion above.

Copy link
Member

Choose a reason for hiding this comment

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

Armel fix is pushed in 55279: https://github.com/dotnet/runtime/pull/55279/files#diff-9da24614831c308827a1ae533ffea392c97638c261dd42bd0f5226baa136d16eR137-R169.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine to add an armel CI build that just does a traversal build. Maybe in the dev-innerloop pipeline?

Copy link
Member

Choose a reason for hiding this comment

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

Adding armel + Tizen CI build leg is tracked by #2394

As @safern said, we are open to adding CI build legs to protect community supported configurations. We expect the community to set them up. The steps for how to do that are in #2394 (comment)

</PropertyGroup>
<Import Project="crossgen2.props" />
</Project>
2 changes: 1 addition & 1 deletion src/tests/build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@

<Target Name="RestorePackage">
<PropertyGroup>
<_ConfigurationProperties>/p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:Configuration=$(Configuration)</_ConfigurationProperties>
<_ConfigurationProperties>/p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:Configuration=$(Configuration) /p:CrossBuild=$(CrossBuild)</_ConfigurationProperties>
<DotnetRestoreCommand Condition="'$(__DistroRid)' == ''">"$(DotNetTool)" restore $(RestoreProj) $(PackageVersionArg) /p:SetTFMForRestore=true $(_ConfigurationProperties)</DotnetRestoreCommand>
<DotnetRestoreCommand Condition="'$(__DistroRid)' != ''">"$(DotNetTool)" restore -r $(__DistroRid) $(RestoreProj) $(PackageVersionArg) /p:SetTFMForRestore=true $(_ConfigurationProperties)</DotnetRestoreCommand>
</PropertyGroup>
Expand Down
4 changes: 4 additions & 0 deletions src/tests/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,10 @@ if [[ "${__BuildArch}" != "${__HostArch}" ]]; then
__CrossBuild=1
fi

if [[ "$__CrossBuild" == 1 ]]; then
__UnprocessedBuildArgs+=("/p:CrossBuild=true")
+fi
ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved

# Set dependent variables
__LogsDir="$__RootBinDir/log"
__MsbuildDebugLogsDir="$__LogsDir/MsbuildDebugLogs"
Expand Down