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.

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

Add proposal for .NET Core 3 targeting packs and runtime packs #50

Merged
merged 7 commits into from
Jun 8, 2020

Conversation

nguerrera
Copy link
Contributor

No description provided.

accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Copying unresolved comments to this PR from the internal one...

accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
accepted/targeting-packs-and-runtime-packs.md Outdated Show resolved Hide resolved
@natemcmaster
Copy link
Contributor

I've started implementing this proposal for ASP.NET Core. I have some follow-up questions which I think should be addressed in this spec:

  • Where does a targeting pack or runtime pack specify metadata about package conflict resolution?
    • Today, Microsoft.AspNetCore.App specifies this as an MSBuild item using NuGet package MSBuild assets
      build/Microsoft.AspNetCore.App.props:
       <ItemGroup>
       	<PackageConflictOverrides Include="Microsoft.AspNetCore.App">
             <OverriddenPackages>
               Microsoft.AspNetCore.Antiforgery|3.0.0-preview3-19111-04;
               Microsoft.AspNetCore.Authentication.Abstractions|3.0.0-preview3-19111-04;
               Microsoft.AspNetCore.Authentication.Cookies|3.0.0-preview3-19111-04;
       		// .... (etc)
  • Where does a runtime pack specify the platform manifest?
    • Today, this is specified (1) as an MSBuild item using NuGet package MSBuild assets which points to (2) a text file
      build/Microsoft.AspNetCore.App.props:
         <ItemGroup Condition="'$(RuntimeIdentifier)' == '' or '$(SelfContained)' != 'true'">
           <PackageConflictPlatformManifests Include="$(MSBuildThisFileDirectory)Microsoft.AspNetCore.App.PlatformManifest.txt" />
         </ItemGroup>
      Microsoft.AspNetCore.App.PlatformManifest.txt:
       aspnetcorev2_inprocess.dll|Microsoft.AspNetCore.App||13.0.19042.0
       Microsoft.AspNetCore.Antiforgery.dll|Microsoft.AspNetCore.App|3.0.0.0|3.0.0.19042
       Microsoft.AspNetCore.Authentication.Abstractions.dll|Microsoft.AspNetCore.App|3.0.0.0|3.0.0.19042
      
  • What should shared framework authors name the Linux installers (Debian, RPM)? How should these packages version? Should installing a newer runtime pack installers cause older versions of the runtime pack to uninstall? I would expect the targeting pack installers to version differently from the runtime pack installers and the shared framework installers.
  • What should shared framework authors name the Windows installers (MSI, EXE)? Are these targeting packs supposed to be architecture specific (x86/x64)? Same question about versioning - should installing a newer runtime pack installer cause older versions to uninstall?

cc @pakrym

@dsplaisted
Copy link
Member

@nguerrera @natemcmaster The questions about conflict resolution data and the platform manifest made me realize that we need that information to be in both the targeting packs and the runtime packs, and the SDK needs to select the correct source for that information based on whether the app is self-contained or not. I've filed dotnet/sdk#2933

@dsplaisted
Copy link
Member

@natemcmaster We currently don't plan to have runtime pack installers in the same way that we will have targeting pack installers. So runtime packs will always be acquired via PackageDownload, and building a self-contained app won't be supported "offline" (ie without an initial restore which downloads the appropriate runtime pack).

@nguerrera
Copy link
Contributor Author

The package conflict overrides will need to be converted to a data file like the manifest.

Possibly this could be folded into 1 file. To bootstrap, we can extract the props somewhere in the sdk where we can conditionally import it, a bit like bundled versions.

We're already reading the manifest by convention, but looks like we have the (small) bug Daniel mentions for runtime pack case.

I think we can fix that and do the bundled version-esque trick for preview 3.

@nguerrera
Copy link
Contributor Author

For architecture specificity of targeting pack installer, see pr for m.nc.app.ref installer. different approaches were discussed, but ultimately multiple msis seems simplest. We do need x86 and x64 sdks to install tps to their own location with appropriate ref counting. I'm deferring to msi experts to decide how best to achieve that.

I don't remember the name Davis picked offhand and I'm away from a computer to check, but it seemed good to me. Can you follow his lead for that?

@nguerrera
Copy link
Contributor Author

For uninstalling, I think it should follow the same policy as the shared frameworks, which is changing but I think it is currently still 100% side by side so we should match that and just include the tps in any push to change it.

I'm imagining every build will produce a tp, but once we hit rtm, we'll re ship .0 in patches unless there's a critical issue that requires a serviced tp. I alluded to this in the doc but I'll make it more detailed

@nguerrera
Copy link
Contributor Author

Actually, if I understood the plans for upgrading/uninstalling patches/previews correctly, I think it is only a concern for the bundles / higher level packages that can uninstall old things bundled with previous versions and install new things.

@ericstj
Copy link
Member

ericstj commented Jul 18, 2019

Any reason why this hasn't been merged? I believe it is in the product.

@nguerrera
Copy link
Contributor Author

There are some things that are out of date. It's on my list to clean it up.

@jzabroski

This comment has been minimized.

@nguerrera

This comment has been minimized.

@jzabroski
Copy link

I am still looking for a specification. Very difficult to file bugs for something with no official documentation.

@terrajobst
Copy link
Member

@dsplaisted can this be merged? Or should this be closed?

@nguerrera
Copy link
Contributor Author

@terrajobst I'll fix it up. (Yes, I know I've said that forever, but give me a bit more time.)

@nguerrera
Copy link
Contributor Author

This is on an extremely short list of things I still intend to wrap up even though I don’t work on the sdk anymore

@steveisok
Copy link
Member

With the introduction of the mono vm in .net 5, there is a need to uniquely differentiate the runtime packs. To be clear, this would apply to scenarios where there are > 1 vm's being used (Xamarin.Mac targeting OSX).

To me, it would make sense to just add the vm to the name of the runtime pack. Something like Microsoft.NETCore.App.Runtime.{vm}.{rid}.{version}.nukpg.

What's the best way to proceed?

/cc @marek-safar @ericstj

@jzabroski
Copy link

I don't believe that's how nupkg's are supposed to work. You have one nupkg for all runtime targets. Maybe FrameworkReference(s) are handled differently. Unless what you're saying is related to this part of the specification:

Package Versions

Runtime pack package versions will be 1:1 with .NET Core Runtime / shared framework versions. This follows from the fact that runtime packs contain shared framework implementation and thus must change whenever the implementation changes.

And adding Mono changing what it means to be 1:1 with .NET Core Runtimes, as it adds a second runtime?

P.S. I'm not sure what {rid} is here - RuntimeIdentifier?

@nguerrera
Copy link
Contributor Author

Finally had a moment to look at this. I've fixed the inaccuracies related to .NET Core 3's final shape and I'm going to go ahead and merge this now.

@nguerrera
Copy link
Contributor Author

nguerrera commented Jun 8, 2020

Ah, I don't have permission to merge anymore. @terrajobst @dsplaisted, go ahead and merge if you approve.

@terrajobst terrajobst merged commit 966a8e1 into dotnet:master Jun 8, 2020
@terrajobst
Copy link
Member

Done ✅

@nguerrera nguerrera deleted the netcore3-tp branch June 8, 2020 17:42
@jzabroski
Copy link

@nguerrera Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.