Nunit-console: Multi-targeted Engine Extensions

[ChrisMaddock]

It would be good to support multi-targeted engine addin's. Two motivations:

1. To allow extensions to target two different .NET Framework versions, where there is a significant advantage to build a version targeting a higher framework, but we don't wish to drop support for older frameworks. https://github.com/nunit/vs-project-loader/issues/25 is a great example of this.

2. To allow the packaging of extensions for potential separate .NET Standard and .NET Framework engines, which we may have in the future.

Here's a rough breakdown of what I think would be involved:

• [ ] Allow engine to prioritise 'highest possible' available framework of addin.
• [ ] Permit engine to safely ignore extensions targeting a framework which can't be loaded. (May already exist?)
• [ ] Define .addins file format for these situations, closely tied to the recommended NuGet packaging method of extensions.

Here's some of my thoughts on how to go about this - comments welcome:

_Updated post-discussion 02/04/18_

Extension precedence
The engine already has code to select the extension of the highest assembly version, when multiple extensions of the same type are available. I propose we expand this to look at Highest Assembly Version and then 'Best' target framework.

For the sake of consistency with the wider .NET ecosystem, I think our definition of 'Best' framework should match NuGet's. We can limit the overhead by only initially implementing support for addins targetting .NET Framework or .NET Standard.

NuGet packaging of Extensions
~For consistency with the wider .NET ecosystem, I believe we should aim to make use of the existing multi-target packaging conventions introduced by NuGet - or the defaults created by dotnet pack on a multi-targeting project.~

~We currently seem to package our extensions under /tools/ in their NuGet package. Is this the right location, given the main engine NuGet package, packages that under /lib/? The NuGet docs recommend /tools/ for "Powershell scripts and programs accessible from the Package Manager Console".~

~Taking account of backwards compatibility - I'd like to look at repackaging our extension dll's with the dlls under the /lib/ directory - as I believe is more standard. I could be wrong there - open to discussion on this one with people more knowledgable than me!~

We'll continue to package extensions under the tools subdir. We'll add in framework specific subdirectories, as below:

tools -> net20 -> extension.dll
      -> net45 -> extension.dll

Addins file format/NuGet packaging
The format of the addins file itself doesn't need to change - it can of course be pointed at a specific dll targeting a specific platform already. The question to look at here is the paths we put in the default .addins files to cater for NuGet and .msi installations.

I'd be tempted just to add an extra level of wildcard directory to the existing paths for msi/NuGet resolution, and have the engine attempt to load all extensions following the extension precedence logic described above. This is less efficient than attempting to read directory names and work out the , but would simplify the code, as the below functionality would already be required, given the current functionality to specify specific paths in the .addins file. Thoughts on this bit?

To ensure backwards compatibility between new extension packages, and old engine installations, and .addins file will be added to the tools directory of new extensions with subdirectory. This will allow the .addins files to 'chain' - and old engines to locate new addins, without the user needing to make any manual edits to the addins file.

This is spun out of https://github.com/nunit/nunit-console/issues/388. @rprouse - if you had particular plans for this part, feel free to boot me off and take the issue back! :smile:

[CharliePoole]

@ChrisMaddock The specification at https://github.com/nunit/docs/wiki/Engine-Addins-Spec makes a distinction between __Extensions__ and __Addins__. We have so far implemented Extensions but not Addins. What you are talking about here, I think is Extension packages but the same change would apply to Addins, if we later implement them. FYI, the file extension .addins reflected my belief that we would actuailly implement Addins, so I'm responsible for some of the confusion!

In any case, I agree we should do multi-targeting provided there are multiple targets. 😄 I think that depends on the outcome of #388, specifically in terms of what targets are supported for the primary engine. It seems as if actual implementation of this issue is blocked until that isssue is resolved.

Discussing the design makes sense though. So...

1. When we are initializing extensions, we are running an engine built to target some framework (1) under some installed framework (2). One thing that requires a bit of thought is how to select an extension when (1) and (2) are different.

2. The original use of tools was for things that should not be referenced by the user program or copied to the user output directory. Command scripts are simply one good example of this. Users should not reference either the engine or its extensions, although our current use of the lib directory for the engine does make this happen (incorrectly). We get away with it for the engine because the engine package is not referenced by user tests.

[One side note here: we don't use nuget or chocolatey either in the most common, standard way. That's clear. However, the way we use them was based on in-depth discussions with the folks who developed those apps and I think is a bit beyond what the average app does.]

1. Are the existing .addins wildcard paths the same for msi and nuget packages? That's surprising to me if so. The idea was that we could tailor the default wildcard path to each environment. The one in the nuget packages should point from a nuget install to any extensions also installed using nuget. Similarly, that for chocolatey points to extensions installed via chocolatey. The msi install includes a .addins file, but I can't tell what's in it by inspecting the .wxi. Maybe we need to create a little table of what it is we are currently doing in each install.

The last paragraph in the issue description references something "below" but there's no below that I can see! Could you clarify?

[ChrisMaddock]

Thanks as always for your thoughts Charlie!

I think that depends on the outcome of #388, specifically in terms of what targets are supported for the primary engine. It seems as if actual implementation of this issue is blocked until that isssue is resolved.

388 is the main driver, but I believe this functionality is useful anyway for things like https://github.com/nunit/vs-project-loader/issues/25. I was hoping I could split it out of Rob's work, and work on it independently. 🙂

When we are initializing extensions, we are running an engine built to target some framework (1) under some installed framework (2). One thing that requires a bit of thought is how to select an extension when (1) and (2) are different.

Agreed. My plan was that the extension which can be loaded is always based on (2). This is related to https://github.com/nunit/vs-project-loader/issues/25 again - it means we can provide/use a .NET 4.6 addin, even though the current engine targets .NET 2.0.

The original use of tools was for things that should not be referenced by the user program or copied to the user output directory [...]

Can I ask how you'd propose packaging for NuGet/Choco, Charlie?

My thoughts were:

lib -> net20 -> extension.dll
    -> net45 -> extension.dll

Do you think we should to that under tools instead?

Regarding the msi, everything currently lives directly under the addins directory. I'd propose a similar structure:

addins -> net20 -> extension.dll
       -> net45 -> extension.dll

The last paragraph in the issue description references something "below" but there's no below that I can see! Could you clarify?

Sorry, I reordered without proofreading! 😄 Reworded:

I'd be tempted just to add an extra level of wildcard directory to the existing paths for msi/NuGet resolution, and have the engine attempt to load all extensions ~as below~ following the extension precedence idea described above.

[ChrisMaddock]

Are the existing .addins wildcard paths the same for msi and nuget packages?

Nooo - apologies if my draft read like that! 😄

Here's the NuGet version:

../../NUnit.Extension.*/**/tools/     # nuget v2 layout
../../../NUnit.Extension.*/**/tools/  # nuget v3 layout

The msi version:

addins/nunit.v2.driver.dll
addins/nunit-v2-result-writer.dll
addins/nunit-project-loader.dll
addins/vs-project-loader.dll
addins/teamcity-event-listener.dll

The choco version:

../../nunit-extension-*/tools/     # find extensions installed under chocolatey

I'd propose simply adding an extra level of wildcard dir to each path, and letting the engine resolve the 'best' extension to chose. Paths should be duplicated for backwards compat. So something like...

../../NUnit.Extension.*/**/tools/     # nuget v2 layout
../../../NUnit.Extension.*/**/tools/  # nuget v3 layout
../../NUnit.Extension.*/**/tools/*/     # nuget v2 layout with target platform
../../../NUnit.Extension.*/**/tools/*/  # nuget v3 layout with target platform

msi (installed as single item - so no backwards compat concerns):

addins/*/

Choco

../../nunit-extension-*/tools/       # find extensions installed under chocolatey
../../nunit-extension-*/tools/*/     # and those with target frameworks

I'm less certain whether choco has any conventions for packaging different dll's for running on different frameworks. Maybe @gep13 could advise. :smile:

[CharliePoole]

Let's back up for a moment to what we already have built into the ExtensionService...

1. It can load assemblies listed in the .addins file.
2. It can load assemblies discovered by a wildcard path - a directory name implies *.dll.
3. If an assembly can't be loaded due to a BadImageFormatException, it gives a warning message (in case 1) or continues silently (case 2).
4. If multiple versions of the same extension (based on the assembly name) can be loaded, it picks the one that has the highest version number.

What if we handled the issue of multiple exception versions by enhancing #4 to examine the target runtime of the extension as well as the version. Then, rather than having structured packages with multiple targets, we could simply have different packages. Use of structured packages seems to me to be advantageous in the usual nuget situation because nuget does the work for us. But in this situation, we have to do the work of selecting ourselves.

I'm not completely opposed to adding structure to our packages but I would lean toward exhausting the options we already have first. If we do want to add that structure, then we may want to put annotations about the runtime platform into the .addins file - in a backward compatible way, obviously.

PS: If you leveraged what's already there as described, then nunit/vs-project-loader#25 would no longer depend on #388.

[ChrisMaddock]

What if we handled the issue of multiple exception versions by enhancing #4 to examine the target runtime of the extension as well as the version. Then, rather than having structured packages with multiple targets, we could simply have different packages.

That's exactly what I was suggesting! 😄 I must have done a rubbish job of explaining it!

You're correct we gain nothing 'automated' from using a structured NuGet package, but we should have a 'convention' to package an extension targeting multiple platforms. We can't put two extension.dll's in the same directory - so I think we have two options. a) Rename the files (e.g. extension-net20.dll, extension-net45.dll) or b) use subdirectories. The latter is how we currently package for NuGet, and also our own framework zip - and I'd personally prefer to continue that convention for the engine extensions. I have no strong preference however!

PS: If you leveraged what's already there as described, then nunit/vs-project-loader#25 would no longer depend on #388.

I agree - I don't see there being a dependency/blocker. #388 being on the horizon is just an additional motivation to do this work.

[ChrisMaddock]

I've missed an edge case.

If we start using subdirectories in Extension NuGet packages, than 'old' NuGet Engines won't be able to location 'new' extensions, without a manual edit to the .addins file. However, it looks like we have existing functionality to chain .addins files - so we could circumvent this by including an additional .addins file in the old default location, to point to the new default location. 🙂

(The above also of course applies to choco)

[CharliePoole]

I was imagining option (c) - use separate packages, which automatically puts things into separate directories.

The packages would have different names, e.g.: vs-project-loader and fancy-vs-project-loader. The name of the included assembly would be the same or different depending on whether the two items were mutually exclusive. So, if "fancy" had all the old functionality __plus__ the new, then you would give them the same name and the engine would pick the best. OTOH, if "fancy" were an addition to the basic one, then you'd give them different names. Fancy would respond yes when asked if it could handle the new project format and the original one would respond no.

And here's another option to handle the same thing: implement extensions to extensions, which has been sitting there as an issue for a long time. It's not a hard thing to do and I'd been thinking of doing it for vs-project-loader when I was still working on the engine. VS project loader already has a number of alternative things it tries and might be more maintainable as a set of separate assemblies.

Just throwing ideas against the wall here. 😄

[CharliePoole]

@ChrisMaddock RE: Chaining .addins files.

Yes, that's available. Small correction: there's no "default" location. The directory containing the engine itself is the required location for the initial addins files (there can be more than one). Currently, of course, all chained addins files are processed. We __could__ add some annotation to an addins file that says "only process this for runtime X" Easiest way would be a structured comment on the first line.

[ChrisMaddock]

Small correction: there's no "default" location.

Sorry, by default location, I meant "what's currently specified in out default (distributed) addins files." i.e. ../../../NUnit.Extension.*/**/tools/

I was imagining option (c) - use separate packages, which automatically puts things into separate directories.

Ah! I see, there is third option! I'd disagree that the user should need to install a separate package, depending on which version of the .NET Framework they are running however.

With packaging the extensions together, the user installs the 'feature' they want (in this case, loading VS projects), and gets the best assembly for the job automatically. With separate packages, it seems to user would need to a) be aware of the existence of multiple packages and b) manage which package to pull themselves - which may change in future, if they upgrade the framework they are running on. It would also add additional overhead to our own packaging, and cost with our bundled packages. (What would we include there - the lowest target? All targets? The 'most commonly used', which would presumable be later that net20?)

In my opinion, the end-user experience will be much nicer their if the engine resolves the 'best' target framework to use, and the user is only concerned with which 'features' they require. This is consistent with how we package the framework again - we don't require users to install a separate package per framework version, they just get the 'highest' build available for their platform. The difference of course is that NuGet does the resolution for us there - rather than NUnit being responsible for it.

[CharliePoole]

I don't think option (c) works for the case of the project loader because the functionality is the same in the eyes of the user: "Open VS Projects" We want two implementations for... implementation reasons!

But in other cases, there could be separate functionality from a user POV and we might create separate packages. IMO it's worth experimenting with this and I thought the project issue might be a place to do it. The code you would write to use msbuild would not be all that different and you could later transform it to use a structured package.

[CharliePoole]

I think use of a directory structure is a no-brainer within a package. The real question is whether the engine needs to understand the names of the directories, i.e. a set of constants like net20, etc. It may be better to examine all assemblies (in all directories) and have the metadata specify when they should be used. I'm not sure about this - I'd have to write some code first 😄 - but there could be an advantage to allowing extensions of multiple types to live in the addins directory, for example.

As a reminder... the ExtensionService can examine metadata first, before even trying to load an assembly. The second stage of actually loading it is where you might get an exception, but because we use Mono.Cecil, we can always (so far) look at metadata without error. Once we have loaded, we can ask the assembly things like "can you handle this particular project" and proceed accordingly. It's a fairly simple but powerful structure, if I do say so myself. 😄

[CharliePoole]

RE: Edge Case

Yes, as it now stands, there is no simple way to install a new extension. The user has to edit the .addins file. We could provide a program to do that. In fact, the GUI I'm working on will eventually do it.

So basically, without this, you have to install extensions where the engine expects to find them. A new package with two versions could do this:

  old version => tools/x/extension.dll
  new version => tools/y/extension.dll
  addins file => tools/extension.addins

The addins file would contain

  x/extension.dll
  y/extension.dll

[ChrisMaddock]

don't think option (c) works for the case of the project loader because the functionality is the same in the eyes of the user: "Open VS Projects" We want two implementations for... implementation reasons! But in other cases, there could be separate functionality from a user POV and we might create separate packages.

Agreed!

[examining metadata..]

Yes - I agree basing things on directory name doesn't really fit our current model. If it's not too inefficient, I'd prefer to have the engine examine everything - and rely on directory structure just as the 'human readable' part of the system!

[addins file]

Exactly what I was thinking. 😄

---

I've done a rough spike on the nunit-project-loader over at https://github.com/nunit/nunit-project-loader/pull/14. That's my current thoughts on how we'd want to package - thoughts welcome. 😄

[gep13]

@ChrisMaddock said...
I'm less certain whether choco has any conventions for packaging different dll's for running on different frameworks. Maybe @gep13 could advise. 😄

Does the Chocolatey Package just contain dll's, or does it contain exe's as well?

[ChrisMaddock]

@gep13 - thanks for picking up! Just dll's - below is an example of where we're at now, supporting only full .NET Framework.
https://chocolatey.org/packages/nunit-extension-vs-project-loader

[CharliePoole]

@gep13 The packages are extensions to the our engine, identified by the prefix "nunit-extension-" and contain dlls that are loaded dynamically by the engine itself using the path ../../nunit-extension-*/tools/.

[gep13]

@ChrisMaddock @CharliePoole the reason that I asked is that with the latest release of Chocolatey 0.10.9, there is a new convention of using x86 and x64 folders, in order for the shimming process to correctly identify which exe should be shimmed based on the architecture of the machine being installed onto. If the Chocolatey packages are just containing dll's then the ball is pretty much in your court. We don't really specify any best practices in this area, and instead leave it up to the maintainers of the packages. Chocolatey won't do anything "special" in the case of only dll's.

Hope that helps!

[ChrisMaddock]

Great, thanks @gep13. It does!

[ChrisMaddock]

I've updated the plan above - it seems @CharliePoole and I are in agreement. If anyone else has thoughts - would be keen to hear them. Otherwise, I'll start writing code, as time allows.

[jnm2]

This sounds great! I do think that there are benefits to specially understanding target framework monikers and not looking through the contents of every folder. They might have different kinds of file in them; you wouldn't want to always load exactly one DLL no matter what.

[ChrisMaddock]

They might have different kinds of file in them; you wouldn't want to always load exactly one DLL no matter what.

Bit confused what you mean by this bit - can you give an example of the sort of thing you're thinking of? 🙂 The main downside I can think of not recognising tfm identifiers as directory names is just the inefficiency - hopefully negligable in the scheme of everything else the engine is doing.

My concern is marrying up identifying tfm directories with the current .addins functionality, which reads what it's pointed at. This currently could be one of:

• A directory, in which all assemblies are examined
• A assembly
• Another .addins file

If we start handing directories containing subdirectories differently, things feel like they get a little messy - potentially in a non-backwards-compatible way. (Although I'm sure niche enough not to worry!) What should happen if a dir has subdirs named after tfms? Does it then ignore all assemblies, and just look in the 'best' tfm dir? How about if some subdir's are recognised tfm's and others aren't? Or if there's assemblies in root _and_ tfm-subdirs - do we start ignoring the root assemblies? It just all feels a little unclear to me - where as 'examine everything' is a simple, understandable process to document and maintain.

[CharliePoole]

Seems to me there is a compatible way to do this without having special naming of the directories.

When we examine a directory for extensions, we first look for .addins files. If we find any, we process them and __ignore everything else in the directory__. If you were to couple this with a backward-compatible annotation (special comment, for example) in the first line of the directory, you could invent some way to signal when to use or ignore the .addins file based on target runtime.

[jnm2]

Yeah, I see. I'm thinking there may be extra DLLs in one folder, like AsyncBridge.dll for example. How would the engine decide which of the DLLs to load in that case? What would the .addins file look like when you need to load one DLL if net45-compatible, otherwise load two other DLLs?

[CharliePoole]

It depends on how the extension organizes things. Normally, a single extension has everything in a single directory, unlike our msi where we put everything into an addins directory. The extension Service is designed to handle both situations.

So, taking your situation and assuming you use two subdirectories to avoid name clashes...

xxx\
    xxx.addins
    one.dll
yyy\
   yyy.addins
   two.dll
   three.dll

where xxx.addins has

#@SpecialComment@ net45
one.dll

and yyy.addins has

#@SpecialComment@ someother.net
two.dll
three.dll

Alternatively, you could put all five files in one directory. It's up to the extension author.

BTW... I took you at your word that both two.dll and three.dll are extension assemblies. That seems odd to me. I think it's more likely that you would have one extension assembly in every case, possibly with some dependent assemblies. I can't even understand how you would put a single extension into two assemblies! Of course, you could have a package with multiple extensions.

It will be much easier to deal with this in the context of actual extensions, rather than theoretically.

[jnm2]

I took you at your word that both two.dll and three.dll are extension assemblies

I thought I said the opposite: there may be extra DLLs in one folder, like AsyncBridge.dll for example. In other words there may be non-extension dependencies for one target framework that should not be loaded for newer frameworks.

[CharliePoole]

I misunderstood then. There are often such dependencies. See the v2 framework driver for example. They are either ignored (if there is an addins file) or examined and found to contain no extensions.

[ChrisMaddock]

Mmm, we do already have a 'convention' for addins with dll dependencies - I don't see the need to change that as part of this issue. Sorry - I didn't realise that was what you were initially referring, Joseph.

My thinking was that the engine would continue to examine everything pointed at by the addins file as a potential extension. The engine looks for an NUnit.Engine.Extensibility.ExtensionAttribute to identify all potential extensions - and handles cases where no attribute is found appropriately. If the engine finds two extensions of the same id, it then compares to find the 'best' version - currently solely based off the extensions assembly version.

The above is all current functionality. My thinking is that we don't want to change any of that, and instead just extend the definition of 'best' to also look at 'highest target framework'. Other than that - we continue listing all potential extension assemblies to the engine (via the variety of different formats available in the addins file) - but let the engine resolve which version to use, as it currently does.

Adding new 'special comment' functionality is an option, however I don't see the need to complicate the addins file spec, when the engine can read target framework information directly from the assembly. (Which we already do to identify the target framework of test assemblies.) That feels a little bit like we're adding a human requirement on something - it may be useful as an override, but imo we shouldn't add that override until we have a usecase for it! 🙂

[CharliePoole]

@ChrisMaddock Of course, that has to be "highest target framework loadable under the current runtime."

WRT "special comment" it's an example of how you could add special handling per runtime without constraining the names of the directories. I agree we shouldn't add it unless there's a demonstrated need.

[jnm2]

it may be useful as an override, but imo we shouldn't add that override until we have a usecase for it!

This makes sense! Thanks, both of you!