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.

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

Add binding redirects for all assemblies redistributed by MSBuild #6334

Merged
merged 3 commits into from Apr 27, 2021

Conversation

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Apr 9, 2021

MSBuild redistributes System.Memory but does not binding redirect it. This causes the QB cache plugin to fail (qb.exe works because it has binding redirects, but the plugin fails because it's a dll and dlls don't have binding redirects).
Both MSBuild and VS ship with System.Memory 4.0.1.1 so this should not have any negative consequences.

In general, shouldn't MSBuild add binding redirects for all the assemblies that it redistributes? I see that MSBuild redistributes a bunch of assemblies but does not binding redirect them. @danmoseley @ericstj

@jeffkl
jeffkl approved these changes Apr 9, 2021
@ericstj
Copy link
Member

@ericstj ericstj commented Apr 9, 2021

I suspect it doesn't need to add bindingRedirects if there was no conflict and it didn't need to unify the types. This is going to start forcing tasks to run on MSBuild's assembly if the one they carry is lower, I guess that's OK. In general MSBuild shouldn't force redirection as it can have side-effects on the task (we've had MSBuild redirect us into breaking NuGet changes in the past). Tasks should see it as their own responsibility to handle redirects for their assemblies. cc @dsplaisted @rainersigwald

@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Apr 12, 2021

@ericstj

Tasks should see it as their own responsibility to handle redirects for their assemblies.

Are you referring to tasks adding assembly resolve event handlers to handle redirects? AFAIK that does not work with assemblies redistributed by MSBuild because the redistributed assemblies are placed next to msbuild.exe (the appdomain's appbase) and thus the CLR does not issue resolve events for those.

This is going to start forcing tasks to run on MSBuild's assembly if the one they carry is lower, I guess that's OK.

Since MSBuild is redistributing this dll I assume the engine really needs it in order to function. And since the engine's dependencies are not isolated from the plugins' dependencies, they all swim in the same type space, so by choosing to redistribute a dll, MSBuild is actually forcing all plugins to use its redistributed version right? Hence the binding redirect just formalizes that and prevents accidentally loading the plugin's assembly first and potentially crashing the engine. One way or another something might crash, and it's probably better if it's not the engine?

@ericstj
Copy link
Member

@ericstj ericstj commented Apr 12, 2021

the CLR does not issue resolve events for those.

It should still issue the event. Only time it doesn't raise the event is when the assembly loads correctly (in which case you didn't need the redirect). I did a quick test and a DLL with matching name and wrong version will raise the event.

so by choosing to redistribute a dll, MSBuild is actually forcing all plugins to use its redistributed version right?

They could load a version side-by-side in the same app domain, which is fine so long as they don't need to exchange the types with MSBuild.

Hence the binding redirect just formalizes that and prevents accidentally loading the plugin's assembly first and potentially crashing the engine

Why would that crash?

Your change may still be OK, but I think it should be more complete by establishing a crisper stance about how to think about these dependencies. For instance: MSBuild will provide assemblies X, Y, Z etc. You don't need to include these with your tasks. Just reference packages X, Y, Z versions NNN and exclude them from your package. In fact, I bet MSBuild could provide a single PackageReference that represents what's "safe" for tasks to use without redistributing for a specific version of MSBuild.

I'd really like to hear from the SDK team on this as this is something they've faced for years. @dsplaisted @wli3

@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Apr 19, 2021

We've accidentally shipped binding redirects before when we have a conflict that AutoGenerateBindingRedirects triggered for, so it wasn't checked in but did ship with VS/our packages. I'm not 100% sure that this was one of them, but I think we should explicitly binding-redirect for our direct redistributed dependencies.

@cdmihai cdmihai force-pushed the cdmihai:redirectSystemMemory branch from fc14926 to cf8250e Apr 19, 2021
@cdmihai cdmihai changed the title Add binding redirect for System.Memory Add binding redirects for all assemblies redistributed by MSBuild Apr 19, 2021
@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Apr 19, 2021

Redirected all assemblies redistributed by the msbuild .vsix package.

@Forgind
Copy link
Member

@Forgind Forgind commented Apr 20, 2021

OverlappingBuildsOfTheSameProjectDifferentTargetsAreAllowed seems to be the failing test. I don't know why it would've failed, so I started it running again.

@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Apr 21, 2021

As far as I can tell it passed a VS insertion: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/318516

@Forgind
Copy link
Member

@Forgind Forgind commented Apr 21, 2021

I'm a little confused about that PR, actually. Request perf ddrits succeeded, but I don't see RPS as having run in the list of checks where I'd expect it. There was something else perf-y in the list, but I'm not sure whether that was the same or not. I requeued request perf ddrits, and we'll see if that does anything different.

src/MSBuild/app.config Show resolved Hide resolved
@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Apr 22, 2021

@Forgind I did a new insertion PR and it passed: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/319415
Feel free to merge / label for merge.

@@ -1,4 +1,8 @@
<Project>
<!--
Make sure to update the binding redirects (in src\MSBuild\app.config) for any changes to

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 23, 2021
Member

👍👍👍👍

This comment has been minimized.

@cdmihai

cdmihai Apr 23, 2021
Author Contributor

Updated to include the 64bit appconfig in that comment as well :)

@ladipro ladipro merged commit b945a71 into dotnet:main Apr 27, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210423.7 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Apr 27, 2021
…tnet#6334)

MSBuild redistributes System.Memory but does not binding redirect it. This causes the QB cache plugin to fail (qb.exe works because it has binding redirects, but the plugin fails because it's a dll and dlls don't have binding redirects).
Both MSBuild and VS ship with System.Memory 4.0.1.1 so this should not have any negative consequences.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants