-
Notifications
You must be signed in to change notification settings - Fork 152
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
Implement an algorithm to find extensions #1506
Conversation
@OsirisTerje Any thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CharliePoole Two questions
src/NUnitEngine/nunit.engine.core.tests/Services/ExtensionManagerTests.cs
Outdated
Show resolved
Hide resolved
// Start looking two levels above initial directory | ||
var startDir = initialDirectory.Parent.Parent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this not logical. The user specifies a start directory and then this starts looking two levels higher?
It means it cannot find extension in the current directory.
As the algorithm searches upwards, why not start at the indicated directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be right and this may not be a very clear API to use. I'm not opposed to starting in the provided directory itself, but I'll explain the reasoning and maybe we can come up with a plan.
The engine makes the call and it sets the directory to its own location. But the engine is part of a package and I wanted to automatically find extensions installed outside that package. Currently, that means two levels above the engine, which is why the .addins
files we are replacing all had entries starting with at least two levels of ../
.
It's possible to have extensions included in a package, although we currently only do it for the zip package. We do not want the automatic locating algorithm to find those because it cannot have any knowledge of the structure of any particular package. For that purpose, I would continue to use .addins
files within the package.
I think we hve three possibilities here:
- Starting the search in the initial directory provided as you suggest.
- Using a different name for the argument, like engineDirectory, so it's clear that it's not necessarily where the search starts.
- Having the engine specify decide where to start the search based on the type of package, just as the engine decides the package naming pattern to search for based on the type of package.
I think (3) is a better division of responsibilities but I avoided it because it adds complexity. Your comment makes me think I should look at it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CharliePoole I have no clear preference, but one of 1, 2 or 3 should be picked.
Should the engine know how a package or an addin is installed?
I know we talked about dropping the dotnet tool, but if that is still there, it would be running from the nuget package cache, addins could be other nuget packages which would be not only be living in parent directories, but at a subdirectory in the nuget cache.
Even if installed in 'program files' I don't know if addins would be installed under the same directory or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manfred-brands
Thanks. I'll pick one of the three before moving to the next step.
I think the engine should only only know how it is installed. Up to now, we have required that packages should be installed in the same way as the engine. This still like a reasonable limitation to me, although there are some areas where it would be convenient to extend it.
In addition, we are only able to load extensions if we know where they are located. I feel that's the biggest weakness of the NUnit extensibility approach and I think this PR can be a step in the right direction. That is, by using an internal algorithm of any kind, we eliminate the need for the user to know where things are located. Over time, that algorithm can be improved.
Your comment about the nuget cache, makes me think of a possible incremental approach to this...
- Support the nuget cache, i.e. run the engine out of the cache and find extensions in the same place.
- Do the same for dotnet tools installed locally
- Repeat for dotnet tools installed in the global cache
- Extend to allow a globally installed dotnet tool to find extensions installed locally.
BTW, this is how the chocolatey install now works, which is why I tend to prefer it. There is no need for a package that bundles extensions, because users can install and uninstall them as they please.
I suspect that item 1 and possibly 2 of the above incremental steps may already work with the code in this PR, so I'll try to run some tests to find out. I'll see how much I can get done before my course starts next week. If you'd like to try experimenting with any of the above, let me know and I'll focus on the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify:
Support the nuget cache, i.e. run the engine out of the cache and find extensions in the same place.
I mean that the extensions are other nuget packages.
So if the engine is at: $NUGET_PACKAGES/nunit.engine/version/tool/net8.0/nunit.engine.dll
Extensions could be at: $NUGET_PACKAGES/some.extension/version/lib/net8.0/some.extension.dll
The problem is how do you know there is an extension to look for.
You cannot look for all dlls in $NUGET_PACKAGES as there are a lot.
Do users need to specify, e.g. --addin some.extension and then you find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We first look for directory names, not for dlls. The package directory has to be named "NUnit.Exception." so most directories in $NUGET_PACKAGES would be ignored. If the package is well designed, the tools directory will contain a .addins
file specifying the exact assemblies to scan. Otherwise, we will scan all dlls under subdirectories "tools" or "tools/" (to allow for multiply-targeted extensions.
Bear in mind that this PR is not trying to find all extensions, only those in standard format. There are other proposals in issue #488 intended to allow the user to specify additional places to look.
@CharliePoole I did write a response..... but must have forgotten the Enter button.... Or, did I write it somewhere else... ??? IIRC what I wrote, in short is.....: I believe the approach is sound. Just looking at the parent directories should be enough. I don't think we need to add other directories. It does create more options and flexibility for the user, but also more complexity, and do we really need that flexibility? Since we're only looking for configuration files (".addins"), that should be good enough. Later one could introduce a --global switch, to cover eventually addin files found in a global location, kind of the same way git itself does it. |
@OsirisTerje You commented on the general proposal in #1504 but in this case I wanted a review of the API to be exposed by the ExtensionManager and used by the engine, esp. since the ExtensionManager could end up in a separate assembly in the future. @manfred-brands highlighted one problem with the API, which I want to resolve before merging. |
In the latest commit, I have changed the new API method and algorithm as follows...
I've updated the initial comment to issue #1504 to reflect these changes. |
An updated description of the algorithm has been added to #1504. This PR is being closed and will be replaced by a new one based on the issue-1504 branch. |
Fixes #1504 Part of #488
This fix adds a new method to the API for
ExtensionManager
to be used byExtensionService
.The API already included
ExtensionManager.FindExtensions(string initialDirectory)
. That method examines.addins
files in the directory and uses each entry as a hint about where to find candidate assemblies in which extensions are sought. That method continues to exist and work as it has in the past. It will be used in a future enhancement to examine additional directories provided by the user.This issue adds a new method,
ExtensionManager.FindExtensions(string initialDirectory, string[] patterns)
ExtensionManager.FindStandardExtensions(Assembly hostAssembly)
. An internal algorithm (see below) uses the directory containinghostAssembly
as a starting point and searches for assemblies matching one of the patterns provided.ExtensionService
is also changed. In it's startup, it previously used the first method to locate extensions, but now it uses the new method., providing alternate filter patterns depending on whether it is looking for NuGet or Chocolatey extensions. It detects chocolatey by looking for the required file VERIFICATION.txt in the directory where the engine is installed.THE ALGORITHM
The algorithm is internal, in the sense that the caller has no control over it.
except for providing the filters to be matched.ExtensionManager
decides where to look for those matches. The current filters are the same as those previously used in our distributed.addins
files but without the leading sequences of../
. Thoseaddins
files are now removed.The current algorithm, which could be enhanced in the future, operates as follows...
1 Start in the directory containing the host assembly.
provided initial directory's grandparent, two levels up.2. Apply
each patterna predefined set of patterns within that directory, recording the assemblies found as we have always done.ExtensionManager
is able to determine whether to use standard NuGet naming patterns or chocolatey patterns, depending on the host.3. Move to the parent of the directory we just did and repeat at step 2 until the parent returns as null.
By continuously looking at parent directories, it turns out that we can find our way out of a standalone executable and locate installed extensions, which may be eight to ten levels up. In the current issue, I added one test to the netcore runner using an extension and it works. In issue #1505 I'll complete that work, making all extensions work.
In future, we could look for matches in additional places, e.g. in an
addins
directory in the initial directory. I decided to stick with the basics needed to get our own extensions working. The work on issue #488 will probably tell us if we need to do more here and will also bring the originalFindExtensions
method back into use to support additional root directories for locating extensions.I added a few names of reviewers but I'm no longer familiar with who works on what. I would welcome reviews from any @nunit team members as well as users of the engine.