Enable assembly resolver to ensure that `Start Page` and other icons can be found #1220

jcansdale · 2017-09-07T15:26:01Z

This PR fixes a few potential issues:

Ensures that the icons in GitHub.VisualStudio.imagemanifest icons can be found
Allows assemblies to be resolved when running an unsigned build of the extension
Keeps the extension working even if devenv.exe.config failed to update

The most likely scenario is when someone is using using Visual Studio 2017 (15.3) and has never used the extension before. In this case the Start Page icon next to GitHub will be blank. This is unfortunate because it's when we most want people to notice it!

The reason it doesn't appears is because GitHub.VisualStudio.imagemanifest references the GitHub.VisualStudio by its simple name. In order for the icons to be found, the GitHub.VisualStudio assembly must be loaded before any of them are used. The assembly resolver is in this assembly and will be auto-loaded on UICONTEXT.NoSolution (which should happen before the icons are needed.

Rather than having to maintain a separate list of assemblies that might need resolving, the assembly resolver loads the ProvideCodeBase and ProvideBindingRedirection attributes (from GitHub.VisualStudio) and resolves according to these rules. The idea is to behave as if the dependentAssembly elements had been installed.

The previous implementation of the assembly resolver would resolve any assembly in the extension folder when there was a full name match. This avoided false matches, but also meant it needed to call AssemblyName.GetAssemblyName(file) to see if there was a match.

This implementation will only hit the disk if the assembly being resolved has a matching ProvideCodeBase or ProvideBindingRedirection attribute. It will only resolve an assembly when there is a full name patch.

Fixes #1217

Make `AssemblyResolverPackage` auto-load in case `devenv.exe.config` failed to update when extension was installed.

…ributes This makes assembly resolving behave more like when `devenv.exe.config` has been updated with `dependentAssembly` elements.

This should only happen if installer has failed or running unsigned build.

Assembly resolving should be as quick and side effect free as possible. It's best to avoid triggering assembly loads or writing to disk during assembly resolves. This change delays writing to the log until the package is being disposed.

VS 15.3 appears to load this later than it used to and it can no longer be relied on. Loading on UICONTEXT.NoSolution seems to be enough.

jcansdale · 2017-09-12T11:30:54Z

src/GitHub.VisualStudio/GitHub.VisualStudio.imagemanifest

@@ -1,7 +1,10 @@
 <ImageManifest xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns="http://schemas.microsoft.com/VisualStudio/ImageManifestSchema/2014">

  <Symbols>
+    <!-- We need to ensure that `GitHub.VisualStudio` has loaded before any of these resources are used. -->
+    <!-- This should happen when `AssemblyResolverPackage` auto-loads on `UICONTEXT.NoSolution`. -->
    <String Name="Resources" Value="/GitHub.VisualStudio;component/Resources/icons" />


This seems the culprit was actually here. It tries to find an assembly by its simple name before that assembly has been loaded. I think when the Team Explorer related assemblies get initialized might have changed in VS 15.3. This can cause GitHub.VisualStudio not to get pre-loaded and the icons fail to load.

There is a possible workaround for this:
microsoft/PTVS@ec1b895

I did a spike to see if this might work for us, but it would involve moving a bunch of resources into GitHub.UI and fixing a bunch of other issues (not a point fix). This might be worth investigating at some future time.

jcansdale · 2017-09-12T12:56:49Z

@meaghanlewis, I thought a simple test plan might be useful.

Start from a fresh Visual Studio 2017 (15.3) install.
Install an older version of the extension (2.3.2.32 or earlier).
Open Visual Studio 2017 and confirm that the two circled icons are blank.

(the menu path is View > Other Windows > GitHub)
Install the build from this PR.
Open Visual Studio 2017 and confirm that the two circled icons are now visible (like above).

The Start Page doesn't exist in VS 2015, but it would be worth doing the same but checking the View > Other Windows > GitHub` icon.

It would also be good to get a VM with Visual Studio 2017 (15.2) installed on it. I'm not sure how to install older versions, but it must be possible somehow? They keep updating Visual Studio 2017 and things sometimes break.

It's possible this was also an issue on Visual Studio 2017 (15.2), but no one noticed. It requires a machine where the user hasn't been using the GitHub extension, which makes it very easy to miss! I'm very interested to see what you find...

grokys

LGTM, just a small question and tweak.

grokys · 2017-09-13T09:06:47Z

src/GitHub.VisualStudio/AssemblyResolverPackage.cs

+            if (resolvingAssemblies.Count > 0 || resolvingExceptions.Count > 0)
+            {
+                // Avoid executing any logging code unless there is something to log.
+                WriteToLog();


Why do we log the resolved assemblies and resolve errors when VS is shutdown rather than when they occur?

For a few reasons:

Since this package is always loaded, I wanted to avoid loading any extra assemblies as VS starts.

Avoid the possability of recursive assembly resolving (in a previous version there was an issue with Trace.WriteLine).

Avoid doing any file creation or writes as VS starts.

I've tried to make it as predictable and light weight as possible. :-)

grokys · 2017-09-13T09:21:33Z

src/UnitTests/GitHub.VisualStudio/AssemblyResolverPackageTests.cs

+{
+    public class AssemblyResolverPackageTests
+    {
+        class TheResolveDependentAssemblyMethod


These tests aren't being run under ncrunch because this class needs to be public.

meaghanlewis · 2017-09-13T18:01:06Z

Thanks for the testing notes 👍

I verified this using VS2017 with versions 15.3 and VS 15.0. On a fresh install of an older version of the extension (v2.3.0.24) the GitHub icon was not present, but on updating to the most recent version of the extension (v2.3.2.32) the GitHub icon was present.

The icon is present in both 15.3 and 15.0 with a fresh install of the extension from this PR build.

The View > Other Windows > GitHub option shows the icon in VS2015

jcansdale · 2017-09-13T19:13:59Z

@meaghanlewis,

On a fresh install of an older version of the extension (v2.3.0.24) the GitHub icon was not present, but on updating to the most recent version of the extension (v2.3.2.32) the GitHub icon was present.

Was this with VS2017 15.0? I'm hoping the icon wasn't present on 15.3 until this PR build. 🤞

meaghanlewis · 2017-09-13T21:23:29Z

@jcansdale I started with a fresh install of 15.3 and then installed the extension version v2.3.0.24 and saw no icon, then updated the extension to version v2.3.2.32 and the icon was visible. (Not exactly what you were hoping for...)

Next, I did an uninstall/reinstall of 15.3, and installed extension v2.3.0.24 again. there was no icon at this point but once I updated to the PR build extension the icon was visible.

jcansdale · 2017-09-14T09:10:39Z

@meaghanlewis,

I started with a fresh install of 15.3 and then installed the extension version v2.3.0.24 and saw no icon, then updated the extension to version v2.3.2.32 and the icon was visible. (Not exactly what you were hoping for...)

Lol, it's complicated! I wouldn't be surprised if this happened with VS 15.0. Did you start with a fresh install of 15.0 or 15.3? 😉

Next, I did an uninstall/reinstall of 15.3, and installed extension v2.3.0.24 again. there was no icon at this point but once I updated to the PR build extension the icon was visible.

This is the main thing! 😄 👍

Re-enable AssemblyResolverPackage

6ce7fee

Make `AssemblyResolverPackage` auto-load in case `devenv.exe.config` failed to update when extension was installed.

jcansdale changed the base branch from master to refactor/usage-tracker September 7, 2017 15:37

Make assembly resolver follow rules from ProvideDependentAssembly att…

7b0cc1a

…ributes This makes assembly resolving behave more like when `devenv.exe.config` has been updated with `dependentAssembly` elements.

jcansdale force-pushed the fixes/1217-devenv-config-not-updated branch from 8c6efd7 to 7b0cc1a Compare September 7, 2017 16:23

jcansdale changed the base branch from refactor/usage-tracker to master September 7, 2017 16:24

jcansdale added 4 commits September 8, 2017 10:36

Write to log if assembly is resolved

d542218

This should only happen if installer has failed or running unsigned build.

Avoid logging during assembly resolve event

ef22d3b

Assembly resolving should be as quick and side effect free as possible. It's best to avoid triggering assembly loads or writing to disk during assembly resolves. This change delays writing to the log until the package is being disposed.

No need to load assembly resolver on GitSccProviderId

48c3841

VS 15.3 appears to load this later than it used to and it can no longer be relied on. Loading on UICONTEXT.NoSolution seems to be enough.

Add note about when GitHub.VisualStudio gets auto-loaded

7674dbd

jcansdale commented Sep 12, 2017

View reviewed changes

jcansdale changed the title ~~Enable assembly resolver as fallback for missing dependentAssembly elements in devenv.exe.config~~ Enable assembly resolver to ensure that GitHub.VisualStudio.imagemanifest icons can be found Sep 12, 2017

Tidy up

13b5534

jcansdale changed the title ~~Enable assembly resolver to ensure that GitHub.VisualStudio.imagemanifest icons can be found~~ Enable assembly resolver to ensure that Start Page and other icons can be found Sep 12, 2017

jcansdale requested review from shana and meaghanlewis September 12, 2017 12:00

grokys suggested changes Sep 13, 2017

View reviewed changes

Mark tests as public

48ee1fb

grokys approved these changes Sep 13, 2017

View reviewed changes

Merge branch 'master' into fixes/1217-devenv-config-not-updated

7d6e8de

jcansdale merged commit 75a6572 into master Sep 14, 2017

jcansdale mentioned this pull request Sep 21, 2017

Resolve GitHub.VisualStudio.imagemanifest dependencies using ProvideBindingPath attribute #1236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable assembly resolver to ensure that `Start Page` and other icons can be found #1220

Enable assembly resolver to ensure that `Start Page` and other icons can be found #1220

Uh oh!

jcansdale commented Sep 7, 2017 •

edited

Loading

Uh oh!

jcansdale Sep 12, 2017

Uh oh!

jcansdale commented Sep 12, 2017 •

edited

Loading

Uh oh!

grokys left a comment

Uh oh!

grokys Sep 13, 2017

Uh oh!

jcansdale Sep 13, 2017 •

edited

Loading

Uh oh!

grokys Sep 13, 2017

Uh oh!

meaghanlewis commented Sep 13, 2017 •

edited

Loading

Uh oh!

jcansdale commented Sep 13, 2017

Uh oh!

meaghanlewis commented Sep 13, 2017

Uh oh!

jcansdale commented Sep 14, 2017

Uh oh!

Uh oh!

Enable assembly resolver to ensure that Start Page and other icons can be found #1220

Enable assembly resolver to ensure that Start Page and other icons can be found #1220

Uh oh!

Conversation

jcansdale commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcansdale Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

grokys Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

jcansdale Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

meaghanlewis commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcansdale commented Sep 13, 2017

Uh oh!

meaghanlewis commented Sep 13, 2017

Uh oh!

jcansdale commented Sep 14, 2017

Uh oh!

Uh oh!

Enable assembly resolver to ensure that `Start Page` and other icons can be found #1220

Enable assembly resolver to ensure that `Start Page` and other icons can be found #1220

jcansdale commented Sep 7, 2017 •

edited

Loading

jcansdale commented Sep 12, 2017 •

edited

Loading

jcansdale Sep 13, 2017 •

edited

Loading

meaghanlewis commented Sep 13, 2017 •

edited

Loading