Optionally output unresolved assembly conflicts #5990

Forgind · 2020-12-29T00:40:03Z

@dsplaisted, does this look how you expected?

dsplaisted · 2020-12-29T02:33:20Z

The additional parameter / output to the task look like what we need. The common targets would also need to be updated to use those when calling the ResolveAssemblyReference task.

For the implementation, it should do the following. I wasn't sure but it looked like it wasn't:

Warnings should still be generated even if OutputUnresolvedAssemblyConflicts is true
The output items should use the assembly name (possibly with .dll on the end, possibly not) as the ItemSpec, not the full log message. That will allow code to check whether (for example) there is a conflict for the Microsoft.Windows.SDK.NET.dll assembly. Additional information can be included in the item metadata.

Thanks!

Forgind · 2020-12-29T02:45:04Z

Does that look better? The extra information you'd want in metadata is the conflicting versions and how each was (directly or indirectly) referenced? (I wasn't sure, so I haven't added that yet.)

dsplaisted

This looks like what we wanted, as far as I can tell.

dsplaisted · 2020-12-29T02:50:00Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

+
+                            if (OutputUnresolvedAssemblyConflicts)
+                            {
+                                _unresolvedConflicts.Add(new TaskItem(assemblyName.Name));


I would include the log message with the human-readable details in the metadata for this item.

We don't currently have a use case for programmatically processing data such as conflicting version numbers, how the assembly was referenced, etc. I would say if it's straightforward to include, then go ahead and do so. But if not, then skip it for now.

Conflicting version numbers is easy, so I included that. How it's referenced is doable, but it's a little verbose, since there could be multiple levels of dependencies (A depends on B depends on C depends on D depends on the conflicting dll). I can add it later if we decide we need it, though.

dsplaisted · 2020-12-29T02:51:03Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -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="UnresolvedAssemblyConflicts"/>


We might want to use a name that's more likely to be unique. Similar to the pattern for the input property, that could be ResolveAssemblyReferenceUnresolvedAssemblyConflicts.

ladipro · 2021-01-04T09:06:30Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

@@ -990,16 +1009,36 @@ List<Exception> generalResolutionExceptions
                            // Log the reference which lost the conflict and the dependencies and source items which caused it.
                            LogReferenceDependenciesAndSourceItemsToStringBuilder(fusionName, conflictCandidate, logDependencies.AppendLine());

+                            string toOutput;


Suggested change

string toOutput;

string toOutput = StringBuilderCache.GetStringAndRelease(logConflict);

Both if and else do this as their first statement.

ladipro · 2021-01-04T09:12:48Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

+        public ITaskItem[] UnresolvedAssemblyConflicts {
+            get
+            {
+                return _unresolvedConflicts.ToArray();
+            }
+            }


nit:

Suggested change

public ITaskItem[] UnresolvedAssemblyConflicts {

get

{

return _unresolvedConflicts.ToArray();

}

}

public ITaskItem[] UnresolvedAssemblyConflicts => _unresolvedConflicts.ToArray();

ladipro · 2021-01-04T09:15:25Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

+                                    {"victorVersionNumber", victor.ReferenceVersion.ToString() },
+                                    {"victimVersionNumber", conflictCandidate.ReferenceVersion.ToString() }


super-nit: Missing space after {.

ladipro · 2021-01-04T09:20:07Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

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


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

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

benvillalobos

My understanding of the PR

If a user sets ResolveAssemblyReferenceOutputUnresolvedAssemblyConflicts to true, RAR will populate the output item ResolveAssemblyReferenceUnresolvedAssemblyConflicts, which the SDK will pick up and throw the appropriate error on.

LGTM, pending 2 comments.

benvillalobos · 2021-01-05T02:08:28Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

@@ -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.


instead of logging them.

doesn't it do both?

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

benvillalobos · 2021-01-05T02:15:28Z

src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs

@@ -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()


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?

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.

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.

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.

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

Optionally output unresolved assembly conflicts

722ed62

Fixes dotnet#5934

Forgind added 2 commits December 28, 2020 18:41

Comments

675920d

Switch to item

53cd76d

dsplaisted reviewed Dec 29, 2020

View reviewed changes

Add metadata and test

1a41556

Forgind marked this pull request as ready for review December 29, 2020 18:28

ladipro reviewed Jan 4, 2021

View reviewed changes

Cleanup

ac791d0

benvillalobos reviewed Jan 5, 2021

View reviewed changes

Fix comment

0382b4b

ladipro approved these changes Jan 8, 2021

View reviewed changes

benvillalobos approved these changes Jan 8, 2021

View reviewed changes

Forgind merged commit 0ec390a into dotnet:master Jan 8, 2021

Forgind deleted the assembly-conflict-info branch January 8, 2021 22:17

marcpopMSFT added the new property label Jan 15, 2021

dsplaisted mentioned this pull request Jan 15, 2021

Generate errors for assembly version conflicts for Microsoft.Windows.SDK.NET dotnet/sdk#15370

Closed

sfoslund mentioned this pull request Jan 20, 2021

Fix null reference error on ResolveAssemblyReference #6049

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally output unresolved assembly conflicts #5990

Optionally output unresolved assembly conflicts #5990

Forgind commented Dec 29, 2020

dsplaisted commented Dec 29, 2020 •

edited

Loading

Forgind commented Dec 29, 2020 •

edited

Loading

dsplaisted left a comment

dsplaisted Dec 29, 2020

Forgind Dec 29, 2020

dsplaisted Dec 29, 2020

ladipro Jan 4, 2021

ladipro Jan 4, 2021

ladipro Jan 4, 2021

ladipro Jan 4, 2021

benvillalobos left a comment •

edited

Loading

benvillalobos Jan 5, 2021

Forgind Jan 5, 2021

benvillalobos Jan 5, 2021

Forgind Jan 5, 2021

dsplaisted Jan 5, 2021

benvillalobos Jan 5, 2021

dsplaisted Jan 5, 2021

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

		{"victorVersionNumber", victor.ReferenceVersion.ToString() },
		{"victimVersionNumber", conflictCandidate.ReferenceVersion.ToString() }

Optionally output unresolved assembly conflicts #5990

Optionally output unresolved assembly conflicts #5990

Conversation

Forgind commented Dec 29, 2020

dsplaisted commented Dec 29, 2020 • edited Loading

Forgind commented Dec 29, 2020 • edited Loading

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos left a comment • edited Loading

Choose a reason for hiding this comment

My understanding of the PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Dec 29, 2020 •

edited

Loading

Forgind commented Dec 29, 2020 •

edited

Loading

benvillalobos left a comment •

edited

Loading