Skip to content
This repository was archived by the owner on Oct 18, 2018. It is now read-only.

Add a package dependency from Microsoft.AspNetCore.{App,All} => NETCore.App #1256

Closed
natemcmaster opened this issue Jul 12, 2018 · 11 comments
Closed
Labels
1 - Ready cost: S Will take up to 2 days to complete investigate Investigation item wontfix
Milestone

Comments

@natemcmaster
Copy link
Contributor

Part of implementing dotnet/aspnetcore#3307 we need to resolve is how to make sure both AspNetCore.App and NETCore.App end up in the restore graph so you get all the right ref assemblies.

@JunTaoLuo looked into this in the past but ran into issues. We should take a closer look at those again to inform our strategy about how we should move forward with dotnet/aspnetcore#3307

@natemcmaster natemcmaster added 1 - Ready cost: S Will take up to 2 days to complete investigate Investigation item labels Jul 12, 2018
@natemcmaster natemcmaster added this to the 2.1.3 milestone Jul 12, 2018
@natemcmaster natemcmaster self-assigned this Jul 12, 2018
@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Jul 12, 2018

I forget the issue we ran into originally, but it may have been resolved. A few things we would want to check are:

  1. The correct set of build/compile targets are brought in correctly
  2. The deps files generated are compatible with MVC features (IIRC, the published deps file for the shared framework was wrong and we either needed to hand edit the file or have dotnet publish change its behaviour, otherwise it will break MVC, cc @pranavkm in case he remembers?)
  3. The generated shared frameworks in our build contains the full set of binaries required for layered frameworks
  4. No restore graphs and publish output regression

@dsplaisted
Copy link

Instead of adding a package dependency, why wouldn't we just have two separate implicit package references?

@natemcmaster
Copy link
Contributor Author

We want to avoid a mismatch of the base runtime and the upper layers. AspNetCore.App 2.1.3, the sharedfx, is built to expect NETCore.App 2.1.3. The package graph (and the versions they bring) should reflect that.

@natemcmaster
Copy link
Contributor Author

natemcmaster commented Jul 13, 2018

It looks like there may be one side effect. The .deps.json generation produces a slightly difference result when NETCore.App is transitive.

When you have NETCore.App and AspNetCore.App as direct deps, you get.

      "Microsoft.NETCore.App/2.1.0": {
        "dependencies": {
          "Microsoft.NETCore.DotNetHostPolicy": "2.1.0",
          "Microsoft.NETCore.Platforms": "2.1.0",
          "Microsoft.NETCore.Targets": "2.1.0",
          "NETStandard.Library": "2.0.3"
        },
        "compile": {
          [BUNCH OF ENTRIES FOR ref/netcoreapp2.1/*.dll HERE]
        }
      },

But with only AspNetCore.App as the top-level dep and NETCore.App is transitive, you get,

      "Microsoft.NETCore.App/2.1.3-servicing-26708-02": {
        "dependencies": {
          "Microsoft.NETCore.DotNetHostPolicy": "2.1.3-servicing-26708-02",
          "Microsoft.NETCore.Platforms": "2.1.1-servicing-26708-02",
          "Microsoft.NETCore.Targets": "2.0.0",
          "NETStandard.Library": "2.0.3"
        },
        "compile": {
          [BUNCH OF ENTRIES FOR ref/netcoreapp2.1/*.dll HERE]
        }
+       "compileOnly": true
      },

@dsplaisted is this an SDK bug? @pranavkm would this break MVC/Razor compilation?

@natemcmaster
Copy link
Contributor Author

Otherwise, build output and publish output looks the same.

@pranavkm
Copy link
Contributor

MVC doesn't specifically look at the CompileOnly option from the deps file, so presumably not

@natemcmaster
Copy link
Contributor Author

Ok, then I think we would be okay to make this change. I tested that build-time and runtime compliation still work.

@natemcmaster
Copy link
Contributor Author

This is being punted for now. Removing from the 2.1 milestone

@natemcmaster
Copy link
Contributor Author

At this point, there is no plan to change this in 2.1. @dsplaisted do you think we should address this in 2.2?

@dsplaisted
Copy link

My opinion is still that it would probably be better to address this in a different way (ie via two implicit references instead of a package dependency). It's possible that I don't understand everything about this though. From this issue, it's not clear to me if there's even a concrete problem that would be solved by adding the dependency, or by having multiple implicit references.

@natemcmaster
Copy link
Contributor Author

Cool, two implicit references it is. That just reduces the differences between 2.1 and 2.2, and so far this hasn't been an issue.

Closing as wontfix since we don't have a compelling reason to do this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 - Ready cost: S Will take up to 2 days to complete investigate Investigation item wontfix
Projects
None yet
Development

No branches or pull requests

4 participants