search

NuKeeperDotNet / NuKeeper

Automagically update nuget packages in .NET projects
Apache License 2.0
541 stars 127 forks source link

Sort by Project References to avoid project ordering dependency issues. #1109

Open arikalish opened 3 years ago

arikalish commented 3 years ago

:sparkles: What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix

:arrow_heading_down: What is the current behavior?

Packages to update are sorted by the packages they reference but ignores project dependency ordering, leading to issues when projects at the root of the dependency graph are updated before those that depend on them that are more than one level in the tree apart from one another. In particular, this leads to ERROR NU1605: https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1605

:new: What is the new behavior (if this is a feature change)?

Sorts by project reference before sorting by package dependencies.

:boom: Does this PR introduce a breaking change?

Possibly, but this was tested on a fairly large solution (175+ projects) with a very complex dependency graph and it did not cause any issues.

:bug: Recommendations for testing

Project dependency graph like this:

A -> B -> C -> D

Where -> means "references".

A and D reference the same package. Update the version referenced by D. I had some trouble replicating this in a simple solution, but was able to replicate it consistently with our large solution.

:memo: Links to relevant issues/docs

Similar to: https://github.com/NuKeeperDotNet/NuKeeper/issues/988

:thinking: Checklist before submitting

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

arikalish commented 3 years ago

Still relevant.

promontis commented 3 years ago

@arikalish this seems like good code to resolve NU1605 errors. Do you know why it is not building?

arikalish commented 3 years ago

@promontis Unfortunately not. A lot of PR builds from the time I submitted this PR (and some others) failed for mysterious reasons. Unfortunately, I think it would require a maintainer to resolve the issues as they seem to be with the build pipeline.

The tests that fail are kind of strange, honestly.

promontis commented 3 years ago

@krissetto @CrispyDrone @skolima @rajbos any help is appreciated 🙏

skolima commented 3 years ago

I've generated some automated updates... and yes, the pipeline is failing. I'll see if I can work out why.

msallin commented 3 years ago

https://github.com/microsoft/azure-pipelines-agent/issues/3501

2021-09-20T15:07:27.1692150Z Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1692473Z 
2021-09-20T15:07:27.1692787Z Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1693354Z 
2021-09-20T15:07:27.1693680Z Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1693987Z 
2021-09-20T15:07:27.1694192Z    at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module)
2021-09-20T15:07:27.1694443Z    at System.Reflection.RuntimeModule.GetTypes()
2021-09-20T15:07:27.1694660Z    at System.Reflection.Assembly.GetTypes()
2021-09-20T15:07:27.1694983Z    at Agent.Plugins.Log.TestResultParser.Plugin.ParserFactory.<>c.<GetTestResultParsers>b__0_0(Assembly x)
2021-09-20T15:07:27.1695433Z    at System.Linq.Enumerable.SelectManySingleSelectorIterator`2.MoveNext()
2021-09-20T15:07:27.1695719Z    at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.ToList()
2021-09-20T15:07:27.1695994Z    at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
2021-09-20T15:07:27.1696498Z    at Agent.Plugins.Log.TestResultParser.Plugin.LogParserGateway.<>c__DisplayClass0_0.<InitializeAsync>b__0()
2021-09-20T15:07:27.1696791Z    at System.Threading.Tasks.Task.InnerInvoke()
2021-09-20T15:07:27.1697032Z    at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
2021-09-20T15:07:27.1697449Z    at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
2021-09-20T15:07:27.1697864Z --- End of stack trace from previous location where exception was thrown ---
2021-09-20T15:07:27.1698273Z    at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
2021-09-20T15:07:27.1698742Z    at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
2021-09-20T15:07:27.1699068Z --- End of stack trace from previous location where exception was thrown ---
2021-09-20T15:07:27.1699912Z    at Agent.Plugins.Log.TestResultParser.Plugin.LogParserGateway.InitializeAsync(IClientFactory clientFactory, IPipelineConfig pipelineConfig, ITraceLogger traceLogger, ITelemetryDataCollector telemetry)
2021-09-20T15:07:27.1700445Z    at Agent.Plugins.Log.TestResultParser.Plugin.TestResultLogPlugin.InitializeAsync(IAgentLogPluginContext context)
2021-09-20T15:07:27.1701156Z System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1701547Z 
2021-09-20T15:07:27.1701790Z File name: 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'
2021-09-20T15:07:27.1702029Z 
2021-09-20T15:07:27.1702129Z 
2021-09-20T15:07:27.1703247Z System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1703691Z 
2021-09-20T15:07:27.1703967Z File name: 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'
2021-09-20T15:07:27.1704254Z 
2021-09-20T15:07:27.1704345Z 
2021-09-20T15:07:27.1704777Z System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
2021-09-20T15:07:27.1705215Z 
2021-09-20T15:07:27.1705673Z File name: 'Microsoft.VisualStudio.Services.Agent, Version=2.192.0.0, Culture=neutral, PublicKeyToken=null'
skolima commented 3 years ago

@arikalish this looks IMO like it was a test failure and was solved by #1113 - could you please update from main branch again?

arikalish commented 3 years ago

@skolima sure. I'll update all my PRs today!

promontis commented 3 years ago

Cool! All is green 👍 Can this be merged @skolima?

msallin commented 3 years ago

I like this. However, there was kind of a "feature". Nukeeper usually failed when some references were unnecessary, due to this matter.

Do you think its useful to have an option to opt-in, to check for unnecessary package refs?

skolima commented 3 years ago

I'd like to dig into this a bit more before going "OK". @msallin I'm not fond of "accidental features" like this - there's dedicated tools to trim the dependencies tree.

msallin commented 3 years ago

I'd like to dig into this a bit more before going "OK". @msallin I'm not fond of "accidental features" like this - there's dedicated tools to trim the dependencies tree.

Yes. I agree. It should be accidental. I wasn't aware of such a tool. Can you point to one?

skolima commented 3 years ago

I use ReSharper for this, but seems that VS also has this built-in now https://stackoverflow.com/a/67715612/3205 . I've also used dotnet-outdated for this purpose before https://github.com/dotnet-outdated/dotnet-outdated#reporting-on-transitive-dependencies

arikalish commented 3 years ago

@skolima FWIW: We've added some tests to our solution that pinpoint unused project references using Roslyn but they haven't solved this issue. Also, for folks (perhaps unwisely) using reflection it can be difficult to detect unused references correctly.

IIRC dotnet-outdated calls MSBuild to generate a dependency graph. That's probably the most-correct thing to do but would require significant changes to NuKeeper.

slang25 commented 3 years ago

Yes the way dotnet-outdated does it really nice: https://github.com/dotnet-outdated/dotnet-outdated/blob/846207a9f656f8504f13a4fc51abacb85796c072/src/DotNetOutdated.Core/Services/DependencyGraphService.cs#L28

It shells out to msbuild to create the project.assets.json file, and deserializes that into a model it can work with.

This could also be done in memory using msbuild directly, and picking the appropriate SDK owned msbuild using MSBuildLocator (I've been doing this with a bit of success in other metaprogramming projects).

There is a fair bit of logic that could be simplified in NuKeeper by using MSBuild directly, like evaluating project graphs and and getting PackageReferences "from the horses mouth", with metadata taking us to the source lines they were defined.

skolima commented 3 years ago

@slang25 the catch with this approach - and the reason we avoided it before - is that it doesn't work with .NET Framework Classic. And at least when we looked at this (khem, khem, 2017 I think) it would have been much more complicated, if not completely unsupported.

It might be time to revisit that approach, but keeping in mind that a significant feature of NuKeeper over other tools is that it works with the Classic Framework.

arikalish commented 3 years ago

@skolima dotnet-outdated works on our solution. We're targeting Framework but have updated all of our projects (including a legacy Web project) to the modern csproj format. NuKeeper's support for BitBucket is the big differentiator.

promontis commented 3 years ago

@skolima how is the code looking so far? Would be awesome if this can be merged 🎉

promontis commented 2 years ago

@skolima have you had time to look into the code? Hopefully, this can be merged soon.