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 additional metadata to deps.json to support "framework wins over app" #1847

Closed
steveharter opened this issue Jan 8, 2018 · 25 comments
Closed
Assignees
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Jan 8, 2018

This is the SDK work to support the core-setup feature https://github.com/dotnet/core-setup/issues/3546

That core-setup issue adds new metadata to the framework's deps.json to help determine which assemblies are newer during file-based probing. This would need to be done by the SDK during build and\or publish. That metadata includes assembly version and file version, where assembly version is compared first and if equal, then file version is compared.

@nguerrera

@livarcocc livarcocc added this to the 2.2.0 milestone Jan 25, 2018
@nguerrera
Copy link
Contributor

cc @ericstj @dsplaisted @eerhardt

I'm not sure we need any changes to conflict resolution as it already assumes greatest version wins. Or am I misunderstanding something?

@steveharter
Copy link
Member Author

steveharter commented Jan 26, 2018

@nguerrera I updated the above description to include additional information.

The host is responsible for determining the full path of each assembly, and passing that to the CLR, so it needs to gather that information. Since the app and each framework can contain a copy of a given assembly (and servicing location and the store, but that's not important here), some sort of conflict resolution needs to occur.

Determining which copy of a given assembly wins is a two-step process:

  1. Select all the framework(s) based on roll-forward, etc
  2. Based on the app's and each framework's deps.json file, go through each assembly and probe each framework\app directory to determine if the assembly exists. If so, use that unless the assembly exists in more than one location\framework, then apply rules to help determine which one wins. For 2.0, the higher layer wins (app is highest layer) and no version information is considered.

@steveharter steveharter changed the title Add additional metadata to support "framework wins over app" with package conflict Add additional metadata to deps.json to support "framework wins over app" Jan 26, 2018
@nguerrera
Copy link
Contributor

@steveharter Thanks for the clarification.

I believe that once the deps.json schema is decided, the managed DepedencyModel API implemented in core-setup would need to be updated accordingly, and lastly the SDK would use DependencyModel appropriately. Is there a bug tracking the DependencyModel change?

@nguerrera
Copy link
Contributor

Or perhaps the part where we'd write this is already dynamic/extensible enough to not require an API change?

@eerhardt
Copy link
Member

Is there a bug tracking the DependencyModel change?

I'd think we can use the same core-setup feature bug - https://github.com/dotnet/core-setup/issues/3546. No need to open a new one, IMO.

Or perhaps the part where we'd write this is already dynamic/extensible enough to not require an API change?

No, I think it would require an API change. The DependencyModel isn't dynamic at all - there are properties for each entry in the .json file.

@steveharter
Copy link
Member Author

@nguerrera the core-setup work on DependencyModel is now in, so this issue is now unblocked and the work to add the assemblyVersion and fileVersion can now occur during build\publish

@livarcocc
Copy link
Contributor

@dsplaisted this is the bug I talked to you about. Please give it priority.

@dsplaisted
Copy link
Member

@steveharter What are the new APIs in DependencyModel to take advantage of here, and what version of the DependencyModel package do they show up in?

@eerhardt
Copy link
Member

@dsplaisted - see the new APIs added in dotnet/core-setup#3704.

They show up in any of the past 2 weeks builds of 2.1.0-preview2- or higher. For example, they are in 2.1.0-preview2-26306-03.

@nguerrera
Copy link
Contributor

Is the plan still to write this only on publish in order to not sacrifice inner loop perf?

@steveharter
Copy link
Member Author

I would try writing during build and measure the perf first. If not acceptable, then only publish-time, along perhaps with switch to disable roll-forward for F5 runtimeconfig as previously discussed.

@nguerrera
Copy link
Contributor

The perf is going to be bad. I'm 90% certain of that. By all means measure but we've sweat blood for inner loop perf and I'm not giving anything back for this.

@nguerrera
Copy link
Contributor

nguerrera commented Mar 22, 2018

Is there a correctness issue for leaving it out or is it just an optimization to have the info? In my opinion, if minor roll forward has taken place without this info then runtime needs to check assembly ver itself. Don't we need that for 2.0 apps that have already been published anyway? If we have that, I'm not convinced we need to disable roll forward on F5.

@steveharter
Copy link
Member Author

The concern with not writing the information during build is that build vs. publish may work differently during roll-forward.

We don't crack open any assemblies at run time to check assembly or file version due to technical reasons.

A 2.0 app won't have the metadata and those assemblies will be considered "older".

Some thoughts to address the potential inconsistency:

  1. Do nothing; testing should occur on published bits. Normally roll-forward should not happen during F5 anyway.
  2. Disable roll-forward for F5 (set existing knob in runtimeconfig.dev.json)
  3. Error if there is a roll-forward with conflicting assemblies and without any metadata in the app's deps.json (the framework's deps.json should have it). We only want to error in the F5 (developer) case, so we may need a new knob\flag in the runtimeconfig.dev.json to turn that behavior on.

@nguerrera
Copy link
Contributor

We don't crack open any assemblies at run time to check assembly or file version due to technical reasons.

Care to elaborate on the technical reasons? If this were only on minor roll-forward and only in absence of this data, it seems it would be strictly more correct, and not a performance issue.

A 2.0 app won't have the metadata and those assemblies will be considered "older".

So does that mean that an app published with 2.0 SDK might behave incorrectly with respect to this issue. I know, in general, there is not a 100% guarantee for old apps, but this particular hole seems pluggable to me.

We're currently rolling roslyn forward to 2.1? Are we just lucky that the current version of Roslyn works based on the particular assemblies it has?

@steveharter
Copy link
Member Author

Technical reason is that we have to crack open assembly from native code, and in order to do that initialize the CLR with certain values, all some COM apis to get the information, and then re-initialize the CLR again when calling it "for real" to execute the app. However, re-initializing the CLR is currently not supported.

@steveharter
Copy link
Member Author

@dsplaisted is the link that @eerhardt provided enough information on the new dependencymodel APIs?

@dsplaisted
Copy link
Member

@dsplaisted is the link that @eerhardt provided enough information on the new dependencymodel APIs?

It looks like yes, so far. I may have more questions as I get further into this.

@dsplaisted
Copy link
Member

@eerhardt @steveharter How should this interact with the runtime store feature (I think it's called the "Shared Store" here)?

If I create an MVC app targeting .NET Core 2.0 and build it with my WIP changes to the SDK, I get assemblyVersion and fileVersion metadata in the deps.json for a bunch of files that aren't in the publish folder, rather they are in the runtime store.

@steveharter
Copy link
Member Author

That sounds right. A 2.0 MVC app references all files used by ASP.NET in its deps.json.

A 2.1 MVC app will target the new ASP.NET shared framework instead so it doesn't carry that information any more in its deps.json, except for the "compilation" information (not "runtime").

Note that a MVC app targeting 2.0 won't be able to roll-forward to 2.1 because 2.1 ASP.NET does not install anything to the store.

@dsplaisted
Copy link
Member

Note that a MVC app targeting 2.0 won't be able to roll-forward to 2.1 because 2.1 ASP.NET does not install anything to the store.

@steveharter Is it correct that if the 2.0 runtime store was installed it would be able to run on 2.1, but this is not likely because the 2.0 runtime store is not likely to be installed without the 2.0 runtime?

Are there examples of libraries that have been moved from packages into the framework in .NET Core 2.1? IE something we can use to do end-to-end testing to make sure this feature works.

@steveharter
Copy link
Member Author

Is it correct that if the 2.0 runtime store was installed it would be able to run on 2.1, but this is not likely because the 2.0 runtime store is not likely to be installed without the 2.0 runtime?

Yes the store and runtime are installed together in 2.0. I suppose you can manually delete (or rename so they won't be found) the Program Files\dotnet\shared\Microsoft.NETCore.App\2.0.x folders)

Are there examples of libraries...
I'm not aware of any current issues. There used to be issues, but we fixed them up internally.

For testing, I would suggest adding a copy of a framework assembly to the app, and then change the version numbers manually to ensure it picks the newest one. Set COREHOST_TRACE=1 and parse the output for that file in order to determine which one it picked and\or look at the assemblies that are loaded at runtime via Visual Studio or tools such as Process Monitor.

Note I did write a test, but it is commented out otherwise it would currently fail. If you build core-setup with your changes and uncomment this test it should pass: https://github.com/dotnet/core-setup/blob/master/src/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs#L899

@dsplaisted
Copy link
Member

@steveharter @eerhardt Are there APIs on DependencyContext for reading the new version values? It looks like there aren't.

(This isn't critical for me, I was just trying to use them to validate the deps.json was correct in tests, so I can just read the JSON directly)

@steveharter
Copy link
Member Author

@dsplaisted
Copy link
Member

@steveharter Eric resolved that with me over IM, sorry for not updating here. My issue was the tests were still using the older version of DependencyModel so those APIs weren't accessible 😏

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

No branches or pull requests

5 participants