Skip to content

Validate binding redirects #7153

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

Merged
merged 28 commits into from
Jan 7, 2022
Merged

Validate binding redirects #7153

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1eb3176
Hacky, untested binding redirect validation
Forgind Dec 15, 2021
ef86539
Revert "Hacky, untested binding redirect validation"
Forgind Dec 15, 2021
11d1b98
Use task to validate packages
Forgind Dec 15, 2021
ac03457
Add Source="Foo.cs"
Forgind Dec 15, 2021
288ecc9
Move code into separate task
Forgind Dec 15, 2021
08bd7b5
Make into full class
Forgind Dec 15, 2021
39731bc
Fix up task
Forgind Dec 15, 2021
29e7068
Fix app.config 🙂
Forgind Dec 15, 2021
45d54d4
Opt System.ValueTuple out
Forgind Dec 15, 2021
d0b8b5b
Revert "Opt System.ValueTuple out"
Forgind Dec 15, 2021
a389c1a
Specifically opt out S.ValueTuple
Forgind Dec 15, 2021
747aa87
Missed a quote
Forgind Dec 15, 2021
354316d
Use String.Equals?
Forgind Dec 16, 2021
5f77395
Move file existence check earlier
Forgind Dec 16, 2021
f2c30e5
Somewhat cleaner version
Forgind Dec 16, 2021
60274a2
Add explicit check for System.ValueTuple
Forgind Dec 16, 2021
c5583f7
Add copyright header
Forgind Dec 16, 2021
1e4a18e
Learn to use namespaces in XML
Forgind Dec 16, 2021
b75a202
Update src/MSBuild/MSBuild.csproj
Forgind Dec 17, 2021
cbb140d
Update src/MSBuild/MSBuild.csproj
Forgind Dec 17, 2021
70555c7
Update src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs
Forgind Dec 17, 2021
42a582f
Add comment for ignored assemblies
Forgind Dec 17, 2021
1ecec1c
Merge branch 'validate-packages-match-binding-redirects' of https://g…
Forgind Dec 17, 2021
abf550f
Revert "Update src/MSBuild/MSBuild.csproj"
Forgind Dec 17, 2021
9099712
Revert "Update src/MSBuild/MSBuild.csproj"
Forgind Jan 5, 2022
0be049f
Stop batching
Forgind Jan 5, 2022
fe5159a
Update comment
Forgind Jan 5, 2022
a4f3b4c
Fix comment
Forgind Jan 6, 2022
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
10 changes: 10 additions & 0 deletions src/MSBuild/MSBuild.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,4 +330,14 @@

</Target>

<UsingTask TaskName="ValidateMSBuildPackageDependencyVersions" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll">
<Task>
<Code Source="ValidateMSBuildPackageDependencyVersions.cs" Language="cs" />
</Task>
</UsingTask>

<Target Name="ValidateMSBuildPackageDependencyVersions" BeforeTargets="AfterBuild">
Copy link
Member

@rainersigwald rainersigwald Dec 16, 2021

Choose a reason for hiding this comment

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

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

<ValidateMSBuildPackageDependencyVersions AppConfig="@(AppConfigWithTargetPath)" AssemblyPath="$(OutputPath)%(_TargetFrameworks.Identity)" />
</Target>

</Project>
87 changes: 87 additions & 0 deletions src/MSBuild/ValidateMSBuildPackageDependencyVersions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
using System;
using System.IO;
using System.Reflection;
using System.Xml;
namespace MSBuild
{
public class ValidateMSBuildPackageDependencyVersions : Task
{
[Required]
public string AppConfig { get; set; }
[Required]
public string AssemblyPath { get; set; }

public override bool Execute()
{
XmlDocument doc = new XmlDocument();
doc.Load(AppConfig);
foreach (var topLevelElement in doc.ChildNodes)
{
if (topLevelElement is XmlElement topLevelXmlElement && topLevelXmlElement.Name.Equals("configuration", StringComparison.OrdinalIgnoreCase))
{
foreach (var configurationElement in topLevelXmlElement.ChildNodes)
{
if (configurationElement is XmlElement configurationXmlElement && configurationXmlElement.Name.Equals("runtime", StringComparison.OrdinalIgnoreCase))
{
foreach (var runtimeElement in configurationXmlElement.ChildNodes)
{
if (runtimeElement is XmlElement runtimeXmlElement && runtimeXmlElement.Name.Equals("assemblyBinding", StringComparison.OrdinalIgnoreCase))
{
foreach (var assemblyBindingElement in runtimeXmlElement.ChildNodes)
{
if (assemblyBindingElement is XmlElement assemblyBindingXmlElement && assemblyBindingXmlElement.Name.Equals("dependentAssembly", StringComparison.OrdinalIgnoreCase))
{
string name = string.Empty;
string version = string.Empty;
foreach (var dependentAssembly in assemblyBindingXmlElement.ChildNodes)
{
if (dependentAssembly is XmlElement dependentAssemblyXmlElement)
{
if (dependentAssemblyXmlElement.Name.Equals("assemblyIdentity", StringComparison.OrdinalIgnoreCase))
{
foreach (var assemblyIdentityAttribute in dependentAssemblyXmlElement.Attributes)
{
if (assemblyIdentityAttribute is XmlAttribute assemblyIdentityAttributeXmlElement && assemblyIdentityAttributeXmlElement.Name.Equals("name", StringComparison.OrdinalIgnoreCase))
{
name = assemblyIdentityAttributeXmlElement.Value;
}
}
Copy link
Member

@rainersigwald rainersigwald Dec 16, 2021

Choose a reason for hiding this comment

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

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");

}
else if (dependentAssemblyXmlElement.Name.Equals("bindingRedirect", StringComparison.OrdinalIgnoreCase))
{
foreach (var bindingRedirectAttribute in dependentAssemblyXmlElement.Attributes)
{
if (bindingRedirectAttribute is XmlAttribute bindingRedirectAttributeXmlElement && bindingRedirectAttributeXmlElement.Name.Equals("newVersion", StringComparison.OrdinalIgnoreCase))
{
version = bindingRedirectAttributeXmlElement.Value;
}
}
}
}
}
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;
Copy link
Member

@rainersigwald rainersigwald Dec 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Forgind Forgind Dec 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Forgind Forgind Dec 16, 2021

Choose a reason for hiding this comment

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

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

if (!version.Equals(assemblyVersion))
{
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}).");
Copy link
Member

@rainersigwald rainersigwald Dec 16, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Forgind Forgind Dec 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rainersigwald rainersigwald Dec 16, 2021

Choose a reason for hiding this comment

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

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.

}
}
}
}
}
}
}
}
}
}
}
return !Log.HasLoggedErrors;
}
}
}
2 changes: 1 addition & 1 deletion src/MSBuild/app.amd64.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
<!-- Redirects for assemblies redistributed by MSBuild (in the .vsix). -->
<dependentAssembly>
<assemblyIdentity name="Microsoft.Bcl.AsyncInterfaces" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-1.0.0.0" newVersion="1.0.0.0" />
<bindingRedirect oldVersion="0.0.0.0-6.0.0.0" newVersion="6.0.0.0" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.NET.StringTools" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
Expand Down
2 changes: 1 addition & 1 deletion src/MSBuild/app.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
<!-- Redirects for assemblies redistributed by MSBuild (in the .vsix). -->
<dependentAssembly>
<assemblyIdentity name="Microsoft.Bcl.AsyncInterfaces" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-1.0.0.0" newVersion="1.0.0.0" />
<bindingRedirect oldVersion="0.0.0.0-6.0.0.0" newVersion="6.0.0.0" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.NET.StringTools" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
Expand Down