Add support for additional extension directories

[CharliePoole]

Fixes #488 (Simplify Location of Addins)

Barring bugs that arise, I'm looking at this as the final version 3 feature aimed at the loading of extensions. As such, I'd appreciate one or more reviews that focus on the functionality (is it enough?) and the api itself (is it clear enough?) as well as the more detailed aspects of the code.

I have updated this comment to reflect the actual implementation.

[manfred-brands]

The same assemblies will be visited multiple times looking for extensions.
In my comments I suggested 2 alternatives for fixing this.

[manfred-brands]

_assemblies is appended to in each call to FindExtensionAssemblies.
This means that one the 2nd call to FindExtensions for a 2nd directory, we will revisit the assemblies found in the 1st call again. This could mean that the same extension is loaded multiple times.
In addition the assemblies loaded by FindStandardExtensions will be visited again.

If FindExtensionsAssemblies would return only the list of new assemblies, only those would be searched.

Alternative split the finding of assemblies from the finding of extension in those assemblies.
Delete this method and make FindExtensionAssemblies public.
Then after all assemblies are found call a new method FindExtensionsInAssemblies() which iterates over _assemblies like here.
In addition remove the iteration from FindStandardExtensions.

[CharliePoole]

We keep a dictionary of all assembly paths which have already been seen so that we don't re-process them. This was needed before we changed to this three-stage process of finding extensions, because the user could reference the same directory or assembly in multiple .addins files. You can see this in the ProcessDirectory and ProcessCandidateAssembly methods.

Please look at that code to confirm that it works as intended. Meanwhile, I'll try to construct a test to prove it and prevent regression.

By the way, the new check on _extensionDirectories wasn't strictly necessary, but I wanted to have a clean, non-duplicated list of extension directories for use in this issue. In the end, I didn't make use of it as intended, but I have kept it for a future enhancement.

[manfred-brands]

Indeed you don't visit the same directory twice (_existingDirectories),
don't process the same directory multiple times (_visited) and
don't add the same assembly multiple times (IsDuplicateOf),
However, unless I missed some other guard, I still think you load extensions multiple times.

Say we have two directories on the search lists: A and B, each of them has 1 assembly with 1 extension.
When calling FindExtensions(A) it adds A/Assembly to _assemblies and calls 'FindExtensionsInAssembly(A/Assemby)'
When calling FindExtensions(B) it adds B/Assembly to _assemblies and calls both FindExtensionsInAssembly(A/Assemby) and FindExtensionsInAssembly(A/Assemby)
There is no protection in FindExtensionsInAssembly.

If you have a setup with 2 extension, but them in separate directories and check the logs.
There is no test on FindExtensions, it would require creating extensions on disk and not the Fake ones.

[CharliePoole]

What we need to demonstrate is as follows.

1. There is only one place where an assembly can be added to _assemblies: ProcessCandidateAssembly.
2. That method could be called for a duplicate assembly if...
   2.1 The same asssembly path is specified in two .addins files
   2.2 The same assembly is found in different directories.
3. In either case, it should not add the duplicate to the list...
   3.1 The same path should be ruled out by WasVistited.
   3.2 The same AssemblyName should be ruled out by IsDuplicateOf.

At this point, I'm not convinced it's happening as it should, so I'll set up extensions in two directories and check the logs as you suggest. But really, all of the above needs tests. I think we can create unit tests without anything on disk because we aren't dealing with a list of assemblies here, but a list of ExtensionAssembly and that class can be faked.

Interesting problem. :-)

[CharliePoole]

I'm fairly convinced we are avoiding actual duplicates but we're definitely doing too much re-processing of the assembly list. I'm going to use your first suggestion of returning only the list of new assemblies. The second approach means exposing FindExtensionAssemblies, which should really be called FindPossibleExtensionAssemblies or something like that. I don't think that's a good public method.

[stevenaw]

Thanks for including me as a reviewer here @CharliePoole . Looks great. I've left one nitpick as a comment.

[CharliePoole]

@manfred-brands @stevenaw Thanks for reviewing. I must say that GitHub's file compare leaves something to be desired when code has been moved around.

@manfred-brands I don't think there can be duplication of extensions but I think the code could make that clearer. Please review my comment and see what you think. Clearly there should be a test that demonstrates there's no duplication.

[CharliePoole]

The second commit includes the smaller changes requested in review as well as some refactoring. Additional logging is performed and the IExtensionManager interface is removed. The interface was no longer serving any purpose as ExtensionManager is now only used internally.

The next commit will deal with eliminating the possiblility of duplicate loading of extensions. The key issue is that all candidate assemblies must be identified before any of them are examined for extensions. This is necessary because the best version of a given assembly may end up replacing one that was found earlier.

Currently, this is not happening because the initial call to FindStandardExtensions is made by ExtensionService during it's initialization, while the FindExtension calls are made after initialization by the ConsoleRunner. This will need to be changed so that ExtensionPoints are identified in initialization but all calls to find extensions are made by the runner.

[CharliePoole]

In the third commit, I have followed up on @manfred-brands comments about the potential for duplication. That was a good catch and working to fix it turned up an additional problem.

Before this change, the ExtensionManager located all potential extension assemblies before searching through them for the extensions themselves. This is necessary because the same assembly may exist in different paths, possibly in different versions and targeting different runtimes, and we need to select the best version for our purpose before going any deeper. This was broken in the original code of this PR, with assemblies being found and scanned in distinct groups.

I looked at various ways to fix the problem and settled on the following:

1. FindExtensions methods are replaced with FindExtensionAssemblies.
2. Scanning the assemblies for extensions happens in the ExtensionManager as soon as some call requires it.

[CharliePoole]

@manfred-brands @stevenaw
I'm doing some cleanup and refactoring today for another commit, after which the PR will be ready for re-review.

I'd like to add both of you to the engine team but I wanted to ask first. That's handy even if you don't have time to write code, since GitHub will let me set up automatic assignment of reviews on a round-robin basis. So... would that be OK with you?

[manfred-brands]

@CharliePoole It could be a way for me to get insight in the engine so we can give it a try.

[CharliePoole]

The fourth commit does some refactoring and adds tests. A utility class is now used to track extension assemblies and maintain indexes to them so that it is no longer possible to add an assembly without also updating the indexes. A new internal constructor allows creation of ExtensionAssembly instances without an existing assembly file on disk and is used in testing that class. Some name changes have been made.

@manfred-brands @stevenaw This is now ready to be reviewed again

[stevenaw]

I'd like to add both of you to the engine team but I wanted to ask first

@CharliePoole Thanks for asking. I'd be happy to give it a try as well on the team.

[manfred-brands]

Mostly fine. A few comments.

[manfred-brands]

Thanks @CharliePoole, nothing further.