Don't add target rid to NetCoreRuntimePackRids

[tmds]

Attempt to fix #1202.

I haven't tested this because ./build.sh on release/3.0 is failing for me: #1230.

CC @crummel @dagood @omajid @dsplaisted @nguerrera

[tmds]

I got this built now.
After the build, there is a dotnet root at ./source-build/bin/src/core-sdk.8e7ef240a585805f97abd2c15a39816d00ea2ffd/artifacts/bin/redist/Release.
It works to restore and publish for fedora.30-x64.

BundledVersions.props has this:

    <KnownFrameworkReference Include="Microsoft.NETCore.App"
                              TargetFramework="netcoreapp3.0"
                              RuntimeFrameworkName="Microsoft.NETCore.App"
                              DefaultRuntimeFrameworkVersion="3.0.0-preview9-19423-09"
                              LatestRuntimeFrameworkVersion="3.0.0-preview9-19423-09"
                              TargetingPackName="Microsoft.NETCore.App.Ref"
                              TargetingPackVersion="3.0.0-preview9-19423-09"
                              RuntimePackNamePatterns="Microsoft.NETCore.App.Runtime.**RID**"
                              RuntimePackRuntimeIdentifiers="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"
                              IsTrimmable="true"
                              />

    <KnownAppHostPack Include="Microsoft.NETCore.App"
                      TargetFramework="netcoreapp3.0"
                      AppHostPackNamePattern="Microsoft.NETCore.App.Host.**RID**"
                      AppHostPackVersion="3.0.0-preview9-19423-09"
                      AppHostRuntimeIdentifiers="fedora.30-x64;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"
                      />

AppHostRuntimeIdentifiers -> includes fedora.30-x64
RuntimePackRuntimeIdentifiers -> does not include fedora.30-x64

[dagood]

Makes sense to me.

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.

[tmds]

@dagood can you merge this?

[dagood]

I'd like to wait for @crummel and @dsplaisted or @nguerrera to make sure the patch makes sense to them, and I don't know the status of the failed CI legs right now (if merging regardless is fine).

[dsplaisted]

I don't have context on this. Normally I would expect to have the available RuntimeIdentifiers for the app host packs and runtime packs to be the same. Is this not the case for source build?

[nguerrera]

My understanding is that at this time:

The apphost pack can be included in /packs/ so it can be built by source build and have the specific RID of source build.

The runtime pack is always pulled from nuget and so it is limited to what Microsoft publishes there.

I think there's some potential confusion here, but I don't think this change is wrong or anything.

[nguerrera]

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.

[dagood]

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.

[nguerrera]

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.

[dsplaisted]

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.

[tmds]

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.

[tmds]

@dagood @crummel are we taking this for 3.0?

[crummel]

Yes, I think we can take this and go for a more complete fix in 3.1 like Nick says. I kicked off the CI again, I think it should pass.

[crummel]

Thanks for the fix @tmds!

[omajid]

Hey @crummel is there a fix included in 3.1? Is anyone tracking that?

[crummel]

core-sdk changed to use a hard-coded list here and we're actually doing the opposite now. We should be making a PR to core-sdk to fix #1444 instead, I think.