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

Resolve dependencies from GAC #950

Merged
merged 2 commits into from
Aug 30, 2021
Merged

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Aug 27, 2021

When there are dependencies that the project installs, but they would only resolve via a reference that is located in GAC (e.g. System.ValueTuple via netstandard) then the deployment into Out folder would ignore it.

If user then installs a version that is newer than what is in GAC it will get into their assembly redirects, but won't get copied into Out folder, resulting into TypeLoad exception for tests with DeploymentItem attribute.

This change fixes the resolver to look through all the dependencies including GAC dependencies and then copy over only the ones that are found in the bin folder, because ultimately Out should be just a subset (or the same) as the contents in bin folder.

Looking through GAC assemblies does not seem to add significant overhead the whole resolve is under 300ms so hopefully we don't need an option to configure enabling this.

(The new log messages won't go into diag log, because it probably is not correctly initialized in the new appdomain that loads the dll, but they will be written to Debug Trace and can be observed by DebugView++ or DebugView.)

Fix #949

Copy link
Contributor

@Sanan07 Sanan07 left a comment

Choose a reason for hiding this comment

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

Can you also explain how these changes fix the issue, cause I didn't get it fully.

@nohwnd
Copy link
Member Author

nohwnd commented Aug 30, 2021

@Sanan07

There is DeploymentItem attribute, and this resolver is trying to figure out which assemblies from the bin folder should be copied into Out folder to reduce the amount of copying of unnecessary dependencies.

Before .NET Core it was rare that a GAC assembly (GAC = Global Assembly Cache) would be distributed and acquired outside of the project. But in modern times you can get your dependencies from Nuget rather than from GAC when upgrading them. And then there is .NET Standard, which is a kind of meta assembly that does not pack any implementations, but rather just forwards to other dlls based on the current platform, this gives you unified API surface across platforms, with the flexibility of forwarding to the actual implementation as needed.

The original algorithm searches dependencies of your test assembly recursively, and as soon as it finds a GAC assembly, it will stop searching in that branch of the tree. netstdard.dll is a GAC assembly, so the algo would terminate the search in that branch. BUT under netstandard there are assemblies that can be acquired as Nugets, e.g. System.ValueTuple.dll, which can be a newer version than the one linked by net standard.

Say your project looks like this:

My.Tests.dll
|
---- netstandard.dll (isFromGAC = true)
|       |
|       ----- System.ValueTuple.dll 4.0.0 (IsFromGAC = true) -> System.ValueTuple.dll 4.0.3 (IsFromGAC = false)
|
---- Newtonsoft.Json.dll (IsFromGAC = false)

You have your My.Tests.dll test assembly, which depend on netstandard.dll. Netstandard normally depends on System.ValueTuple.dll from GAC, but because you installed it from Nuget, it will actually end up with System.Value.Tuple 4.0.3 in the bin folder, and a bindind redirect from 4.0.0 to 4.0.3.

When the dependency resolver algorithm kicks in, it will look at My.Tests.dll, will see that it depends on netstandard, and will see that it is from GAC and will search no furher. This will result in System.ValueTuple not being copied into the Out folder.

Instead what needs to happen, is that we will look at the dependencies of the netstandard GAC assembly, and all of it's dependencies to see if any of them is coming from the bin folder and copy all of those over.

Which is what this change does, instead of stopping at any GAC assembly, it resolves all dependencies of all assemblies, and returns all of them that are coming from the bin folder. This is because the bin folder is the king, and we are just trying to reduce what we copy, but never add more.

@nohwnd
Copy link
Member Author

nohwnd commented Aug 30, 2021

The change to hashset is just because sets are better structure to use when you are repeatedly looking for "contains" because they are made for that purpose.

Co-authored-by: Sanan Yuzbashiyev <Sanan07@users.noreply.github.com>
@nohwnd nohwnd requested a review from Haplois August 30, 2021 10:59
Sanan07
Sanan07 previously approved these changes Aug 30, 2021
@nohwnd nohwnd changed the base branch from main to pre/2.2.7 August 30, 2021 11:33
@nohwnd nohwnd dismissed Sanan07’s stale review August 30, 2021 11:33

The base branch was changed.

@nohwnd nohwnd changed the base branch from pre/2.2.7 to main August 30, 2021 11:33
@Haplois Haplois merged commit 0f25394 into microsoft:main Aug 30, 2021
Haplois added a commit that referenced this pull request Aug 30, 2021
Cherry-picked from #950

> When there are dependencies that the project installs, but they would only resolve via a reference that is located in GAC (e.g. System.ValueTuple via netstandard) then the deployment into Out folder would ignore it.
> 
> If user then installs a version that is newer than what is in GAC it will get into their assembly redirects, but won't get copied into Out folder, resulting into TypeLoad exception for tests with DeploymentItem attribute.
> 
> This change fixes the resolver to look through all the dependencies including GAC dependencies and then copy over only the ones that are found in the bin folder, because ultimately Out should be just a subset (or the same) as the contents in bin folder.
> 
> Looking through GAC assemblies does not seem to add significant overhead the whole resolve is under 300ms so hopefully we don't need an option to configure enabling this.
> 
> (The new log messages won't go into diag log, because it probably is not correctly initialized in the new appdomain that loads the dll, but they will be written to Debug Trace and can be observed by DebugView++ or DebugView.)

Co-authored-by: nohwnd <me@jakubjares.com>
Co-authored-by: Sanan Yuzbashiyev <Sanan07@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment items are not copied when they are dependency of a GAC assembly
3 participants