-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Mono compatibility verification #2311
Conversation
Going to turn on Mono CI after this is merged into |
45d53ba
to
1083797
Compare
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 left actual //
comments in most of the code where needed to explain the Mono-specific platform changes. Otherwise most of the code changes I made were fairly innocuous and better than what we had before, even for Windows-only.
@@ -28,7 +28,7 @@ src\.nuget\NuGet.exe install FAKE -ConfigFile src\.nuget\Nuget.Config -OutputDir | |||
|
|||
src\.nuget\NuGet.exe install NUnit.Console -ConfigFile src\.nuget\Nuget.Config -OutputDirectory src\packages\FAKE -ExcludeVersion -Version 3.2.1 | |||
src\.nuget\NuGet.exe install xunit.runner.console -ConfigFile src\.nuget\Nuget.Config -OutputDirectory src\packages\FAKE -ExcludeVersion -Version 2.0.0 | |||
src\.nuget\NuGet.exe install NBench.Runner -OutputDirectory src\packages -ExcludeVersion -Version 0.3.0 | |||
src\.nuget\NuGet.exe install NBench.Runner -OutputDirectory src\packages -ExcludeVersion -Version 0.3.1 |
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.
Mono-friendly implementation of NBench runner.
// Filter out assemblies which can't run on Linux, Mono, .NET Core, etc... | ||
|
||
open Fake.EnvironmentHelper | ||
let filterPlatformSpecificAssemblies (assembly:string) = |
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.
Platform-specific filter for specs inside FAKE. Allows us to run the same FAKE file for multiple platforms and skip tests that can't run due to environmental issues on a particular platform.
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.
It's fine, but there's more elegant pattern (for the future):
let (|Contains|_|) pattern (assembly: string) = if assembly.Contains pattern then Some () else None
// usage
match assembly with
| Contains "Sqlite" when isMono -> false
| _ -> true
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.
Eh, I'll leave it as-is for now in case someone wants a test that isn't just .Contains
@@ -152,7 +152,7 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Akka.TestKit.Xunit", "contr | |||
EndProject | |||
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "DependencyInjection", "DependencyInjection", "{B1D10183-8FAE-4506-B935-403FCED89BDB}" | |||
EndProject | |||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Akka.DI.Core", "contrib\dependencyInjection\Akka.DI.Core\Akka.DI.Core.csproj", "{FDF09D18-B68E-4B95-B1F6-B89D9C6C3AE9}" | |||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Akka.DI.Core", "contrib\dependencyinjection\Akka.DI.Core\Akka.DI.Core.csproj", "{FDF09D18-B68E-4B95-B1F6-B89D9C6C3AE9}" |
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.
Had to fix capitalization of name or Linux builds fail.
@@ -4929,6 +4929,10 @@ namespace Akka.Util | |||
public override bool IsRight { get; } | |||
public TB Value { get; } | |||
} | |||
public class static RuntimeDetector |
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.
New class used to detect runtimes. Decided to unify all of our Mono detector implementations into a single static utility class.
@@ -1069,6 +1069,11 @@ namespace Akka.Remote | |||
void Remove(T resource); | |||
void Reset(); | |||
} | |||
public class static IpExtensions |
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.
Workarounds for IP mapping issues on Mono.
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.
Should this really be a part of public API?
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 can make it internal, since eventually we'll be able to do away with it.
@@ -21,7 +22,7 @@ public static Address ToAddress(this EndPoint endpoint, ActorSystem system) | |||
return new Address(AkkaIOTransport.Protocal, system.Name, dns.Host, dns.Port); | |||
var ip = endpoint as IPEndPoint; | |||
if (ip != null) | |||
return new Address(AkkaIOTransport.Protocal, system.Name, ip.Address.MapToIPv4().ToString(), ip.Port); | |||
return new Address(AkkaIOTransport.Protocal, system.Name, IpExtensions.MapToIPv4(ip.Address).ToString(), ip.Port); |
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.
Using our Mono-friendly IP mappers.
@@ -19,7 +20,7 @@ | |||
|
|||
namespace Akka.Streams.Tests.Dsl | |||
{ | |||
public class FlowAppendSpec : AkkaSpec | |||
public class FlowAppendSpec : Akka.TestKit.AkkaSpec |
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.
Fixed a compilation bug on Mono. Not sure why it was an issue to begin with.
var props = Props.Create<GuardianActor>(UserGuardianSupervisorStrategy); | ||
var cell = rootGuardian.Cell; | ||
cell.ReserveChild(name); | ||
var props = Props.Create<GuardianActor>(UserGuardianSupervisorStrategy); |
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.
The previous method we used here, which was a bit of a YAGNI thing to begin with, caused the mono runtime to randomly fail on some occasions due to an expression compiler bug.
cell.InitChild(child); | ||
child.Start(); | ||
return child; | ||
var props = Props.Create(() => new SystemGuardianActor(userGuardian), _systemGuardianStrategy); |
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.
Same as above.
@@ -165,25 +166,27 @@ private void AddLogger(ActorSystemImpl system, Type loggerType, LogLevel logLeve | |||
{ | |||
var loggerName = CreateLoggerName(loggerType); | |||
var logger = system.SystemActorOf(Props.Create(loggerType).WithDispatcher(system.Settings.LoggersDispatcher), loggerName); | |||
var askTask = logger.Ask(new InitializeLogger(this)); | |||
var askTask = logger.Ask(new InitializeLogger(this), timeout); |
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.
If the logging system failed to start, the task we issued would never be de-allocated. This code has been corrected to align with the JVM.
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.
In general everything is ok. Only one question to answer and one issue to be eventually created.
// Filter out assemblies which can't run on Linux, Mono, .NET Core, etc... | ||
|
||
open Fake.EnvironmentHelper | ||
let filterPlatformSpecificAssemblies (assembly:string) = |
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.
It's fine, but there's more elegant pattern (for the future):
let (|Contains|_|) pattern (assembly: string) = if assembly.Contains pattern then Some () else None
// usage
match assembly with
| Contains "Sqlite" when isMono -> false
| _ -> true
@@ -1069,6 +1069,11 @@ namespace Akka.Remote | |||
void Remove(T resource); | |||
void Reset(); | |||
} | |||
public class static IpExtensions |
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.
Should this really be a part of public API?
@@ -299,6 +300,9 @@ public void A_Flow_with_SelectAsync_must_handle_cancel_properly() | |||
[Fact] | |||
public void A_Flow_with_SelectAsync_must_not_run_more_futures_than_configured() | |||
{ | |||
if (IsMono) // TODO: 9/14/2016: Mono 4.4.2 blows up the XUnit test runner upon calling Thread.Interrupt below (@Aaronontheweb) |
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.
Is this issue noticed anywhere? If not, please create a ticket for it. This is not a good practice.
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.
And createa ticket on the Mono repository
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.
Pretty certain that this is an XUnit issue; it looks as though the Thread.Interrupt
call interrupts the XUnit worker thread too.
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.
1083797
to
ee02d4c
Compare
Updated everything in response to the feedback I received on the PR. |
Shit, forgot to update the approved API.... |
close akkadotnet#2254 close akkadotnet#2195 close akkadotnet#2194 close akkadotnet#1594 close akkadotnet#1110
ee02d4c
to
dfcb4f7
Compare
You have my blessing to merge that. |
close #2254
close #2195
close #2194
close #1594
close #1110