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

Jump to bottom

Optionally output unresolved assembly conflicts #5990

Merged
merged 6 commits into from
Jan 8, 2021
Merged

Optionally output unresolved assembly conflicts #5990

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
722ed62
Optionally output unresolved assembly conflicts
Forgind Dec 29, 2020
675920d
Comments
Forgind Dec 29, 2020
53cd76d
Switch to item
Forgind Dec 29, 2020
1a41556
Add metadata and test
Forgind Dec 29, 2020
ac791d0
Cleanup
Forgind Jan 4, 2021
0382b4b
Fix comment
Forgind Jan 5, 2021
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
3 changes: 3 additions & 0 deletions ref/Microsoft.Build.Tasks.Core/net/Microsoft.Build.Tasks.Core.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,7 @@ public ResolveAssemblyReference() { }
public Microsoft.Build.Framework.ITaskItem[] InstalledAssemblySubsetTables { get { throw null; } set { } }
public Microsoft.Build.Framework.ITaskItem[] InstalledAssemblyTables { get { throw null; } set { } }
public string[] LatestTargetFrameworkDirectories { get { throw null; } set { } }
public bool OutputUnresolvedAssemblyConflicts { get { throw null; } set { } }
public string ProfileName { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
public Microsoft.Build.Framework.ITaskItem[] RelatedFiles { get { throw null; } }
Expand Down Expand Up @@ -954,6 +955,8 @@ public ResolveAssemblyReference() { }
public string[] TargetFrameworkSubsets { get { throw null; } set { } }
public string TargetFrameworkVersion { get { throw null; } set { } }
public string TargetProcessorArchitecture { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
public Microsoft.Build.Framework.ITaskItem[] UnresolvedAssemblyConflicts { get { throw null; } }
public bool UnresolveFrameworkAssembliesFromHigherFrameworks { get { throw null; } set { } }
public string WarnOrErrorOnTargetArchitectureMismatch { get { throw null; } set { } }
public override bool Execute() { throw null; }
Expand Down
3 changes: 3 additions & 0 deletions ref/Microsoft.Build.Tasks.Core/netstandard/Microsoft.Build.Tasks.Core.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ public ResolveAssemblyReference() { }
public Microsoft.Build.Framework.ITaskItem[] InstalledAssemblySubsetTables { get { throw null; } set { } }
public Microsoft.Build.Framework.ITaskItem[] InstalledAssemblyTables { get { throw null; } set { } }
public string[] LatestTargetFrameworkDirectories { get { throw null; } set { } }
public bool OutputUnresolvedAssemblyConflicts { get { throw null; } set { } }
public string ProfileName { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
public Microsoft.Build.Framework.ITaskItem[] RelatedFiles { get { throw null; } }
Expand Down Expand Up @@ -699,6 +700,8 @@ public ResolveAssemblyReference() { }
public string[] TargetFrameworkSubsets { get { throw null; } set { } }
public string TargetFrameworkVersion { get { throw null; } set { } }
public string TargetProcessorArchitecture { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
public Microsoft.Build.Framework.ITaskItem[] UnresolvedAssemblyConflicts { get { throw null; } }
public bool UnresolveFrameworkAssembliesFromHigherFrameworks { get { throw null; } set { } }
public string WarnOrErrorOnTargetArchitectureMismatch { get { throw null; } set { } }
public override bool Execute() { throw null; }
Expand Down
31 changes: 31 additions & 0 deletions src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3686,6 +3686,37 @@ public void ConflictGeneratesMessageReferencingAssemblyName()
warningMessage.ShouldContain(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ResolveAssemblyReference.FourSpaceIndent", ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("ResolveAssemblyReference.ReferenceDependsOn", "D, Version=1.0.0.0, CulTUre=neutral, PublicKeyToken=aaaaaaaaaaaaaaaa", Path.Combine(s_myLibraries_V1Path, "D.dll"))));
}

[Fact]
public void ConflictOutputsExtraInformationOnDemand()
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a theory and test what happens when the item has the .dll extension? @dsplaisted is there a concrete answer for including .dll in the assembly name? Or did your comment, "possibly with .dll possibly not," refer to the fact that assemblyName.Name may not have the extension along with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I understood him to mean that the SDK can handle the output having the .dll extension or not having it—it's the same information, just presented slightly differently. Since I'm creating the item, and I went with no .dll extension, that shouldn't be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure whether the assembly name processed by ResolveAssemblyReference would have the .dll extension on it. If it consistently does not have the extension, then I would leave it out. It's also theoretically possible to reference an .exe, I believe.

Copy link
Member

Choose a reason for hiding this comment

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

The summary behind assemblyName.Name:

Gets or sets the simple name of the assembly. This is usually, but not necessarily,
the file name of the manifest file of the assembly, minus its extension.

Looks like the SDK will have to account for both scenarios, unless there's a preference on how we handle it here. I'm okay with either scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like MSBuild shouldn't do anything special here then (ie don't add the .dll extension).

{
ResolveAssemblyReference t = new ResolveAssemblyReference();

MockEngine e = new MockEngine(_output);
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 };
t.OutputUnresolvedAssemblyConflicts = true;

Execute(t);

ITaskItem[] conflicts = t.UnresolvedAssemblyConflicts;
conflicts.Length.ShouldBe(1);
conflicts[0].ItemSpec.ShouldBe("D");
conflicts[0].GetMetadata("victorVersionNumber").ShouldBe("1.0.0.0");
conflicts[0].GetMetadata("victimVersionNumber").ShouldBe("2.0.0.0");
}

/// <summary>
/// Consider this dependency chain:
///
Expand Down
45 changes: 42 additions & 3 deletions src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public ResolveAssemblyReference()
private ITaskItem[] _scatterFiles = Array.Empty<TaskItem>();
private ITaskItem[] _copyLocalFiles = Array.Empty<TaskItem>();
private ITaskItem[] _suggestedRedirects = Array.Empty<TaskItem>();
private List<ITaskItem> _unresolvedConflicts = new List<ITaskItem>();
private string[] _targetFrameworkSubsets = Array.Empty<string>();
private string[] _fullTargetFrameworkSubsetNames = Array.Empty<string>();
private string _targetedFrameworkMoniker = String.Empty;
Expand Down Expand Up @@ -214,6 +215,12 @@ public bool IgnoreTargetFrameworkAttributeVersionMismatch
/// </remarks>
public bool FindDependenciesOfExternallyResolvedReferences { get; set; }

/// <summary>
/// If true, outputs any unresolved assembly conflicts (MSB3277) in UnresolvedAssemblyConflicts
/// instead of logging them.
Copy link
Member

Choose a reason for hiding this comment

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

instead of logging them.

doesn't it do both?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! Fixed. That was a holdover from how I'd originally designed it.

/// </summary>
public bool OutputUnresolvedAssemblyConflicts { get; set; }

/// <summary>
/// List of target framework subset names which will be searched for in the target framework directories
/// </summary>
Expand Down Expand Up @@ -915,6 +922,18 @@ public String DependsOnNETStandard
private set;
}

/// <summary>
/// If OutputUnresolvedAssemblyConflicts then a list of information about unresolved conflicts that normally would have
/// been outputted in MSB3277. Otherwise empty.
/// </summary>
[Output]
public ITaskItem[] UnresolvedAssemblyConflicts {
get
{
return _unresolvedConflicts.ToArray();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
public ITaskItem[] UnresolvedAssemblyConflicts {
get
{
return _unresolvedConflicts.ToArray();
}
}
public ITaskItem[] UnresolvedAssemblyConflicts => _unresolvedConflicts.ToArray();


#endregion
#region Logging

Expand Down Expand Up @@ -990,16 +1009,36 @@ quiet at the engine level.
// Log the reference which lost the conflict and the dependencies and source items which caused it.
LogReferenceDependenciesAndSourceItemsToStringBuilder(fusionName, conflictCandidate, logDependencies.AppendLine());

string toOutput;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string toOutput;
string toOutput = StringBuilderCache.GetStringAndRelease(logConflict);

Both if and else do this as their first statement.

if (logWarning)
{
// 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, the victor was older.
Log.LogWarningWithCodeFromResources("ResolveAssemblyReference.FoundConflicts", assemblyName.Name, StringBuilderCache.GetStringAndRelease(logConflict));
toOutput = StringBuilderCache.GetStringAndRelease(logConflict);
Log.LogWarningWithCodeFromResources("ResolveAssemblyReference.FoundConflicts", assemblyName.Name, toOutput);
}
else
{
Log.LogMessage(ChooseReferenceLoggingImportance(conflictCandidate), StringBuilderCache.GetStringAndRelease(logConflict));
Log.LogMessage(MessageImportance.Low, StringBuilderCache.GetStringAndRelease(logDependencies));
toOutput = StringBuilderCache.GetStringAndRelease(logConflict);
string extra = StringBuilderCache.GetStringAndRelease(logDependencies);
Log.LogMessage(ChooseReferenceLoggingImportance(conflictCandidate), toOutput);
Log.LogMessage(MessageImportance.Low, extra);

// This does an extra allocation, so only do it when necessary.
if (OutputUnresolvedAssemblyConflicts)
{
toOutput += '\n' + extra;
Copy link
Member

Choose a reason for hiding this comment

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

Why not be more structured add extra in its own metadatum? Something like:

{ "logMessage", toOutput },
{ "logMessageDetails", extra }

}
}

if (OutputUnresolvedAssemblyConflicts)
{
_unresolvedConflicts.Add(new TaskItem(assemblyName.Name, new Dictionary<string, string>()
{
{ "logMessage", toOutput },
{"victorVersionNumber", victor.ReferenceVersion.ToString() },
{"victimVersionNumber", conflictCandidate.ReferenceVersion.ToString() }
Copy link
Member

Choose a reason for hiding this comment

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

super-nit: Missing space after {.

}));
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/Tasks/Microsoft.Common.CurrentVersion.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2156,6 +2156,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<ResolveAssemblyReferencesFindRelatedSatellites Condition="'$(ResolveAssemblyReferencesFindRelatedSatellites)' == ''">$(BuildingProject)</ResolveAssemblyReferencesFindRelatedSatellites>
<ResolveAssemblyReferencesFindSerializationAssemblies Condition="'$(ResolveAssemblyReferencesFindSerializationAssemblies)' == ''">$(BuildingProject)</ResolveAssemblyReferencesFindSerializationAssemblies>
<ResolveAssemblyReferencesFindRelatedFiles Condition="'$(ResolveAssemblyReferencesFindRelatedFiles)' == ''">$(BuildingProject)</ResolveAssemblyReferencesFindRelatedFiles>
<ResolveAssemblyReferenceOutputUnresolvedAssemblyConflicts Condition="'$(ResolveAssemblyReferenceOutputUnresolvedAssemblyConflicts)' == ''">false</ResolveAssemblyReferenceOutputUnresolvedAssemblyConflicts>
</PropertyGroup>

<ItemGroup>
Expand Down Expand Up @@ -2218,6 +2219,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
IgnoreTargetFrameworkAttributeVersionMismatch ="$(ResolveAssemblyReferenceIgnoreTargetFrameworkAttributeVersionMismatch)"
FindDependenciesOfExternallyResolvedReferences="$(FindDependenciesOfExternallyResolvedReferences)"
ContinueOnError="$(ContinueOnError)"
OutputUnresolvedAssemblyConflicts="$(ResolveAssemblyReferenceOutputUnresolvedAssemblyConflicts)"
Condition="'@(Reference)'!='' or '@(_ResolvedProjectReferencePaths)'!='' or '@(_ExplicitReference)' != ''"
>

Expand All @@ -2233,6 +2235,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<Output TaskParameter="FilesWritten" ItemName="FileWrites"/>
<Output TaskParameter="DependsOnSystemRuntime" PropertyName="DependsOnSystemRuntime"/>
<Output TaskParameter="DependsOnNETStandard" PropertyName="_DependsOnNETStandard"/>
<Output TaskParameter="UnresolvedAssemblyConflicts" ItemName="ResolveAssemblyReferenceUnresolvedAssemblyConflicts"/>
</ResolveAssemblyReference>
</Target>

Expand Down