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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Oct 4, 2017
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9a67833
Modify ResolveAssemblyReference to include assembly name in the message
theunrepentantgeek Aug 1, 2017
ea66621
Modify tests that trigger MSB3277 to verify the message
theunrepentantgeek Aug 1, 2017
ec85ec5
Promote check outside of inner loop
theunrepentantgeek Aug 2, 2017
b50c788
Add new test to explicitly test the message in MSB3277
theunrepentantgeek Aug 3, 2017
a756072
Add unit test to ensure multiple conflicts are correctly reported
theunrepentantgeek Aug 21, 2017
f31e60c
Fixes for compilation and test failures
theunrepentantgeek Aug 21, 2017
b6b3691
Add asserts to affected tests
theunrepentantgeek Aug 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 91 additions & 4 deletions src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3538,7 +3538,7 @@ public void ConflictBetweenNonCopyLocalDependencies()

Execute(t);

Assert.Equal(2, t.ResolvedDependencyFiles.Length);
Assert.Equal(3, t.ResolvedDependencyFiles.Length);
Copy link
Member

Choose a reason for hiding this comment

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

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

Assert.True(ContainsItem(t.ResolvedDependencyFiles, s_myLibraries_V2_DDllPath)); // "Expected to find assembly, but didn't."
Assert.True(ContainsItem(t.ResolvedDependencyFiles, s_myLibraries_V1_DDllPath)); // "Expected to find assembly, but didn't."
Assert.Equal(1, t.SuggestedRedirects.Length);
Expand Down Expand Up @@ -3770,6 +3770,7 @@ public void ConflictWithBackVersionPrimary()
bool result = Execute(t);

Assert.Equal(1, e.Warnings); // @"Expected one warning."
e.AssertLogContainsMessageFromResource(AssemblyResources.GetString, "ResolveAssemblyReference.FoundConflicts", "D");

Assert.Equal(0, t.SuggestedRedirects.Length);
Assert.Equal(3, t.ResolvedFiles.Length);
Expand Down Expand Up @@ -3808,12 +3809,98 @@ public void ConflictWithBackVersionPrimaryWithAutoUnify()
bool result = Execute(t);

Assert.Equal(1, e.Warnings); // @"Expected one warning."
e.AssertLogContainsMessageFromResource(AssemblyResources.GetString, "ResolveAssemblyReference.FoundConflicts", "D");

Assert.Equal(0, t.SuggestedRedirects.Length);
Assert.Equal(3, t.ResolvedFiles.Length);
Assert.True(ContainsItem(t.ResolvedFiles, s_myLibraries_V1_DDllPath)); // "Expected to find assembly, but didn't."
}

/// <summary>
/// Consider this dependency chain:
///
/// App
/// References - B
/// Depends on D version 2
/// References - D, version 1
///
/// Both D1 and D2 are CopyLocal. This is a warning because D1 is a lower version
/// than both D2 so that can't unify. These means that eventually when
/// they're copied to the output directory they'll conflict.
/// </summary>
[Fact]
public void ConflictGeneratesMessageReferencingAssemblyName()
{
ResolveAssemblyReference t = new ResolveAssemblyReference();

MockEngine e = new MockEngine();
t.BuildEngine = e;

t.Assemblies = new ITaskItem[]
{
new TaskItem("B"),
new TaskItem("D, Version=1.0.0.0, Culture=neutral, PublicKeyToken=aaaaaaaaaaaaaaaa")
};

t.SearchPaths = new string[]
{
s_myLibrariesRootPath, s_myLibraries_V2Path, s_myLibraries_V1Path
};

t.TargetFrameworkDirectories = new string[] { s_myVersion20Path };

bool result = Execute(t);

Assert.Equal(1, e.Warnings); // @"Expected one warning."

// Check that we have a message identifying conflicts with "D"
e.AssertLogContainsMessageFromResource(AssemblyResources.GetString, "ResolveAssemblyReference.FoundConflicts", "D");
}


/// <summary>
/// Consider this dependency chain:
///
/// App
/// References - B
/// Depends on D version 2
/// Depends on G, version 2
/// 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.
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

/// </summary>
[Fact]
public void ConflictGeneratesMessageReferencingEachConflictingAssemblyName()
{
ResolveAssemblyReference t = new ResolveAssemblyReference();

MockEngine e = new MockEngine();
t.BuildEngine = e;

t.Assemblies = new ITaskItem[]
{
new TaskItem("B"),
new TaskItem("D, Version=1.0.0.0, Culture=neutral, PublicKeyToken=aaaaaaaaaaaaaaaa"),
new TaskItem("G, Version=1.0.0.0, Culture=neutral, PublicKeyToken=aaaaaaaaaaaaaaaa")
};

t.SearchPaths = new string[]
{
s_myLibrariesRootPath, s_myLibraries_V2Path, s_myLibraries_V1Path
};

t.TargetFrameworkDirectories = new string[] { s_myVersion20Path };

bool result = Execute(t);

Assert.Equal(2, e.Warnings); // @"Expected two warnings."

// Check that we have both the expected messages
e.AssertLogContainsMessageFromResource(AssemblyResources.GetString, "ResolveAssemblyReference.FoundConflicts", "D");
e.AssertLogContainsMessageFromResource(AssemblyResources.GetString, "ResolveAssemblyReference.FoundConflicts", "G");
}

/// <summary>
/// Consider this dependency chain:
///
Expand Down Expand Up @@ -3853,7 +3940,7 @@ public void ConflictWithForeVersionPrimary()
Assert.True(result); // @"Expected a success because this conflict is solvable."
Assert.Equal(3, t.ResolvedFiles.Length);
Assert.True(ContainsItem(t.ResolvedFiles, s_myLibraries_V2_DDllPath)); // "Expected to find assembly, but didn't."
Assert.Equal(1, t.ResolvedDependencyFiles.Length);
Assert.Equal(2, t.ResolvedDependencyFiles.Length);
}


Expand Down Expand Up @@ -4482,7 +4569,7 @@ public void Regress265054()
Execute(t);

Assert.Equal(2, t.ResolvedFiles.Length);
Assert.Equal(3, t.ResolvedDependencyFiles.Length);
Assert.Equal(4, t.ResolvedDependencyFiles.Length);
Assert.True(ContainsItem(t.ResolvedDependencyFiles, s_myLibraries_V1_DDllPath)); // "Expected to find assembly, but didn't."
Assert.True(ContainsItem(t.ResolvedDependencyFiles, s_myLibraries_V1_DDllPath)); // "Expected to find assembly, but didn't."

Expand Down Expand Up @@ -8938,4 +9025,4 @@ private static void GenerateRedistAndProfileXmlLocations(string fullRedistConten
File.WriteAllText(profileRedistList, profileListContents);
}
}
}
}
19 changes: 17 additions & 2 deletions src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceTestFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@ public void Dispose()
protected static readonly string s_myLibraries_V1_DDllPath = Path.Combine(s_myLibraries_V1Path, "D.dll");
protected static readonly string s_myLibraries_V1_E_EDllPath = Path.Combine(s_myLibraries_V1_EPath, "E.dll");
protected static readonly string s_myLibraries_V2_DDllPath = Path.Combine(s_myLibraries_V2Path, "D.dll");
protected static readonly string s_myLibraries_V1_GDllPath = Path.Combine(s_myLibraries_V1Path, "G.dll");
protected static readonly string s_myLibraries_V2_GDllPath = Path.Combine(s_myLibraries_V2Path, "G.dll");

protected static readonly string s_regress454863_ADllPath = Path.Combine(s_rootPathPrefix, "Regress454863", "A.dll");
protected static readonly string s_regress454863_BDllPath = Path.Combine(s_rootPathPrefix, "Regress454863", "B.dll");
Expand Down Expand Up @@ -397,6 +399,8 @@ internal void StopIOMonitoringAndAssert_Zero_IOUse()
s_myLibraries_V1_E_EDllPath,
@"c:\RogueLibraries\v1\D.dll",
s_myLibraries_V2_DDllPath,
s_myLibraries_V1_GDllPath,
s_myLibraries_V2_GDllPath,
@"c:\MyStronglyNamed\A.dll",
@"c:\MyWeaklyNamed\A.dll",
@"c:\MyInaccessible\A.dll",
Expand Down Expand Up @@ -1298,6 +1302,16 @@ internal static AssemblyNameExtension GetAssemblyName(string path)
return new AssemblyNameExtension("D, VErsion=2.0.0.0, CulturE=neutral, PublicKEyToken=aaaaaaaaaaaaaaaa");
}

if (path.EndsWith(s_myLibraries_V1_GDllPath))
{
return new AssemblyNameExtension("G, Version=1.0.0.0, Culture=neutral, PublicKEyToken=aaaaaaaaaaaaaaaa");
}

if (path.EndsWith(s_myLibraries_V2_GDllPath))
{
return new AssemblyNameExtension("G, Version=2.0.0.0, Culture=neutral, PublicKEyToken=aaaaaaaaaaaaaaaa");
}

if (String.Compare(path, @"C:\Regress317975\a.dll", StringComparison.OrdinalIgnoreCase) == 0)
{
return new AssemblyNameExtension("A, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null");
Expand Down Expand Up @@ -2117,7 +2131,8 @@ internal static AssemblyNameExtension[] GetDependencies(string path)
{
return new AssemblyNameExtension[]
{
new AssemblyNameExtension("D, Version=2.0.0.0, Culture=neutral, PuBlIcKeYToken=aaaaaaaaaaaaaaaa")
new AssemblyNameExtension("D, Version=2.0.0.0, Culture=neutral, PuBlIcKeYToken=aaaaaaaaaaaaaaaa"),
new AssemblyNameExtension("G, Version=2.0.0.0, Culture=neutral, PuBlIcKeYToken=aaaaaaaaaaaaaaaa")
};
}

Expand Down Expand Up @@ -3057,4 +3072,4 @@ internal void ExecuteRAROnItemsAndRedist(ResolveAssemblyReference t, MockEngine
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/Tasks/AssemblyDependency/ReferenceTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,7 +1866,7 @@ out AssemblyNameReference[] conflictingReferences
if (entry != null)
{
// We have found an entry in the redist list that this assembly is a framework assembly of some version
// also one if its parent refernces has specific version set to true, therefore we need to make sure
// also one if its parent references has specific version set to true, therefore we need to make sure
// that we do not consider it for conflict resolution.
continue;
}
Expand Down
21 changes: 8 additions & 13 deletions src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,6 @@ quiet at the engine level.
if (idealAssemblyRemappings != null)
{
bool foundAtLeastOneValidBindingRedirect = false;
bool foundAtLeastOneUnresolvableConflict = false;

var buffer = new StringBuilder();
var ns = XNamespace.Get("urn:schemas-microsoft-com:asm.v1");
Expand All @@ -972,9 +971,10 @@ quiet at the engine level.
AssemblyName idealRemappingPartialAssemblyName = idealRemapping.PartialAssemblyName;
Reference reference = idealAssemblyRemappingsIdentities[i].reference;

AssemblyNameExtension[] conflictVictims = reference.GetConflictVictims();

for (int j = 0; j < idealRemapping.BindingRedirects.Length; j++)
{
AssemblyNameExtension[] conflictVictims = reference.GetConflictVictims();
foreach (AssemblyNameExtension conflictVictim in conflictVictims)
{
// Make note we only output a conflict suggestion if the reference has at
Expand Down Expand Up @@ -1034,11 +1034,13 @@ quiet at the engine level.
buffer.Append(node.ToString(SaveOptions.DisableFormatting));
}
}
}

if (conflictVictims.Length == 0)
{
foundAtLeastOneUnresolvableConflict = true;
}
if (conflictVictims.Length == 0)
{
// 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);
}
}

Expand All @@ -1060,13 +1062,6 @@ quiet at the engine level.
// else AutoUnify is on and bindingRedirect generation is not supported
// we don't warn in this case since the binder will automatically unify these remappings
}

if (foundAtLeastOneUnresolvableConflict)
{
// 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");
}
}

// Fifth, log general resolution problems.
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,7 +1532,7 @@
<comment>{StrBegin="MSB3276: "}</comment>
</data>
<data name="ResolveAssemblyReference.FoundConflicts">
<value>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.</value>
<value>MSB3277: Found conflicts between different versions of "{0}" that could not be resolved. These reference conflicts are listed in the build log when log verbosity is set to detailed.</value>
<comment>{StrBegin="MSB3277: "}</comment>
</data>
<data name="ResolveAssemblyReference.LogAttributeFormat">
Expand Down