Trimming PackageReferences about csprojtovs2017

[hvanbakel]

@khellang I'm just trying to say, keep this as a transformation tool and have a separate tool do this package simplification. There's enough complexity and risk in there to split it out.

As to things breaking, I'd be very interested to see those cases if you can simplify them. Like you said, we should try to make it as good as possible :).

[emgarten]

The easiest way would probably be to:

1. Restore the project with everything top level
2. Read project.assets.json with LockFileFormat in https://www.nuget.org/packages/NuGet.ProjectModel/
3. From the target graph (with no RID if there RIDs), find all libraries that are not depended on by other libraries.
4. Then determine which child packages actually need to be top level and keep those also. Example: A -> B >= 1.0.0 will bring in B 1.0.0 by default (lowest possible), but if the user had B 2.0.0 in packages.config then B 2.0.0 should be top level so that the dependencies stay the same.

You can take a look at a packages.config -> PackageReference migrator the nuget team is working on, the logic for trimming is around this area: https://github.com/NuGet/NuGet.Client/blob/86bca57154953ebee960af2a232866e2a0e44594/src/NuGet.Clients/NuGet.PackageManagement.UI/Actions/PackagesConfigToPackageReferenceMigrator.cs#L140

Note that migrator doesn't convert between project types like this one.

[mungojam]

One thing to be careful of when trimming is not to trim too much.

Let's say in your package project is A and A -> B -> json.net. So internally B depends on json.net, but doesn't expose anything from it. In A you also depend on json.net for some other reason. If you only pull in B, and trim json.net, then any project just pulling in A will work today but you run the risk of a runtime exception at a later date when B gets updated and decides to remove the json.net dependency and the top level project manually installs this new B.

Bit of a mess that I've tried to explain in this issue and this comment stream

[hvanbakel]

@mungojam that was the entire reason I didn't put it in in the first place. It's not worth the risk of breaking a project, I believe this is better as a separate tool as it doesn't really have anything to do with the transition of project files.
This is an operation that you should be able to run over and over again.

[mungojam]

Thanks, that makes a lot of sense to me. I've been thinking about how to tackle it for our codebase and it isn't an easy problem. I'm hoping that I can make use of nuget or roslyn libraries to do some magic around detecting packages that should be left in.

[khellang]

Trimming JSON.NET is exactly what I want. That's what makes transitive dependencies so nice, and the projects clean. If B decides to remove their dependency on JSON.NET and nothing else uses it, how will that lead to runtime errors? The package will just disappear. If you actually depend on JSON.NET, you'll get compile-time errors because you suddenly lost the (transitive) reference to it.

[khellang]

Anyway, I wrote up the code after @emgarten's awesome pointers yesterday. Thanks! Works pretty nice for the projects I needed it for. I guess people can wait for MS to publish their bits instead.

[mungojam]

The scenario that gives runtime exceptions is a little contrived but it does happen, I reproduced it on the issue.

If you're pulling in Json.net transitively through B to A and using it in A, then you are referencing both A and B in your project. When you then update B in your project, everything will happily compile because only A is using Json.net but the compiler doesn't know that it is (A is already compiled). You only find out at runtime when you call the function in A that can't find json.net DLL.

[khellang]

BTW

It's not worth the risk of breaking a project

This tool already does that in several cases. Specifically around duplicate entries with explicit and implicit Compile elements. That's just an inherit risk when upgrading using a tool. That doesn't mean we shouldn't try to make it as good as possible.

it doesn't really have anything to do with the transition of project files.

IMO it has a lot to do with the transition. PackageReference is one of the most prominent features of SDK-based projects and one of the most compelling reasons to upgrade.

This is an operation that you should be able to run over and over again.

I don't understand why that's a problem? The console app I wrote up yesterday is totally idempotent. I can run it over and over and it will always produce the same package closure, as long as the project doesn't change.

[khellang]

As to things breaking, I'd be very interested to see those cases if you can simplify them.

They were quick to fix and definitely within the threshold of issues I'd expect when migrating complex projects using a tool. I'll see if I can reproduce some of them (I could probably time travel back and re-run the upgrade) and get back to you...