Multi-Targeting of engine extensions (.NET Framework only) #452

ChrisMaddock · 2018-08-18T13:45:39Z

This is some unfinished work from a few months back - excuse it being a bit random! Fixes #394 as far as is currently possible - if we're considering implementing extension functionality for the .NET Standard engine as a whole separate issue!

The engine will now prioritise the extension with the highest target framework - when two of the same type are made available. Preference order in determining which extension version to load is now:

Assembly Version -> Target Framework -> Found via wildcard/path directly specified

In the short term, this unblocks nunit/vs-project-loader#25 - something I might have a look at soon. In the longer-term - this is hopefully the foundations for resolving extensions made available for different platforms.

Again, I recommend review commit-by-commit - I'll add in a few line-comments shortly on some bits worth highlighting. The changes are less significant then they look - I did an amount of refactoring so we could test this better, as I imagine this functionality may become more complex over time!

…erence, refactor

…n console

ChrisMaddock · 2018-08-18T13:46:53Z

src/NUnitEngine/nunit.engine.tests/Extensibility/ExtensionAssemblyTests.cs

 using System.Reflection;
-using System.Text;
+using NUnit.Engine.Extensibility;
 using NUnit.Framework;



Significant changes in this file are mainly due to tests here split out into ExtensionSelectorTests

ChrisMaddock · 2018-08-18T13:47:56Z

src/NUnitEngine/nunit.engine/Extensibility/ExtensionAssembly.cs

@@ -0,0 +1,77 @@
+// ***********************************************************************


This file has been moved and isn't entirely new - I guess GitHub hasn't noticed as there's also some changes.

ChrisMaddock · 2018-08-18T13:49:41Z

src/NUnitEngine/nunit.engine/Extensibility/ExtensionSelector.cs

@@ -0,0 +1,75 @@
+// ***********************************************************************


Functionality in this new class mostly extracted from ExtensionAssembly.cs - some changes to consider targetframework. Refactor was for test purposes.

ChrisMaddock · 2018-08-18T13:50:48Z

src/NUnitEngine/nunit.engine/Services/ExtensionService.cs

+                {
+                    if (!fromWildCard)
+                        throw;
+                }


Additional check as we now attempt to read target frameworks in this section, which can throw.

ChrisMaddock · 2018-08-18T13:52:43Z

Tests are failing on mono-only, I'm going to guess some differences in assembly name reflection. Looking into it...

ChrisMaddock · 2018-08-18T14:43:19Z

Issue is that our RuntimeFramework code currently believes Mono 5.10.0.160 can't support .NET 2.0 targeted things. I think I might have to deal with this as a separate issue...

ChrisMaddock · 2018-08-18T15:19:57Z

Cool - an actual bug! 😄

Turns out RuntimeFramework.Supports() wasn't doing what I expected - I've added in a RuntimeFramework.CanLoad() instead.

Ready for review now!

ChrisMaddock · 2018-08-18T15:50:24Z

@rprouse - this is a non-trivial change, and could probably be merged post-release, if you'd prefer? I don't mind either way. 🙂

CharliePoole · 2018-08-18T15:52:51Z

src/NUnitEngine/nunit.engine/Services/ExtensionService.cs

@@ -432,7 +446,7 @@ public void FindExtensionsInAssembly(ExtensionAssembly assembly)
                if (versionArg != null && new Version((string)versionArg) > ENGINE_VERSION)
                    continue;

-                var node = new ExtensionNode(assembly.FilePath, type.FullName);
+                var node = new ExtensionNode(assembly.FilePath, type.FullName, assemblyTargetFramework);


Once we decide that we can load the assembly in the current runtime, why is the assemblyTargetFramework still needed in the extension node? Is it only so that the console runner can display it?

Yes, exactly. Info is only stored in ExtensionNode to be accessible to runners.

CharliePoole · 2018-08-18T15:58:38Z

src/NUnitEngine/nunit.engine/Properties/AssemblyInfo.cs

+    "48cd57a6")]
+
+[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2,PublicKey=002400000480000094" +


Did I miss where this is explained? Maybe a comment here would help.

Sorry - this is to allow using NSubstitute with internal types. It should have a comment, yes!

CharliePoole · 2018-08-18T16:09:05Z

I had to go back and re-read all the discussion, including my own comments, to try to remember why we were doing this! Restating what I think you are trying to accomplish in my own terms so you can tell me if I get it correctly... 😄

Currently, we can write extensions that target .NET 2.0. We ask that extension-writers do so in order to make them available to all users.
You'd like to write extensions that target higher framework versions and eventually .NET Core. This change takes care of the first and prepares for doing the second later on.

Where I am a bit confused is that I thought we already handled the first case. If you write an extension that targets say 4.5, then we don't load it unless we are running under 4.5 or higher. The existing extension service is supposed to take care of that and I'm pretty sure it once did.

So, are we doing anything new here? Or are we just preparing for eventual .NET Core extensions? IOW, what is the use case that currently fails but will succeed under this PR?

If that case is, for example, loading a 4.6 extension when running under 3.5, I'd like to figure out when it stopped working correctly - that is, ignoring the extension. At least I think it used to work that way!

ChrisMaddock · 2018-08-18T16:23:40Z

@CharliePoole - your understanding is correct. (Specifically, in my case right now, I want to write a net46 version of the vs-project-loader extension)

What's definitely new, is that the engine will now purposefully choose the highest target framework version of an extension - previously, with two equal assembly-version extensions, it would have just taken the first it encountered. That allows us to package multiple versions of the same extension in the same NuGet package, and ensure the user always gets the 'best' version available.

I don't believe there was any previous functionality at all to check if the current framework supports the target framework of the extension. As far as I can see, it would have crashed at the point it tries to load the assembly. I'd have thought that we previously haven't encountered that problem as all the extensions we look after are .NET 2.0. Whether any other extension authors have, I don't know.

Of course, if we start distributing none .NET 2.0 extensions, then that situation becomes much more likely, so I added in the check. 🙂

CharliePoole · 2018-08-18T16:30:12Z

My memory (which could easily be wrong) was that we caught the exception (from 4.6 for example) logged the error and ignored the assembly without crashing. I had to deal with it even though we didn't have extensions > 2.0 because there could still be assemblies in the same directory that targeted a higher version. In fact, at one point, we threw if there was a non-assembly in the same directory! However, this all may have been before we started using Mono.Cecil and you obviously have to deal with what we now do, not with what we used to do.

In any case, selecting the highest loadable version is a new thing that I hadn't thought about.

BTW, I assume you are dealing with the identical assembly being found twice? It's possible to set up connected .addins files in a way that the same directory is scanned twice and it's also possible to have two copies of the same assembly in different directories. In fact, that's what the original "duplicate" logic was about.

ChrisMaddock · 2018-08-18T16:58:27Z

Ah. So if there's an exception at the point of examining an assembly - then yes, we would catch and continue (or error, circumstance depending) - adding that was my first ever contribution to NUnit, in fact! 😄 I had thought however that it would be possible to examine an assembly targeting a higher framework without crashing...but I'm not sure. The particular case I fixed was an exception on examining an unmanaged dll.

BTW, I assume you are dealing with the identical assembly being found twice?

I didn't change existing functionality here - if there's no differentiating factors two assemblies, the engine chooses the first it encounters.

CharliePoole · 2018-08-18T17:44:34Z

Then it all makes sense to me. Nice work!

CharliePoole

Looks Great now!

rprouse

Looks good...

ChrisMaddock added 5 commits August 18, 2018 14:34

Add TargetFramework to ExtensionAssembly, consider for extension pref…

3ad6148

…erence, refactor

Handle Extensions which can't be loaded under current target framework

ae8b52c

Expose RuntimeFramework through ExtensionNode interface and display i…

2fbe6a5

…n console

Refactor/Add tests

a9331ac

Misc formatting/copyright improvements

bd396e7

ChrisMaddock commented Aug 18, 2018

View reviewed changes

ChrisMaddock mentioned this pull request Aug 18, 2018

build-mono-docker.ps1 fails to run out the box #453

Closed

ChrisMaddock force-pushed the issue-394b branch from 13eb7ab to d37abbe Compare August 18, 2018 14:35

ChrisMaddock changed the title ~~Multi-Targeting of engine extensions (.NET Framework only)~~ [WIP] Multi-Targeting of engine extensions (.NET Framework only) Aug 18, 2018

ChrisMaddock added 2 commits August 18, 2018 16:11

Add RuntimeFramework.CanLoad() method

ec6c877

Extension service to consider "CanLoad" rather than supports()

bd35170

ChrisMaddock force-pushed the issue-394b branch from d37abbe to bd35170 Compare August 18, 2018 15:13

ChrisMaddock changed the title ~~[WIP] Multi-Targeting of engine extensions (.NET Framework only)~~ Multi-Targeting of engine extensions (.NET Framework only) Aug 18, 2018

ChrisMaddock mentioned this pull request Aug 18, 2018

Misc improvements to ExtensionServiceTests #454

Merged

CharliePoole reviewed Aug 18, 2018

View reviewed changes

Add comment

c670356

CharliePoole approved these changes Aug 18, 2018

View reviewed changes

ChrisMaddock added the awaiting:review label Aug 21, 2018

rprouse approved these changes Aug 25, 2018

View reviewed changes

rprouse merged commit e355877 into master Aug 25, 2018

rprouse deleted the issue-394b branch August 25, 2018 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-Targeting of engine extensions (.NET Framework only) #452

Multi-Targeting of engine extensions (.NET Framework only) #452

ChrisMaddock commented Aug 18, 2018

ChrisMaddock Aug 18, 2018

ChrisMaddock Aug 18, 2018

ChrisMaddock Aug 18, 2018

ChrisMaddock Aug 18, 2018

ChrisMaddock commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 •

edited

Loading

ChrisMaddock commented Aug 18, 2018

CharliePoole Aug 18, 2018

ChrisMaddock Aug 18, 2018

CharliePoole Aug 18, 2018

ChrisMaddock Aug 18, 2018

ChrisMaddock Aug 18, 2018

CharliePoole commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 •

edited

Loading

CharliePoole commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 •

edited

Loading

CharliePoole commented Aug 18, 2018

CharliePoole left a comment

rprouse left a comment

		@@ -0,0 +1,77 @@
		// ***********************************************************************

		@@ -0,0 +1,75 @@
		// ***********************************************************************

		"48cd57a6")]

		[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2,PublicKey=002400000480000094" +

Multi-Targeting of engine extensions (.NET Framework only) #452

Multi-Targeting of engine extensions (.NET Framework only) #452

Conversation

ChrisMaddock commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 • edited Loading

ChrisMaddock commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharliePoole commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 • edited Loading

CharliePoole commented Aug 18, 2018

ChrisMaddock commented Aug 18, 2018 • edited Loading

CharliePoole commented Aug 18, 2018

CharliePoole left a comment

Choose a reason for hiding this comment

rprouse left a comment

Choose a reason for hiding this comment

ChrisMaddock commented Aug 18, 2018 •

edited

Loading

ChrisMaddock commented Aug 18, 2018 •

edited

Loading

ChrisMaddock commented Aug 18, 2018 •

edited

Loading