Include the name of the conflicting assembly as a part of warning MSB3277 #2379

theunrepentantgeek · 2017-08-01T23:25:05Z

Modify the message MSB3277 to include the name of the conflicting assembly - a partial resolution of #608 . The message will be repeated once for each distinct assembly that is experiencing conflicts.

Old message:

MSB3277: Found conflicts between different versions of the same dependent assembly that could not be resolved.  
These reference conflicts are listed in the build log when log verbosity is set to detailed.

New message (sample):

MSB3277: Found conflicts between different versions of "Newtonsoft.Json" that could not be resolved.  
These reference conflicts are listed in the build log when log verbosity is set to detailed.

I found two unit tests that trigger this warning - ConflictWithBackVersionPrimary() and ConflictWithBackVersionPrimaryWithAutoUnify(), both found in src\Tasks.UnitTests\AssemblyDependency\Miscellaneous.cs; both tests have been modified to assert the correct message exists in the log.

dnfclas · 2017-08-01T23:25:09Z

@theunrepentantgeek,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-08-01T23:33:22Z

@theunrepentantgeek, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

davkean · 2017-08-02T01:49:17Z

👍 Note, this will increase the number of warnings a given project will have, so there is a theoretical break here if the developer has something that baselines warnings and fails if they increase. /warnaserror usage will be fine because they have the same code.

This is a great start, it the next step (not this PR) would be actually include the conflicts; similar to how NuGet has package conflicts in 15.3.

KirillOsenkov · 2017-08-02T02:39:54Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

-                                    foundAtLeastOneUnresolvableConflict = true;
+                                    // This warning is logged regardless of AutoUnify since it means a conflict existed where the reference
+                                    // chosen was not the conflict victor in a version comparison, in other words it was older.
+                                    Log.LogWarningWithCodeFromResources("ResolveAssemblyReference.FoundConflicts", idealRemappingPartialAssemblyName.Name);


If I'm reading this right, this is inside both the for-i and for-j loops. idealRemappingPartialAssemblyName only seems to depend on i, but not j.

I think maybe it's better to keep the bool foundAtLeastOneUnresolvableConflict, and set it to true as before, but log the warning outside of for-j but inside the for-i loop, so that it is output only once per each assembly name.

I didn't test my theory, it's all just from reading the code and psychic debugging. It'd be great to have unit-tests support or deny this.

I think you're right.

Noting that conflictVictims is also invariant across the for-j loop, I've promoted both variable and warning message out of the loop.

I tried to create a unit test that generated multiple identical messages but didn't manage to find a way.

rainersigwald

This looks great, but I'd really like to see a unit test.

That is apparently pretty complicated. I'm looking at writing one myself (since I think that's almost exactly the same amount of effort required to suggest how you could do it).

theunrepentantgeek · 2017-08-02T22:37:53Z

Totally onboard with the desire for a unit test. I've made some progress on getting one written - I think I largely have my head around the way MockEngine stubs out the filesystem to allow the unit tests to run in memory.

But, every scenario that I've tried ends up giving me a different error (MSB3247) instead.

rainersigwald · 2017-08-03T21:01:23Z

@theunrepentantgeek I was running into very similar problems. This is what I got: https://github.com/rainersigwald/msbuild/tree/force-msb3277-test

But it doesn't seem to emit any warnings, and I'm not entirely sure why.

theunrepentantgeek · 2017-08-03T22:41:52Z

I've had a look at your branch @rainersigwald and I suspect we're trying to test different things.

Unless I'm reading them incorrectly (always a possibility), your tests are trying to generate a MSB3277 warning so you can check the message is the expected one. Is this right?

Instead of writing a new test, I found two existing tests (ConflictWithBackVersionPrimary() and ConflictWithBackVersionPrimaryWithAutoUnify(), both in Miscellaneous.cs) that already triggered MSB3277 and then extended them to also check the message.

What I've been unable to do is to write a test where idealRemapping.BindingRedirects (see ResolveAssemblyReferences.cs around line 976) contains more than one item - to try and verify that moving the code out of the for-j loop was necessary.

KirillOsenkov · 2017-08-03T23:02:50Z

I found this old bug: #1996

It mentions that you can trigger this if the primary reference has the lower version than the victim one. Hope this helps??

theunrepentantgeek · 2017-08-03T23:54:32Z

It's not possible for the property idealRemapping.BindingRedirects to contain more than one item, even though it's an array - the code that populates it only supplies a single element array. Here's the code from ReferenceTable.cs, around line # 1914, emphasis comment added:

foreach (AssemblyNameReference assemblyNameReference in assemblyNamesList)
{
    DependentAssembly remapping = new DependentAssembly();
    remapping.PartialAssemblyName = assemblyNameReference.assemblyName.AssemblyName;
    BindingRedirect bindingRedirect = new BindingRedirect();
    bindingRedirect.OldVersionLow = new Version("0.0.0.0");
    bindingRedirect.OldVersionHigh = assemblyNameReference.assemblyName.AssemblyName.Version;
    bindingRedirect.NewVersion = assemblyNameReference.assemblyName.AssemblyName.Version;

    // Populate BindingRedirects with a single element array
    remapping.BindingRedirects = new BindingRedirect[] { bindingRedirect };

    idealRemappingsList.Add(remapping);
}

While moving the log message out of the for-j loop was the right thing to do (it makes more sense when a future dev is reading LogResults()), this means that writing a unit test to show that moving the message prevents duplicates can't be done.

Arguably, this likely also means that the array property BindingRedirects should be changed to a simple reference - but that's a challenge for another time.

I have added an explicit test on the message in MSB3277 which you can see in commit b50c788.

KirillOsenkov · 2017-08-04T00:15:49Z

Awesome detective work, thanks!

I found only one other location here where it's being assigned an array with potentially multiple elements, however I'm not sure that code ever runs:
http://source.dot.net/#Microsoft.Build.Tasks.Core/AppConfig/DependentAssembly.cs,132
http://source.dot.net/#Microsoft.Build.Tasks.Core/AppConfig/DependentAssembly.cs,ddfbec501dc56af9,references
http://source.dot.net/#Microsoft.Build.Tasks.Core/AppConfig/DependentAssembly.cs,c8cedbffa26ad5e0,references

Yeah I think you're right and it's impossible to get that message multiple times, so don't worry about testing that part I think. All changes look good to me.

KirillOsenkov · 2017-08-04T00:18:45Z

I also just found this, not sure if relevant:
http://source.dot.net/#Microsoft.Build.Tasks.UnitTests/AppConfig_Tests.cs,37

theunrepentantgeek · 2017-08-04T01:23:00Z

@KirillOsenkov: I had a look at your test, and I have a suggestion.

In GetAssemblyName() (found in ResolveAssemblyReferenceTestFixture.cs), the assembly names you're returning for Alpha.dll and Beta.dll have zero version specified:

if (string.Compare(path, s_myLibraries_V1_DependsOnV1OfAlphaAndBetaDllPath, StringComparison.OrdinalIgnoreCase) == 0)
{
    return new AssemblyNameExtension("DependsOnV1OfAlphaAndBeta, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null");
}

if (string.Compare(path, s_myLibraries_V2_DependsOnV2OfAlphaAndBetaDllPath, StringComparison.OrdinalIgnoreCase) == 0)
{
    return new AssemblyNameExtension("DependsOnV2OfAlphaAndBeta, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null");
}

I suspect this would cause the second reference to merge with the first (same name, same version), leaving no conflict. What happens if you mark the first as v1.0.0.0 and the second as v2.0.0.0?

theunrepentantgeek · 2017-08-04T20:42:29Z

I've read through the build logs for the failing build on Windows and can't see what the problem might be - looks like a problem writing a file rather than my code change.

rainersigwald · 2017-08-04T20:48:20Z

@dotnet-bot test Windows_NT Build for Full please

Agreed, that doesn't look related to your change.

theunrepentantgeek · 2017-08-15T09:07:06Z

Is there anything more you want done on this PR @rainersigwald ?
I believe it's got good unit test coverage now, but if there's something else you have in mind I can give it a shot.

rainersigwald · 2017-08-15T16:11:31Z

Sorry, this got a bit lost in my to-do list.

What does the new test you created add over the ones that you modified?

What I was hoping for is a test that shows this warning being emitted for all of the assembly names that are in this state (each idealAssemblyRemappings). It sounds like you're saying that's not possible, but I don't understand why; the detailed RAR log should have log information for all such conflicts so it should be somewhere.

That's why I was trying to construct a new set of references; the current set has two versions of D but I couldn't see another one to drag in.

theunrepentantgeek · 2017-08-21T04:37:44Z

Sorry, this got a bit lost in my to-do list.

No concerns at all, @rainersigwald.

I've added a new test ConflictGeneratesMessageReferencingEachConflictingAssemblyName to check the new message is correctly emitted when there are multiple active conflicts. I needed to add another conflicting reference (G.dll) to the fixture metadata.

The comment I made above ("It's not possible for the property idealRemapping.BindingRedirects to contain more than one item") refers to the collection within each idealRemapping item - the code in the public repo always populates that collection with a single item only.

theunrepentantgeek · 2017-08-21T04:59:06Z

Found some tests that I'd broken by adding the new dependency in the fixture metadata - have updated those to pass. Also fixed a dumbass compilation error on my part.

rainersigwald

Awesome! I'm now waiting only on explanation/clarification of the additional reference returned in a few tests.

rainersigwald · 2017-08-21T14:31:13Z

src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs

@@ -3538,7 +3538,7 @@ public void ConflictBetweenNonCopyLocalDependencies()

            Execute(t);

-            Assert.Equal(2, t.ResolvedDependencyFiles.Length);
+            Assert.Equal(3, t.ResolvedDependencyFiles.Length);


Is the new one here G, added for the new test? Please add an assert for it as "documentation".

rainersigwald · 2017-08-21T14:32:02Z

src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs

+        ///   References - D, version 1
+        ///   References - G, version 1
+        ///
+        /// All of Dv1, Dv2, Gv1 and Gv2 are CopyLocal. We should get two conflict warnings, one for D and one for G.


theunrepentantgeek · 2017-08-21T21:32:06Z

I've added assertions to the affected tests to ensure that .ResolvedDependencyFiles contains only the expected items.

BTW - I spotted a bug in the test Regress265054: it was asserting the same thing twice.

rainersigwald

Very awesome. Thanks!

davkean · 2017-10-04T04:44:33Z

@AndyGerlicher @jeffkl @cdmihai Is there anything holding this from getting merged?

AndyGerlicher · 2017-10-04T17:27:58Z

Thanks for bringing this back to the top @davkean. I think everything is addressed. And perf looks good:

Evaluation: Time (ms)

Test	Overall	Significant	Value
DotnetConsoleProject	🔴	yes	26.9986 -> 27.0686 (0.259%)
DotnetWebProject	::ok_hand:	no	41.4498 -> 41.4041 (-0.11%)
DotnetMvcProject	✅	yes	78.7176 -> 78.5915 (-0.16%)
Picasso	👌	no	232.4183 -> 232.8356 (0.18%)
SmallP2POldCsproj	✅	yes	35.6656 -> 35.5598 (-0.297%)
Generated_100_100_v150	👌	no	1373.0152 -> 1376.4266 (0.248%)
LargeP2POldCsproj	✅	yes	636.4005 -> 633.2157 (-0.5%)
roslyn	✅	yes	5188.0676 -> 5165.7136 (-0.431%)

Evaluation: Memory (bytes)

Test	Overall	Significant	Value
DotnetConsoleProject	👌	no	4594952 -> 4595428 (0.01%)
DotnetWebProject	::ok_hand:	no	6076905 -> 6076807 (-0.002%)
DotnetMvcProject	::ok_hand:	no	8345692 -> 8345574 (-0.001%)
Picasso	👌	no	31947819 -> 31947889 (0%)
SmallP2POldCsproj	👌	no	5073401 -> 5073424 (0%)
Generated_100_100_v150	::ok_hand:	no	171881050 -> 171881048 (0%)
LargeP2POldCsproj	👌	no	67948686 -> 67948859 (0%)
roslyn	✅	yes	571248416 -> 571243687 (-0.001%)

AndyGerlicher · 2017-10-04T17:56:23Z

Thanks for the contribution!

…3277 (dotnet#2379) * Modify ResolveAssemblyReference to include assembly name in the message * Modify tests that trigger MSB3277 to verify the message * Promote check outside of inner loop * Add new test to explicitly test the message in MSB3277 * Add unit test to ensure multiple conflicts are correctly reported

theunrepentantgeek added 2 commits August 2, 2017 11:18

Modify ResolveAssemblyReference to include assembly name in the message

9a67833

Modify tests that trigger MSB3277 to verify the message

ea66621

dnfclas added the cla-required label Aug 1, 2017

dnfclas added cla-signed and removed cla-required labels Aug 1, 2017

KirillOsenkov requested review from cdmihai, rainersigwald and AndyGerlicher August 2, 2017 02:33

KirillOsenkov reviewed Aug 2, 2017

View reviewed changes

Promote check outside of inner loop

ec85ec5

rainersigwald requested changes Aug 2, 2017

View reviewed changes

Add new test to explicitly test the message in MSB3277

b50c788

Add unit test to ensure multiple conflicts are correctly reported

a756072

Fixes for compilation and test failures

f31e60c

rainersigwald reviewed Aug 21, 2017

View reviewed changes

Add asserts to affected tests

b6b3691

rainersigwald approved these changes Aug 21, 2017

View reviewed changes

cdmihai approved these changes Oct 4, 2017

View reviewed changes

AndyGerlicher merged commit 6c8548c into dotnet:master Oct 4, 2017

cdmihai mentioned this pull request Nov 21, 2017

List conflicts in MSB3277 reference-version-conflict message #608

Closed

theunrepentantgeek deleted the feature/improve-message-for-msb3277 branch April 10, 2018 20:18

This was referenced Apr 28, 2021

Wrong parsing of "There was a conflict" under RAR KirillOsenkov/MSBuildStructuredLog#443

Closed

Improve logging in RAR for FoundConflicts #1996

Closed

qwertie mentioned this pull request Dec 20, 2021

Improve the dreaded "MSB3277: warning MSB3277: Found conflicts between different versions of the same dependent assembly that could not be resolved. " #1712

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include the name of the conflicting assembly as a part of warning MSB3277 #2379

Include the name of the conflicting assembly as a part of warning MSB3277 #2379

theunrepentantgeek commented Aug 1, 2017 •

edited

Loading

dnfclas commented Aug 1, 2017

dnfclas commented Aug 1, 2017

davkean commented Aug 2, 2017

KirillOsenkov Aug 2, 2017

theunrepentantgeek Aug 2, 2017

rainersigwald left a comment

theunrepentantgeek commented Aug 2, 2017 •

edited

Loading

rainersigwald commented Aug 3, 2017

theunrepentantgeek commented Aug 3, 2017 •

edited

Loading

KirillOsenkov commented Aug 3, 2017

theunrepentantgeek commented Aug 3, 2017

KirillOsenkov commented Aug 4, 2017

KirillOsenkov commented Aug 4, 2017

theunrepentantgeek commented Aug 4, 2017

theunrepentantgeek commented Aug 4, 2017 •

edited

Loading

rainersigwald commented Aug 4, 2017

theunrepentantgeek commented Aug 15, 2017

rainersigwald commented Aug 15, 2017

theunrepentantgeek commented Aug 21, 2017 •

edited

Loading

theunrepentantgeek commented Aug 21, 2017

rainersigwald left a comment

rainersigwald Aug 21, 2017

rainersigwald Aug 21, 2017

theunrepentantgeek commented Aug 21, 2017

rainersigwald left a comment

davkean commented Oct 4, 2017

AndyGerlicher commented Oct 4, 2017

AndyGerlicher commented Oct 4, 2017

Include the name of the conflicting assembly as a part of warning MSB3277 #2379

Include the name of the conflicting assembly as a part of warning MSB3277 #2379

Conversation

theunrepentantgeek commented Aug 1, 2017 • edited Loading

dnfclas commented Aug 1, 2017

dnfclas commented Aug 1, 2017

davkean commented Aug 2, 2017

KirillOsenkov Aug 2, 2017

Choose a reason for hiding this comment

theunrepentantgeek Aug 2, 2017

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

theunrepentantgeek commented Aug 2, 2017 • edited Loading

rainersigwald commented Aug 3, 2017

theunrepentantgeek commented Aug 3, 2017 • edited Loading

KirillOsenkov commented Aug 3, 2017

theunrepentantgeek commented Aug 3, 2017

KirillOsenkov commented Aug 4, 2017

KirillOsenkov commented Aug 4, 2017

theunrepentantgeek commented Aug 4, 2017

theunrepentantgeek commented Aug 4, 2017 • edited Loading

rainersigwald commented Aug 4, 2017

theunrepentantgeek commented Aug 15, 2017

rainersigwald commented Aug 15, 2017

theunrepentantgeek commented Aug 21, 2017 • edited Loading

theunrepentantgeek commented Aug 21, 2017

rainersigwald left a comment

Choose a reason for hiding this comment

rainersigwald Aug 21, 2017

Choose a reason for hiding this comment

rainersigwald Aug 21, 2017

Choose a reason for hiding this comment

theunrepentantgeek commented Aug 21, 2017

rainersigwald left a comment

Choose a reason for hiding this comment

davkean commented Oct 4, 2017

AndyGerlicher commented Oct 4, 2017

Evaluation: Time (ms)

Evaluation: Memory (bytes)

AndyGerlicher commented Oct 4, 2017

theunrepentantgeek commented Aug 1, 2017 •

edited

Loading

theunrepentantgeek commented Aug 2, 2017 •

edited

Loading

theunrepentantgeek commented Aug 3, 2017 •

edited

Loading

theunrepentantgeek commented Aug 4, 2017 •

edited

Loading

theunrepentantgeek commented Aug 21, 2017 •

edited

Loading