Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix runtime package build for -allconfigurations build on Linux #34346

Closed
wants to merge 2 commits into from

Conversation

dagood
Copy link
Member

@dagood dagood commented Jan 3, 2019

I hit a few issues trying to get -allconfigurations --pack /p:SkipBuildRuntimePackage=false /p:PackageRID=linux-x64 able to build on Linux (dotnet/source-build#210) where the build tries to create some runtime packages that can't be built or don't make sense. More details in the commit messages.

This might be exploring new ground, since the official build builds runtime packages on separate legs than the -allconfigurations leg, but in source-build we need both -allconfigurations and runtime packages on the same machine. It seems to behave reasonably with these changes, though.

dagood added 2 commits January 3, 2019 17:05
Without this change, setting PackageRID to linux-x64 matches linux-x64-aot because the "-aot" is not considered. This creates confusing results later: the bad match causes the build to attempt to build a runtime package that includes native assets from 'bin/native/netcoreapp-Linux-Release-x64-aot', but the build calls it the "linux-x64" runtime package.

Including the runtime suffix ('-aot') when matching up package targets with PackageRID prevents that bad match.
Setting this property to 'true' continues current default behavior, where a PackageRID runtime package is always created even if it's not an officially supported RID. ("Unofficial RIDs".) This is the case with Microsoft.Private.CoreFx.NETCoreApp, which is expected to always be an output of this repo on any successful build.

The reason for this change is to disable the UAP runtime package on unofficial RIDs. The UAP project doesn't build natives on arbitrary Linux distros, so to build allconfigurations on Linux, we have to disable this behavior for the UAP runtime package.

If there are actually unofficial RIDs that need to build a UAP runtime package, this property will need to be reworked.
@dagood dagood requested a review from bartonjs January 3, 2019 23:38
@@ -17,7 +17,7 @@
We'll add a RID for the current platform even if it isn't in the officially supported set -->
<ItemGroup Condition="'@(OfficialBuildRID)' != ''">
<BuildRID Include="@(OfficialBuildRID)" Exclude="$(PackageRID)"/>
<BuildRID Include="$(PackageRID)">
<BuildRID Include="$(PackageRID)" Condition="'$(BuildsEvenIfRIDNotOfficiallySupported)' == 'true'">
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 think the win10 uapaot leg fails because I didn't account for the Exclude="$(PackageRID)" on the first item. This second item isn't only for unofficial RIDs, it also adds official RIDs back in if the Exclude got rid of it. Looking into 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.

That might be a problem, but the logic in pkg/dir.traversal.targets looks like a bigger problem:

%(Project.PackageTargetRuntime)$(PackageTargetRuntimeSuffixWithDash)' == '$(PackageRID)'

Since PackageTargetRuntimeSuffixWithDash is (I think) set based on the current configuration, rather than Project's target, this makes the win10-x64-aot configuration/PackageRID match up with the win10-x64 Project and not the win10-x64-aot Project. I believe this causes a mismatch resulting in the errors this change is hitting:

pkg\frameworkPackage.targets(61,5): error : IncludeLibFiles was specified but no file props were found in 'C:\git\source-build\src\corefx\artifacts\bin\pkg\uap\lib'

My scenario on Linux only happened to work because I never wanted an aot RID. Working on a better fix.

@dagood
Copy link
Member Author

dagood commented Jan 4, 2019

Closing this PR: there's no bug here, just me misunderstanding how -allconfigurations and PackageRID were interacting.

(I have a much smaller change to do what I need here that I'll submit in a new PR. Unlike this PR, the new change is a clear no-op unless specifically opted into.)

@dagood dagood closed this Jan 4, 2019
@dagood dagood deleted the fix-linux-allconfig-pkg branch January 4, 2019 21:52
@karelz karelz added this to the 3.0 milestone Mar 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants