Changing the base requirement to use assemblies from VS2015

[StanleyGoldman]

👋

I was testing some changes to the GitHub for Visual Studio extension, and I came across an issue testing in Visual Studio 2015 (https://github.com/github/VisualStudio/issues/2334). After a bit of tracking down, @jcansdale and I the cause of the issue to be both "VS Customize Window Title" and "GitHub Visual Studio" being installed on VS2015. In other versions of Visual Studio this error does not present itself.

In an effort to support multiple versions of Visual Studio, we deploy several MEF assemblies, one compiled against each version of Visual Studio. We expect that only one will be able to load successfully in the given version of Visual Studio and the others will fail.

The cause of the issue stems from this code here. You are allowing us to load our Visual Studio 2017 and 2019 packages. Which causes us to error when we attempt to obtain a service and we erroneously find three.

https://github.com/mayerwin/vs-customize-window-title/blob/9666d5f841a829ea344e8eea1377a1e9ae801d35/CustomizeVSWindowTitle/CustomizeVSWindowTitlePackage.cs#L79-L93

@jcansdale and I did our best to review your code and determine your thinking. The easiest solution we could suggest is to bind your extension against the Visual Studio 2015 libraries and only attach the assembly handler fix when your extension is not running in Visual Studio 2015.

There are problems with this approach, is that it breaks some of your code here. You are allowing us to load our Visual Studio 2017+ distributions in Visual Studio 2015, which is causing us to error out when we try to load things from our packages.

https://github.com/StanleyGoldman/vs-customize-window-title/blob/6b171ba1e2b8d672df3113306281ac93f44846e2/CustomizeVSWindowTitle/Lib/TfsHelper.cs#L54-L55

Let us know what you think of the approach. We don't mind helping out to get us both working together in VS2015 again. ❤️

[mayerwin]

Thanks for this feedback, however won't binding these DLLs cause them to be included in the VSIX package (substantially increasing its size)? Do you have a link to your code where you're loading multiple versions of the same assembly, so I can understand how you're doing it?

I feel the simplest way to fix this would be to prevent my CurrentDomain_AssemblyResolve handler to load versions of assemblies that are for more recent VS versions than the VS instance being run, except if no version has been found for the current version. What do you think of this?

[jcansdale]

Hi @mayerwin,

Thanks for this feedback, however won't binding these DLLs cause them to be included in the VSIX package (substantially increasing its size)?

This shouldn't be a problem because Microsoft.* assemblies are automatically excluded from being included in VSIX files (worth double checking through).

Do you have a link to your code where you're loading multiple versions of the same assembly, so I can understand how you're doing it?

Here is where our MEF assemblies are declared: https://github.com/github/VisualStudio/blob/master/src/GitHub.VisualStudio.Vsix/source.extension.vsixmanifest#L30-L32

Here is the project that targets 15.0 (there are ones for 14.0 and 16.0): https://github.com/github/VisualStudio/blob/master/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj

Unfortunately the TargetVersion attribute is only used by Visual Studio 2017 and 2019. Visual Studio 2015 will attempt to load all 3 of these assemblies. The GitHub.TeamFoundation.15 and GitHub.TeamFoundation.16 assemblies will fail to install their components because they target assemblies that don't exist on Visual Studio 2015. It's a hack, but before the TargetVersion attribute was introduced it was the only way to make it work. 😢

I feel the simplest way to fix this would be to prevent my CurrentDomain_AssemblyResolve handler to load versions of assemblies that are for more recent VS versions than the VS instance being run, except if no version has been found for the current version. What do you think of this?

The problem is that when running on Visual Studio 2015, you would need to redirect the 15.0 (that you reference) to the 14.0 version (which is live). It's allowing a version built against 15.0 to work on Visual Studio 2015 that inadvertently breaks us.

[mayerwin]

Hi @jcansdale

The problem is that when running on Visual Studio 2015, you would need to redirect the 15.0 (that you reference) to the 14.0 version (which is live). It's allowing a version built against 15.0 to work on Visual Studio 2015 that inadvertently breaks us.

I had not found a way to redirect version 15.0 to the version 14.0, this is why I resorted to the assembly loader hook.

You are allowing us to load our Visual Studio 2017 and 2019 packages. Which causes us to error when we attempt to obtain a service and we erroneously find three.

I think the simplest solution would be to just edit this loop as per the TODO:

         foreach (var asm in AppDomain.CurrentDomain.GetAssemblies()) { 
             if (asm.GetName().Name == aname.Name) {
                 //TODO: if we're in VS 14, check if the assembly version is higher than it should be and if so, continue the loop.
                 return asm; 
             }
         } 

The TODO line could be replaced with something like:

if (IsVS14() && shouldSkipVersion(asm.GetName().Version)) continue;

I can easily detect the VS version, but could you just tell me what should be the shouldSkipVersion function?

[jcansdale]

I can easily detect the VS version, but could you just tell me what should be the shouldSkipVersion function?

I think the following would work for us, but wouldn't this stop your extension from working on VS 2015?

if (asm.GetName().Version.Major == 14) continue;

I think this might be the easiest fix. This way it would work without any assembly resolving tricks on VS 2015 and the reflection code would make things work on VS 2017/2019 (combined with the assembly loader hook).

[mayerwin]

Shouldn't this be:

if (IsVS14() && (asm.GetName().Version.Major > 14) continue;

Basically we want to make sure no more recent versions of the assembly are loaded. This is also what you'd need for your extension to work.

For the other solution you suggest with Reflection, it would still require to have the TFS assemblies loaded, so not sure how it solves the issue by itself? The assemblies > V14 also seem to have a number of incompatible changes (some ways to load the credentials are not available in V14).

[jcansdale]

Shouldn't this be:

if (IsVS14() && (asm.GetName().Version.Major > 14) continue;

I believe asm.GetName().Version.Major will always be 14 when running on Visual Studio 2015. The Microsoft.TeamFoundation.* and Microsoft.VisualStudio.Services.* are versioned as part of Visual Studio. Later versions than the running instance should never be loaded and shouldn't be packaged with extensions.

Basically we want to make sure no more recent versions of the assembly are loaded. This is also what you'd need for your extension to work.

More recent versions shouldn't be available and therefor can't be loaded. We need to make sure that the version 14 assembly isn't loaded when a version 15 or 16 version is referenced. The version 15 and 16 versions aren't referenced by our MEF assemblies (and there is no way of stopping them from being visible on Visual Studio 2015).

For the other solution you suggest with Reflection, it would still require to have the TFS assemblies loaded, so not sure how it solves the issue by itself? The assemblies > V14 also seem to have a number of incompatible changes (some ways to load the credentials are not available in V14).

This only makes is work on Visual Studio 2017+ when the version 14 assemblies are being referenced. The same exception will be thrown as before on Visual Studio 2015 (so no functional change there).

I hope that makes sense. It is frustrating and confusing. 😄

[mayerwin]

OK sorry, I had misunderstood you. I thought your package was referencing newer version of the TFS assemblies that shouldn't be loaded.

The cause of the issue stems from this code here. You are allowing us to load our Visual Studio 2017 and 2019 packages.

So in fact, the issue is that your extension includes assemblies that shouldn't be loaded, but that my extension loads. Then, the simplest way is to tell me what are the name of these assemblies (I understand these names have nothing to do with Microsoft but are specific to your extension) so I exclude them from the loop. Let me know if I'm still not getting it :).

[jcansdale]

So in fact, the issue is that your extension includes assemblies that shouldn't be loaded, but that my extension loads.

It doesn't actually include assemblies, but it does reference them. 😄

To give a concrete example:

1. Your assembly references Microsoft.TeamFoundation.Common, v15.0.0.0
2. This doesn't exist on VS 2015 so an AssemblyResolve event is fired
3. Your extension searches the current app domain and finds/returns Microsoft.TeamFoundation.Common, v14.0.0.0 (whenever anything references v15.0.0.0 from this point on, v14.0.0.0 will be used instead)
4. This fixes your extension 🎉
5. Our assembly references Microsoft.TeamFoundation.Common, v15.0.0.0
6. Your extension has already resolved Microsoft.TeamFoundation.Common, v15.0.0.0 to Microsoft.TeamFoundation.Common, v14.0.0.0
7. Our MEF component that references Microsoft.TeamFoundation.Common, v15.0.0.0 loads unexpectedly in VS 2015
8. We end up with multiple MEF components with the same export which breaks our extension 😭

Does that make sense?

[mayerwin]

5. Our assembly references Microsoft.TeamFoundation.Common, v15.0.0.0
6. Your extension has already resolved Microsoft.TeamFoundation.Common, v15.0.0.0 to Microsoft.TeamFoundation.Common, v14.0.0.0
7. Our MEF component that references Microsoft.TeamFoundation.Common, v15.0.0.0 loads unexpectedly in VS 2015

Thanks for this timeline, how can I then prevent your MEF component that references v15.0.0.0 to be loaded? When/where does that happen? I don't understand why it would be loaded by the unresolved assembly hook in my extension if it isn't an assembly. If it is an assembly, then I can simply add a condition to exclude it specifically from being loaded.