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

Why the nuget library required another library which actually not required? #46182

Closed
chucklu opened this issue Dec 15, 2020 · 26 comments
Closed
Assignees

Comments

@chucklu
Copy link

chucklu commented Dec 15, 2020

Today when I checked the dependency problem, I found I have multi versions of System.Security.Principal.Windows installed across multi projects in same solution.
And when I tried to figure out the dependent of System.Security.Principal.Windows, I got nothing.
There is no library required System.Security.Principal.Windows.

When I try to uninstall System.Security.Principal.Windows, and I get the error info:
Error Unable to uninstall 'System.Security.Principal.Windows.4.7.0' because 'System.Diagnostics.EventLog.4.7.0' depends on it.

When I check the dependency of System.Diagnostics.EventLog.4.7.0 under packages\System.Diagnostics.EventLog.4.7.0\lib\net461\System.Diagnostics.EventLog.dll by dnSpy.
It's weird, it seems the .net version of System.Diagnostics.EventLog did not require the System.Security.Principal.Windows.
However the .net standard verison packages\System.Diagnostics.EventLog.4.7.0\lib\netstandard2.0\System.Diagnostics.EventLog.dll requires the System.Security.Principal.Windows.

When I check the nuget release info https://www.nuget.org/packages/System.Diagnostics.EventLog/4.7.0
Why the documentation shows the .net version has dependency of System.Security.Principal.Windows?

.NETFramework 4.6.1

System.Security.Principal.Windows (>= 4.7.0)

.NETFramework 4.7.2

System.Security.Principal.Windows (>= 4.7.0)
@chucklu
Copy link
Author

chucklu commented Dec 15, 2020

@joperezr Still looks like the release policy problem, could you analysis this?

@chucklu chucklu changed the title Why the nuget required dependency, actually not required by the nuget library? Why the nuget library required another library which actually not required? Dec 15, 2020
@chucklu
Copy link
Author

chucklu commented Dec 15, 2020

The same problem for https://www.nuget.org/packages/System.Security.AccessControl/5.0.0

Check the references of packages\System.Security.AccessControl.5.0.0\lib\net461\System.Security.AccessControl.dll, and it did not reference System.Security.Principal.Windows. However the documentation is as following:

NETFramework 4.7.2

System.Security.Principal.Windows (>= 4.7.0)

@chucklu
Copy link
Author

chucklu commented Dec 15, 2020

The following two library dependencies also not required.
packages\System.Security.Permissions.5.0.0\lib\net461\System.Security.Permissions.dll https://www.nuget.org/packages/System.Security.Permissions/5.0.0
System.Security.Permissions .net version also not require System.Security.AccessControl

.NETFramework 4.6.1

System.Security.AccessControl (>= 5.0.0)

https://www.nuget.org/packages/System.Configuration.ConfigurationManager/5.0.0
packages\System.Configuration.ConfigurationManager.5.0.0\lib\net461\System.Configuration.ConfigurationManager.dll

.NETFramework 4.6.1

System.Security.Permissions (>= 5.0.0)

@joperezr
Copy link
Member

Interesting it does seem like there are some dependencies that might not be required. I have only checked the EventLog package and as you pointed out, it is true that there shouldn't be a dependency onto System.Security.Principal.Windows package. I'll do a deeper investigation into why this is happening.

@joperezr joperezr transferred this issue from dotnet/sdk Dec 17, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 17, 2020
@ghost
Copy link

ghost commented Dec 17, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Today when I checked the dependency problem, I found I have multi versions of System.Security.Principal.Windows installed across multi projects in same solution.
And when I tried to figure out the dependent of System.Security.Principal.Windows, I got nothing.
There is no library required System.Security.Principal.Windows.

When I try to uninstall System.Security.Principal.Windows, and I get the error info:
Error Unable to uninstall 'System.Security.Principal.Windows.4.7.0' because 'System.Diagnostics.EventLog.4.7.0' depends on it.

When I check the dependency of System.Diagnostics.EventLog.4.7.0 under packages\System.Diagnostics.EventLog.4.7.0\lib\net461\System.Diagnostics.EventLog.dll by dnSpy.
It's weird, it seems the .net version of System.Diagnostics.EventLog did not require the System.Security.Principal.Windows.
However the .net standard verison packages\System.Diagnostics.EventLog.4.7.0\lib\netstandard2.0\System.Diagnostics.EventLog.dll requires the System.Security.Principal.Windows.

When I check the nuget release info https://www.nuget.org/packages/System.Diagnostics.EventLog/4.7.0
Why the documentation shows the .net version has dependency of System.Security.Principal.Windows?

.NETFramework 4.6.1

System.Security.Principal.Windows (>= 4.7.0)

.NETFramework 4.7.2

System.Security.Principal.Windows (>= 4.7.0)
Author: chucklu
Assignees: -
Labels:

area-Infrastructure-libraries, packaging, untriaged

Milestone: -

@chucklu
Copy link
Author

chucklu commented Dec 17, 2020

@joperezr By the way, the .net standard version of System.Diagnostics.EventLog requires System.Security.Principal.Windows.
packages\System.Diagnostics.EventLog.4.7.0\lib\netstandard2.0\System.Diagnostics.EventLog.dll
// System.Diagnostics.EventLog, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

Maybe someone mixed the .net version and .net standard version of System.Diagnostics.EventLog library.

@joperezr
Copy link
Member

Dependencies get automatically calculated by our build tooling so this would hardly be a human error, I believe it may instead be a bug in that tooling, which is what I have to investigate

@chucklu
Copy link
Author

chucklu commented Dec 17, 2020

The first version System.Diagnostics.EventLog 4.5.0-preview1-25914-04 also has the same problem
packages\System.Diagnostics.EventLog.4.5.0-preview1-25914-04\lib\net461\System.Diagnostics.EventLog.dll

However the difference is, documentation shows it requires another library(actually id did not require)

.NETFramework 4.6.1

System.Security.Permissions (>= 4.5.0-preview1-25914-04)

@iSazonov
Copy link
Contributor

iSazonov commented Dec 17, 2020

Does it load extra assemblies? If so the repo need to have protection tests for expected dependencies..

@safern
Copy link
Member

safern commented Dec 17, 2020

which is what I have to investigate

@joperezr should we assign you this issue?

@joperezr
Copy link
Member

sure

@joperezr joperezr self-assigned this Dec 17, 2020
@safern safern removed the untriaged New issue has not been triaged by the area owner label Dec 17, 2020
@joperezr
Copy link
Member

The issue seems to be with our PromoteDependencies Task in arcade. Seems like it is not finding the net4.6.1 ref asset in the package for some reason, causing it to promote the netstandard2.0 dependencies to all of the lib implementations where netstandard2.0 would apply (so this includdes net461) I will have to debug into PromoteDependencies in order to figure out why it doesn't see the net461 ref that is also in the package.

@chucklu
Copy link
Author

chucklu commented Dec 21, 2020

@joperezr Any good news? Today I found another similar problem #46284

@joperezr
Copy link
Member

Hey @chucklu sorry I forgot to update my github status, I'm taking some time off for the rest of the year so won't really have time to look at this until January

@chucklu
Copy link
Author

chucklu commented Dec 22, 2020

@joperezr Thanks for your update, have a good time. I will follow this issue.

@chucklu
Copy link
Author

chucklu commented Jan 13, 2021

@joperezr Any update?

@joperezr
Copy link
Member

joperezr commented Jan 13, 2021

Hey @chucklu I’m back from vacation but I’m looking into other issues at the moment but I have this one on my backlog of things to do. Sorry in advance for the delay.

@chucklu
Copy link
Author

chucklu commented Jan 21, 2021

@joperezr Another week passed, when are you available to investigate this issue?

@joperezr
Copy link
Member

Hey @chucklu sorry for the delay but as I said before this is on my backlog but I don’t expect to be able to take a look at this any time soon. Is this blocking you in some way? I ask because we have some other higher priority items we have to get to first, and this is not currently high in priority given that it is not really considered as breaking customers now and just seems to be an issue with our build infrastructure that we only need to get to before our next release so there is still time to do that. If this is actually breaking you, then we can consider your case to get priorities re-evaluated.

@chucklu
Copy link
Author

chucklu commented Mar 15, 2021

@joperezr I have found that you are still keep delivering new packages, https://www.nuget.org/packages/System.Diagnostics.EventLog/5.0.1, 6.0.0-preview.2.21154.6
But the new packages still have the same problem.

@joperezr
Copy link
Member

The fix for this will go in 6.0 packages, so 5.x version packages will still have this issue. As for the 6.x preview packages, once we fix this issue those should no longer have the problem.

@chucklu
Copy link
Author

chucklu commented May 26, 2021

@joperezr Thanks for the update,

@ViktorHofer
Copy link
Member

Based on my understanding, the references to System.Security.Principal.Windows are correct. If a package ships a .NETStandard library with dependencies to other libraries which are shipping then the application tfms (in this case net461 and net472) need to have those dependencies as well.

See #52982 and the repro in it for more details.

@chucklu
Copy link
Author

chucklu commented May 26, 2021

@ViktorHofer Interesting, nobody else find my issue. Thanks for your notification, please check my comments in that issue. @ericstj just give a wrong case.

@joperezr
Copy link
Member

Hey @chucklu, I took a deeper look at this and @ViktorHofer is right, these references are correct and by design so this is not actually an issue. I may be wrong but I think you haven't fully understand why we add these extra references to the packages on netfx even when netfx asset doesn't depend on it. Basically as my collegues @ericstj and @ViktorHofer have mentioned, the problem here is that because the .NET Standard asset depends on those additional references, the .NET Framework asset must also have a dependency on those packages even when itself doesn't directly depend on them. Let me try to explain why:

Imagine the following scenario:
You have an app called MyApplication.exe which targets .NET Framework 4.8, and depend on a library called Foo.dll which targets .NET Standard 2.0. Then imagine that Foo.dll, depends on System.Diagnostics.EventLog package. When Foo.dll got built, it was using the .NET Standard2.0 asset of EventLog because itself targets .NETStandard 2.0, and as part of that, he was getting an additional reference to System.Security.Principal.Windows.dll.
Now imagine that Foo.dll also used System.Security.Principal.Windows.dll directly, and it didn't need to add a specific package reference to it, because simply by having a package reference to System.Diagnostics.EventLog, it was automatically also getting a reference to System.Security.Principal.Windows.dll. When MyApplication.exe consumes Foo.dll and System.Diagnostics.EventLog, it will actually get the net461 asset of System.Diagnostics.EventLog which does not directly depend on System.Security.Principal.Windows, but remember that Foo.dll itself depends on System.Security.Principal.Windows and it didn't have a package reference to it directly because it depended on System.Diagnostics.Eventlog to transitively add a dependency to System.Security.Principal.Windows.
If we followed your advice and remove the unused dependency of System.Security.Principal.Windows from the System.Diagnostics.EventLog package for net48, MyApplication.exe would crash as soon as Foo.dll tries to use System.Security.Principal.Windows. We know that System.Diagnostics.EventLog won't need that dll, but we can't guarantee that other libraries, like Foo.dll in this case, aren't counting on this package to bring in the additional reference to System.Security.Principal.Windows. For this reason, as my collegues @ericstj and @ViktorHofer have explained, System.Diagnostics.EventLog needs to carry a package dependency to System.Security.Principal.Windows in netfx even when it doesn't directly depend on it.

I hope this helps clarifying a bit on why we add these package references, but if not feel free to ask any follow up questions. I'll go ahead and close this issue.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants