-
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
Fix for #3402. Allows spawning an actor via expression. #3667
Conversation
@@ -374,14 +374,15 @@ module Linq = | |||
|
|||
let toExpression<'Actor>(f : System.Linq.Expressions.Expression) = | |||
match f with | |||
| Lambda(_, Invoke(Call(null, Method "ToFSharpFunc", Ar [| Lambda(_, p) |]))) |
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.
Nice catch, so this looks like this is what was causing the trouble @chrisjhoare ?
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.
Yeah I think the previous signature of this got changed/removed in the move to netstandard and added back in some form from F# 4.5 onwards - but to be honest its a little beyond me. But this does appear to allow the actor to be spawned now.
Hard test failure caused by these changes in Akka.FSharp |
The test that's failing doesn't appear to be related to these changes. #3407 added this test (the F# serializer and the cluster pub serializer have the same Identifier - 9). I'm not sure #3404 is fixed as these two still have the same identifier as far as I can see. I could probably fix this in the PR by giving the F# one another number e.g. 99 ? I'm not sure how the id range is determined though? |
@@ -29,7 +29,7 @@ module Serialization = | |||
type ExprSerializer(system) = | |||
inherit Serializer(system) | |||
let fsp = FsPickler.CreateBinarySerializer() | |||
override __.Identifier = 9 | |||
override __.Identifier = 99 |
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.
Good call
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.
Approved. Going to get this in as part of 1.3.11
Build stage failed because connection to Github was disrupted when TeamCity was fetching latest patch changes. Going to re-run it. |
Previous test stages failed earlier because I had changed some settings around on the build server in order to support #3668 - changed those back so we can run specs under the current build system |
This fixes the exception when spawning from an expression using the FSharp api. This did require bumping the FSharp version to 4.5 to get it work though.