Skip to content

Don't add target rid to NetCoreRuntimePackRids #1231

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
merged 1 commit into from
Sep 24, 2019
Merged

Don't add target rid to NetCoreRuntimePackRids #1231

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
41 changes: 41 additions & 0 deletions patches/core-sdk/0003-Don-t-add-target-rid-to-NetCoreRuntimePackRids.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
From 50a32f2e8708fd838a29ab3133e3f8e75d7bb942 Mon Sep 17 00:00:00 2001
From: Tom Deseyn <[email protected]>
Date: Tue, 17 Sep 2019 08:29:21 +0200
Subject: [PATCH] Don't add target rid to NetCoreRuntimePackRids

---
src/redist/targets/GenerateBundledVersions.targets | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/redist/targets/GenerateBundledVersions.targets b/src/redist/targets/GenerateBundledVersions.targets
index b440f78e1..3f725a792 100644
--- a/src/redist/targets/GenerateBundledVersions.targets
+++ b/src/redist/targets/GenerateBundledVersions.targets
@@ -62,13 +62,14 @@
</MSBuild>

<GetRuntimePackRids MetapackagePath="$(NuGetPackageRoot)/microsoft.netcore.app.internal/$(MicrosoftNETCoreAppPackageVersion)">
- <Output TaskParameter="AvailableRuntimePackRuntimeIdentifiers" ItemName="NetCoreRuntimePackRids" />
+ <Output TaskParameter="AvailableRuntimePackRuntimeIdentifiers" ItemName="NetCoreAppHostPackRids" />
</GetRuntimePackRids>
<GetRuntimePackRids MetapackagePath="$(NuGetPackageRoot)/microsoft.windowsdesktop.app/$(MicrosoftWindowsDesktopPackageVersion)">
<Output TaskParameter="AvailableRuntimePackRuntimeIdentifiers" ItemName="WindowsDesktopRuntimePackRids" />
</GetRuntimePackRids>

<ItemGroup>
+ <NetCoreRuntimePackRids Include="linux-arm;linux-arm64;linux-musl-arm64;linux-musl-x64;linux-x64;osx-x64;rhel.6-x64;tizen.4.0.0-armel;tizen.5.0.0-armel;win-arm;win-arm64;win-x64;win-x86"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if this were authored in a way that can be merged upstream. Are we ok with this hard-coding for all builds or should this be conditioned out. @dsplaisted I notice that AspNet is already hard coded while base and WindowsDesktop are not. Why is that? I notice this is taking dependencies on private packages and I guess this is why ASP.NET is hard-coded, because there is no such package?

I'd love for there to be one approach for all, and any quirks of source build to be handled conditionally.

It may be the case that runtime pack and apphost pack rids are not added often enough to require much sophistication and can just be hard-coded to the microsoft build set, with source build appending its RID to the apphost pack set. I don't know if this complexity we have now is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, from my accept comment:

Will take some extra work to incorporate the patch into the repo later. It'd be more maintainable IMO to have Core-SDK still use microsoft.netcore.app.internal for both sets of RIDs, but remove the current/build RID from the runtime pack RID list if DotNetBuildFromSource is true. Would ensure source-built SDKs can do self-contained publish on new RIDs that come up.

Doesn't seem like it would be that complex to do that, but everyone has dev time budgets.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a time crunch here, I can accept this as the patch for 3.0 if we can work together on a PR to core-sdk 3.1 starting now that does something that will hold up for the long term and not require patching. I'd prefer to be reviewing that PR now while this is fresh in everyone's mind then to be scrambling when "please remove your patches" mail comes at some arbitrary time.

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to get the list of ASP.NET RuntimeIdentifiers from a package that is inserted instead of hardcoding them. There wasn't such a list when we were getting things working, and I think there still isn't.

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 would also prefer an upstream fix.
We need a fix for 3.0. This is breaking two use-cases:

  • restore against the source-build target rid, this fails because the sdk expects to aquire a Microsoft.NETCore.App.Runtime package for that rid from nuget.org.
  • ReadyToRun, this fails even for the linux-x64 rid, I think the sdk assumes crossgen from linux-x64 won't work on a source build target rid system.

PS: I hope that for .NET 5 we can include a Microsoft.NETCore.App.Runtime with the sdk for the source build target rid, see #1215.

<AspNetCoreRuntimePackRids Include="
win-x64;
win-x86;
@@ -187,7 +188,7 @@ Copyright (c) .NET Foundation. All rights reserved.
TargetFramework="netcoreapp3.0"
AppHostPackNamePattern="Microsoft.NETCore.App.Host.**RID**"
AppHostPackVersion="$(_NETCoreAppPackageVersion)"
- AppHostRuntimeIdentifiers="@(NetCoreRuntimePackRids, '%3B')"
+ AppHostRuntimeIdentifiers="@(NetCoreAppHostPackRids, '%3B')"
/>

<KnownFrameworkReference Include="Microsoft.WindowsDesktop.App"
--
2.21.0