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

NoWarn does not flow transitively through P2P references #5501

Closed
natidea opened this issue Jun 27, 2017 · 12 comments
Closed

NoWarn does not flow transitively through P2P references #5501

natidea opened this issue Jun 27, 2017 · 12 comments
Assignees
Labels
Milestone

Comments

@natidea
Copy link

natidea commented Jun 27, 2017

Repro

  1. Create two .NETStandard projects and create a P2P reference: ProjA -> ProjB

  2. Add the following package reference to ProjB:

  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="10.0.0" />
  </ItemGroup>
  1. Observe two NU1603 warnings, and corresponding warnings in the Dependencies Node:

image

  1. Suppress the warning in ProjB either with NoWarn metadata on the package reference, or a global NoWarn property, or through Project Properties UI

Expected
All warnings go away

Actual
The warning for ProjB goes away but the warning for ProjA remains in the error list, and is also in the assets file. The dependency node also displays what looks like a phantom warning for ProjA.

image

This phantom warning is happening because the diagnostic in the assets file was raised and sent to the dependencies node, but it is not displayed because the faulty package reference does not actually exist in ProjA. (Dependencies node should probably do better about not showing a warning icon if the underlying diagnostic is not visible)

Workaround is to also suppress the warning in ProjA, but this can only be done using a global NoWarn. Updating the transitive package reference in ProjA with an MSBuild "Update" does not work, i.e.:

  <ItemGroup>
    <PackageReference Update="Newtonsoft.Json">
      <NoWarn>NU1603</NoWarn>
    </PackageReference>
  </ItemGroup>

Of course, another workaround is to explicitly add the project reference to ProjA instead of receiving it transitively.

Details about Problem

VS: d15prerel.26625.0

NuGet: 4.3.0-preview3-4168

dotnet.exe --version: 1.1.0-preview1-005051

/cc @mishra14 @emgarten @rrelyea @srivatsn

@natidea
Copy link
Author

natidea commented Jun 27, 2017

Perhaps the real problem here is that the original NU1603 warning should not flow transitively through the P2P ref in the first place, because it ends up looking like a duplicate:

image

@mishra14 mishra14 self-assigned this Jun 28, 2017
@mishra14 mishra14 added this to the 4.3 milestone Jun 28, 2017
@mishra14
Copy link
Contributor

I did some preliminary analysis. It seems that on restore, Newtonsoft.Json does flow up to ProjA as we take the flattened graph while restoring ProjA and that contains all the dependencies from ProjB.

Will discuss this a bit more internally.

@rrelyea
Copy link
Contributor

rrelyea commented Jul 5, 2017

Let's continue meeting on friday. for now, this is pushed to post 4.3

@rrelyea rrelyea modified the milestones: 4.4, 4.3 Jul 5, 2017
@mishra14
Copy link
Contributor

mishra14 commented Jul 10, 2017

Moving to 4.4/To-Do based on internal discussion.

@mishra14 mishra14 added the Type:DCR Design Change Request label Jul 10, 2017
@nkolev92
Copy link
Member

Feedback ticket 463600 also raised this concern.

@mishra14
Copy link
Contributor

mishra14 commented Jul 31, 2017

@bording
Copy link

bording commented Aug 10, 2017

Will this work also change how NoWarn is handled for transitive dependencies on a package reference?

For example, in a netcoreapp2.0 project, if I have the following package reference:

<PackageReference Include="ApprovalTests" Version="3.0.13" NoWarn="NU1701" />

The warning is suppressed for the ApprovalTests package itself, but I still see a warning for the ApprovalUtilities package that it depends on:
image

I currently have to add an additional package reference to suppress that warning:

<PackageReference Include="ApprovalUtilities" Version="3.0.13" NoWarn="NU1701" />

It doesn't make sense to me that I should have to do this. If I've suppressed the warning on the package I care about, it should apply to all of its dependencies implicitly.

@mishra14
Copy link
Contributor

@bording - Thanks for bringing up a great point. Unfortunately, current plan will not resolve your case.

but you can easily work that by adding a project wide no warn property -

  <PropertyGroup>
    <NoWarn>NU1701</NoWarn>
  </PropertyGroup>

If you feel that your scenario needs to be resolved as well, please feel free to open up a new issue and link it here. We will add it to our next triage/discussion.

hope this helps!

@bording
Copy link

bording commented Aug 10, 2017

@mishra14 I'd like to avoid a global warning suppression since it would hide the warning for any new packages I might add in the future as well.

I'll open a new issue then, thanks!

edit: Issue opened: #5740

@mishra14
Copy link
Contributor

@bording thanks! We will consider the issue.

@mishra14
Copy link
Contributor

@mishra14
Copy link
Contributor

mishra14 commented Sep 1, 2017

Merged: NuGet/NuGet.Client@4be29f3

@mishra14 mishra14 closed this as completed Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants