Never mark files from nuget as in-box fx ref assemblies #2154
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before conflict resolution between nuget and redist list, we would end up in situations where MSBuild marked an assembly from nuget matching redist list with <= version with FrameworkFile=true metadata. This caused us to designate the assembly as type "referenceassembly" in deps.json for PreserveCompilationContext. However, that is not correct as that designation means that the assembly is literally from the targeting pack in Program Files and not from nuget. This no longer happens because the nuget assembly with smaller assembly version will be discarded during conflict resolution, favoring the in-box assembly that is correctly designated as "referenceassembly". Furthermore, an OOB with greater version than in-box will not get FrameworkFile=true set by MSBuild. As such, there's no longer a known real scenario (and hence no test here) where we will observe FrameworkFile=true on an assembly from nuget. This is therefore an defensive change in case it ever happens in the wild for unforeseeable reasons.
|
Hmm, seems I was wrong about this not impacting anything because tests are failing. Investigating |
|
So it turns out that NuGetSourceType == Package and NuGetIsFrameworkReference == true when a package nuspec indicates that a framework reference is to be added. So the fix is wrong for that case, and thankfully we had a test that hit it. This makes me concerned that all NuGetSourceType != '' are vulnerable to this. :( |
eerhardt
approved these changes
May 30, 2018
|
@nguerrera is this good to go in? |
|
somehow someone denied me write access to this repo, meaning my reviews no longer "count". 😝 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before conflict resolution between nuget and redist list, we would end up in
situations where MSBuild marked an assembly from nuget matching redist list with
<= version with FrameworkFile=true metadata. This caused us to designate the
assembly as type "referenceassembly" in deps.json for
PreserveCompilationContext. However, that is not correct as that designation
means that the assembly is literally from the targeting pack in Program Files
and not from nuget.
This no longer happens because the nuget assembly with smaller assembly version
will be discarded during conflict resolution, favoring the in-box assembly that
is correctly designated as "referenceassembly". Furthermore, an OOB with greater
version than in-box will not get FrameworkFile=true set by MSBuild. As such,
there's no longer a known real scenario (and hence no test here) where we will
observe FrameworkFile=true on an assembly from nuget. This is therefore an
defensive change in case it ever happens in the wild for unforeseeable reasons.
Fix #1738