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

Detect package references containing dll's that were not used during compilation #234

Merged
merged 5 commits into from
Apr 14, 2017

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Apr 14, 2017

Note: it does not tell you that you can safely remove package reference it just points out which dll's were not used during compilation. Package that has unused dll could have brought transitive reference that is used, content files, etc.

Sometimes package references are unused only for one of the targets so users need to pay attention to which target produced the warning.

@dnfclas
Copy link

dnfclas commented Apr 14, 2017

@pakrym,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

/cc @Eilon

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

Sample output:

C:\Users\pakrym\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-15234\build\FindUnusedReferences.targets(13,5): warning : Unused reference in Microsoft.Extensions.Configuration.Test.Common.csproj/net46 Microsoft.Extensions.FileProviders.Physical/2.0.0-preview1-24445/lib/netstandard1.3/Microsoft.Extensions.FileProviders.Physical.dll from package Microsoft.Extensions.FileProviders.Physical [D:\github\aspnet\Configuration\test\Microsoft.Extensions.Configuration.Test.Common\Microsoft.Extensions.Configuration.Test.Common.csproj]
  Microsoft.Extensions.Configuration.Test.Common -> D:\github\aspnet\Configuration\test\Microsoft.Extensions.Configuration.Test.Common\bin\Debug\net46\Microsoft.Extensions.Configuration.Test.Common.dll
C:\Users\pakrym\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-15234\build\FindUnusedReferences.targets(13,5): warning : Unused reference in Microsoft.Extensions.Configuration.AzureKeyVault.csproj/net46 System.Reflection.Metadata/1.4.2/lib/portable-net45+win8/System.Reflection.Metadata.dll from package System.Reflection.Metadata [D:\github\aspnet\Configuration\src\Microsoft.Extensions.Configuration.AzureKeyVault\Microsoft.Extensions.Configuration.AzureKeyVault.csproj]

@pakrym pakrym requested a review from JunTaoLuo April 14, 2017 16:55
<Sdk_FindUnusedReferences Assembly="@(IntermediateAssembly)" References="@(ReferencePath)" Packages="@(PackageDependencies)" Files="@(FileDefinitions)" >
<Output TaskParameter="UnusedReferences" ItemName="UnusedReferences" />
</Sdk_FindUnusedReferences>
<Warning Condition="'@(UnusedReferences)' != ''" Text="Unused reference in $(MSBuildProjectFile)/$(TargetFramework) %(UnusedReferences.Identity) from package %(UnusedReferences.PackageName)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to make this a verifier rule? We really don't have a good way to suppress individual warnings here and I rather not spend time every compilation doing extra work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I need list of things that got passed to compiler and it's not easy to infer from assets.json anymore :(.

@natemcmaster
Copy link
Contributor

How many warnings do you get when building Universe with this change? I'm concerned this will further clutter our build output with warnings that are not actionable.

I think with a few extra checks, this target could make strong recommendations about whether a package is safe to remove. Those additional checks might include:

  • listed as a PackageReference?
  • build targets or content files actually in use?
  • are its transitive references in use?

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

@natemcmaster tons of warnings right now, this definitely needs to be disabled for tests and samples. Maybe this should be run only occasionally to cleanup libraries and not on every run, or it needs a way to specify exclusions.

  1. It always looks only on direct package reference
  2. I think that there are to many ways for package to be involved in build pipeline and it's to hard to track them all, my goal was not to make a tool that gives definite result but just points to a possible problem
  3. Sometimes it's good to know that you can lower package reference to it's transitive dependency

@natemcmaster
Copy link
Contributor

+1 reduce noise.

  • make it a diagnostic that only shows up if you run /verbosity:Normal. Most ppl won't care or won't know what to do with the info
  • or, disable by default. Require setting /p:FindUnusedReferences=true

@kichalla
Copy link
Member

Sometimes it's good to know that you can lower package reference to it's transitive dependency

+1

Does this give warning in the scenario where I have a direct package reference but is not really required to be specified as its going to be brought transitively by another direct package reference that I have? (I feel VS should have built-in support for this)

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

@kichalla no. It gives a warning when you reference a package but don't use it's dll, sometimes it happens that you at the same time use dll of transitive dependency in brings, so you can replace package reference with it's transitive dependency.

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

This is disabled by default.

@pranavkm
Copy link
Contributor

Is there a lot of utility in this if it's disabled by default?

@pakrym
Copy link
Contributor Author

pakrym commented Apr 14, 2017

I will do aspnet/Coherence#175 by enabling it per repo maybe fix more bugs and if we feel it gives useful results I'm going to enable it by default.

StringComparer.OrdinalIgnoreCase);

using (var fileStream = File.OpenRead(Assembly))
using (PEReader reader = new PEReader(fileStream))
Copy link
Contributor

Choose a reason for hiding this comment

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

var all day everyday.

@pakrym pakrym merged commit 598f84d into dev Apr 14, 2017
@natemcmaster natemcmaster deleted the pakrym/unused-dll branch June 28, 2017 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants