Add .NET Core 3.1 build of engine to access APIs for loading .NET Core assemblies correctly #778
Conversation
| MSBuild(ENGINE_CSPROJ, CreateMSBuildSettings("Publish") | ||
| .WithProperty("TargetFramework", framework) | ||
| .WithProperty("PublishDir", BIN_DIR + framework)); | ||
|
|
||
| foreach(var framework in NETSTANDARD_FRAMEWORKS) | ||
| foreach(var framework in new [] { "netstandard1.6", "netstandard2.0" }) | ||
| MSBuild(ENGINE_API_CSPROJ, CreateMSBuildSettings("Publish") | ||
| .WithProperty("TargetFramework", framework) | ||
| .WithProperty("PublishDir", BIN_DIR + framework)); |
There was a problem hiding this comment.
The NUnit Engine API remains targeting netstandard2.0 only - the NuGet packaging ensures the correct Engine version is used.
I think having a .NET Standard assembly make use of a .NET Core assembly is a bit of a code smell, but I also think we have a larger issue to consider re: the API assemblys use of reflection to load the engine.
| var assemblyName = typeof(NUnit.Framework.TestAttribute).Assembly.GetName(); | ||
| Assert.That(new NUnit3FrameworkDriver(AppDomain.CurrentDomain, assemblyName), Throws.ArgumentException); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is unrelated, but this test was missing its TestAttribute so not running. The test is no longer valid.
| #if NETCOREAPP2_1 | ||
| Assembly netstandard = typeof(ExtensionService).Assembly; | ||
| #if NETCOREAPP | ||
| Assembly netstandard = Assembly.LoadFile(Path.Combine(GetSiblingDirectory("netstandard2.0"), "nunit.engine.dll")); |
There was a problem hiding this comment.
These tests were using the executing engine as an example .NET Standard assembly - which of course caused problems when that assembly is build for .NET Core. I changed them to fetch the .NET Standard build specifically, in the same way the other tests in this class do.
Ideally, I think we need a set of test assemblies for this. (Or perhaps to use mock-assembly?)
| @@ -3,7 +3,7 @@ | |||
| <OutputType>Exe</OutputType> | |||
| <RootNamespace>NUnit.ConsoleRunner</RootNamespace> | |||
| <AssemblyName>nunit3-console</AssemblyName> | |||
| <TargetFrameworks>net20;netcoreapp2.1</TargetFrameworks> | |||
There was a problem hiding this comment.
The console is as-yet unreleased, but will now target .NET Core 3.1 as a minimum. With the long-term aim of making it self-contained anyway, I'm not too worried about this.
|
Just added the blocked label, as I'll need to rebase this over #779 once that's merged, and make the same change for the new .NET Core 3.1 build. That's only a minor change however, the rest is still ready for review. 🙂 |
|
Reverting to draft until I can figure out what's causing the test failures seen in #781 |
ChrisMaddock
added
awaiting:contributor
Blocked
and removed
awaiting:review
Blocked
labels
ChrisMaddock
force-pushed
the
issue-710a
branch
3 times, most recently
from
33a4bdd
to
8d766e2
Compare
|
Ok - I'm happy this is the right approach now and everything's working - I'm just going to tidy up the commits, and it'll be ready for review. 🙂 |
ChrisMaddock
force-pushed
the
issue-710a
branch
2 times, most recently
from
05a22cc
to
5ae3e0b
Compare
|
Thanks Joseph. 😄
Yeah, that bit is in #781. 🙂 It's depends on three other open PRs, so I'll rebase and tidy up once all those are merged. Looks like there's a little more battle required yet, to get .NET Core 1.1 running on Azure. Might have to come back to that later, will sort your feedback at the same time. Thanks! |
|
So the old Azure image won't build .NET Core 3.1, and the new one won't build .NET Core 1.1. 😤 Guess this is a task for another day... |
|
Just a suggestion and I might be missing some context, but how sure are we that .NET Core 1.1 is worth building for? It's past end of life. |
|
Yeah, I'm inclined to agree, see #725. It's too much of a timesink for the benefit, in my opinion. I just feel like that should be done separately and intentionally, rather than taken out because something else broke it - but it does pull in to question how much of my time it's worth investing in trying to get it back up and running again... |
|
It's a little frustrating, as it would be a breaking change for runner-maintainers...however I expect the number of actual end users is critically small. |
ChrisMaddock
force-pushed
the
issue-710a
branch
6 times, most recently
from
f02fe38
to
79dbcd4
Compare
|
Finally got CI working again. Thanks Joseph for reviewing this. 🙂 As it's a significant change, I'll just leave the PR up a few more days before merging, to give anyone else on the team the chance to raise any concerns. |
To sort #710 properly, we need to make use of the AssemblyLoadContext APIs, which are only available in .NET Core 2.1 and above.
In our 'full vision', this functionality would live in a .NET Core specific agent, and not require a full build of the engine. However, I don't want to delay the .NET Core Console further while we work on .NET Core agents - and I consider #710 a major enough bug that I don't want to release the .NET Core Console without a solution.
Accordingly, this PR adds a .NET Core 3.1 build of the engine. We use v3.1 specifically to make use of the
AssemblyDependencyResolverclass, which was first available in .NET Core 3. (Of which, 3.1 was the first LTS release). There are workarounds for earlier frameworks - however my priority now is getting the latest version of .NET Core working. If we end up with separate agents for older versions of .NET Core, it would be possible for someone to look at adding the workarounds into those - if there's sufficient demand.I expect this build to be a temporary build, until we have .NET Core agents, at which point we can remove it and go back to just packaging .NET Standard 2.0. I can't think of any way that could be a breaking change, on the basis that any .NET Core 3.1 runners can consume the .NET standard package instead?
For ease of review, this PR just includes adding the .NET Core 3.1 build. The actual fix will be in a second PR.