Validate binding redirects #7153

Forgind · 2021-12-15T02:40:15Z

Context

If our binding redirects are off from the packages we actually ship, that can cause problems downstream when we try to integrate with anything other than MSBuild.

Changes Made

Added a post-build (and tests) validation step.

Testing

Built

Notes

How do I convert from a 3-part version to a 4-part version or vice versa? I imagine 3 -> 4 is easier if I can find the relevant assembly, but that would mean looking in our artifacts folder for something that matches its name and seeing if its version matches?
Worked around by looking for shipped assemblies with versions not matching our binding redirects.

~~I was timeboxing this with little bash experience. Sorry it's a mess. Please help fix 🙂~~
Has been fixed.

This reverts commit 1eb3176.

This reverts commit 45d54d4.

rainersigwald · 2021-12-16T15:04:28Z

src/MSBuild/MSBuild.csproj

+    </Task>
+  </UsingTask>
+
+  <Target Name="ValidateMSBuildPackageDependencyVersions" BeforeTargets="AfterBuild">


Only run this target on .NET Framework builds (since that's the only case where the binding redirects apply).

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

rainersigwald · 2021-12-16T15:17:07Z

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

+                                                        foreach (var assemblyIdentityAttribute in dependentAssemblyXmlElement.Attributes)
+                                                        {
+                                                            if (assemblyIdentityAttribute is XmlAttribute assemblyIdentityAttributeXmlElement && assemblyIdentityAttributeXmlElement.Name.Equals("name", StringComparison.OrdinalIgnoreCase))
+                                                            {
+                                                                name = assemblyIdentityAttributeXmlElement.Value;
+                                                            }
+                                                        }


You can simplify this as

Suggested change

foreach (var assemblyIdentityAttribute in dependentAssemblyXmlElement.Attributes)

{

if (assemblyIdentityAttribute is XmlAttribute assemblyIdentityAttributeXmlElement && assemblyIdentityAttributeXmlElement.Name.Equals("name", StringComparison.OrdinalIgnoreCase))

{

name = assemblyIdentityAttributeXmlElement.Value;

}

}

name = dependentAssemblyXmlElement.GetAttribute("name");

rainersigwald · 2021-12-16T15:24:41Z

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

+                                            if (!string.IsNullOrEmpty(name) && !string.IsNullOrEmpty(version))
+                                            {
+                                                string path = Path.Combine(AssemblyPath, name + ".dll");
+                                                string assemblyVersion = File.Exists(path) ? Assembly.LoadFile(path).GetName().Version.ToString() : version;


Calling LoadFile will load the assemblies into the running MSBuild process in a way that's not unloadable, potentially causing problems on subsequent builds (the same problem you're tackling for task assemblies in #7132).

Fortunately since we just need the assembly identity, we don't need to use System.Reflection.Metadata here.

Suggested change

string assemblyVersion = File.Exists(path) ? Assembly.LoadFile(path).GetName().Version.ToString() : version;

string assemblyVersion = AssemblyName.GetAssemblyName(path).Version.ToString();

I don't think the File.Exists default is right--we want to guarantee that we ship the file if we have a binding redirect for it.

I've found a number of cases that seem pretty reasonable that we should ignore like M.B.Conversion.Core, M.B.Engine, and M.NET.StringTools.net35. I can make a list of assemblies to ignore, but it's a bit messy.

I'll ignore all of them, then let you comment on which to keep. (So far, I'd say those three and none of the others.)

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

rainersigwald · 2021-12-16T15:43:29Z

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

+                                                {
+                                                    if (!(String.Equals(name, "System.ValueTuple", StringComparison.OrdinalIgnoreCase) && String.Equals(version, "4.0.0.0") && String.Equals(assemblyVersion, "4.0.3.0")))
+                                                    {
+                                                        Log.LogError($"Binding redirect for '{name}' redirects to a different version ({version}) than MSBuild ships ({assemblyVersion}).");


It would be really nice if you could log the line number in the app.config in this error--then GitHub can put a nice annotation up indicating "you should edit here!"

This appears to be possible but nontrivial: https://stackoverflow.com/a/45182162

Slightly more complicated—this could be an issue in Packages.props or app.config, so if I specify a specific line (in app.config), I may be completely off. I think the current version is roughly as informative (gives the assembly name, so you can find it quickly) but without that possibility of erring.

I don't think I agree with this reasoning--why not point explicitly to one of the locations? But since it's fiddly to get the line number you can leave it if you prefer.

rainersigwald

Much cleaner!

src/MSBuild/MSBuild.csproj

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

rainersigwald · 2021-12-16T22:42:42Z

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

+                                                {
+                                                    if (!(String.Equals(name, "System.ValueTuple", StringComparison.OrdinalIgnoreCase) && String.Equals(version, "4.0.0.0") && String.Equals(assemblyVersion, "4.0.3.0")))
+                                                    {
+                                                        Log.LogError($"Binding redirect for '{name}' redirects to a different version ({version}) than MSBuild ships ({assemblyVersion}).");


I don't think I agree with this reasoning--why not point explicitly to one of the locations? But since it's fiddly to get the line number you can leave it if you prefer.

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

Co-authored-by: Rainer Sigwald <[email protected]>

…ithub.com/Forgind/msbuild into validate-packages-match-binding-redirects

This reverts commit cbb140d.

This reverts commit b75a202.

rainersigwald · 2022-01-06T15:50:40Z

src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

+        [Required]
+        public string AssemblyPath { get; set; }
+
+        // Microsoft.Build.Conversion.Core and Microsoft.Build.Engine are deprecated. We don't ship them, but they're still used in VS for now. We should ensure they're the right version if present.


Suggested change

// Microsoft.Build.Conversion.Core and Microsoft.Build.Engine are deprecated. We don't ship them, but they're still used in VS for now. We should ensure they're the right version if present.

// Microsoft.Build.Conversion.Core and Microsoft.Build.Engine are deprecated. We don't ship them, but they're still used in VS for now. We should ensure they're the right version if present.

Wait, sorry, I didn't notice this before. We do ship these assemblies. Why does this say we don't? Are they maybe not present in this project's output because they're not direct projectreferences?

Good call. Don't know what I was thinking here.

Forgind added 14 commits December 14, 2021 18:36

Hacky, untested binding redirect validation

1eb3176

Revert "Hacky, untested binding redirect validation"

ef86539

This reverts commit 1eb3176.

Use task to validate packages

11d1b98

Add Source="Foo.cs"

ac03457

Move code into separate task

288ecc9

Make into full class

08bd7b5

Fix up task

39731bc

Fix app.config 🙂

29e7068

Opt System.ValueTuple out

45d54d4

Revert "Opt System.ValueTuple out"

d0b8b5b

This reverts commit 45d54d4.

Specifically opt out S.ValueTuple

a389c1a

Missed a quote

747aa87

Use String.Equals?

354316d

Move file existence check earlier

5f77395

Forgind changed the title ~~Hacky, untested binding redirect validation~~ Validate binding redirects Dec 16, 2021

Forgind marked this pull request as ready for review December 16, 2021 01:20

rainersigwald requested changes Dec 16, 2021

View reviewed changes

Forgind added 4 commits December 16, 2021 12:24

Somewhat cleaner version

f2c30e5

Add explicit check for System.ValueTuple

60274a2

Add copyright header

c5583f7

Learn to use namespaces in XML

1e4a18e

rainersigwald reviewed Dec 16, 2021

View reviewed changes

Forgind and others added 8 commits December 16, 2021 16:51

Update src/MSBuild/MSBuild.csproj

b75a202

Co-authored-by: Rainer Sigwald <[email protected]>

Update src/MSBuild/MSBuild.csproj

cbb140d

Co-authored-by: Rainer Sigwald <[email protected]>

Update src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs

70555c7

Co-authored-by: Rainer Sigwald <[email protected]>

Add comment for ignored assemblies

42a582f

Merge branch 'validate-packages-match-binding-redirects' of https://g…

1ecec1c

…ithub.com/Forgind/msbuild into validate-packages-match-binding-redirects

Revert "Update src/MSBuild/MSBuild.csproj"

abf550f

This reverts commit cbb140d.

Revert "Update src/MSBuild/MSBuild.csproj"

9099712

This reverts commit b75a202.

Stop batching

0be049f

Update comment

fe5159a

rainersigwald approved these changes Jan 6, 2022

View reviewed changes

Fix comment

a4f3b4c

Forgind merged commit 0006147 into dotnet:main Jan 7, 2022

Forgind deleted the validate-packages-match-binding-redirects branch January 7, 2022 17:21

rainersigwald mentioned this pull request Apr 5, 2023

Redirect all redistributed dlls #3896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate binding redirects #7153

Validate binding redirects #7153

Forgind commented Dec 15, 2021 •

edited

Loading

rainersigwald Dec 16, 2021

rainersigwald Dec 16, 2021

rainersigwald Dec 16, 2021

Forgind Dec 16, 2021

Forgind Dec 16, 2021

rainersigwald Dec 16, 2021

Forgind Dec 16, 2021

rainersigwald Dec 16, 2021

rainersigwald left a comment

rainersigwald Dec 16, 2021

rainersigwald Jan 6, 2022

Forgind Jan 6, 2022

	string assemblyVersion = File.Exists(path) ? Assembly.LoadFile(path).GetName().Version.ToString() : version;
	string assemblyVersion = AssemblyName.GetAssemblyName(path).Version.ToString();

	// Microsoft.Build.Conversion.Core and Microsoft.Build.Engine are deprecated. We don't ship them, but they're still used in VS for now. We should ensure they're the right version if present.
	// Microsoft.Build.Conversion.Core and Microsoft.Build.Engine are deprecated. We don't ship them, but they're still used in VS for now. We should ensure they're the right version if present.

Validate binding redirects #7153

Validate binding redirects #7153

Conversation

Forgind commented Dec 15, 2021 • edited Loading

Context

Changes Made

Testing

Notes

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

rainersigwald 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

Forgind commented Dec 15, 2021 •

edited

Loading