Skip to content
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

.NET Core 2.1 #1650

Closed
megakid opened this issue Jun 3, 2018 · 20 comments
Closed

.NET Core 2.1 #1650

megakid opened this issue Jun 3, 2018 · 20 comments
Labels
detectability/hard Negative impact will be hard to detect impact/high High prospect of negative impact platform/.NET Core Issues relating to ES on .NET Core

Comments

@megakid
Copy link
Contributor

megakid commented Jun 3, 2018

I don't want to do a PR for this as there is clearly a lot of considerations to be discussed and debated. I spent a few hours yesterday getting a working .NET Core 2.1 build - I used Jetbrains Rider 2018.1.2 but looks to be buildable from VS2017 as well.

https://github.com/BlizardPuppet/EventStore/tree/netcore21

Some of my initial changes/observations/questions:

  • The big API gap was UrlTemplate - Copied it verbatim from .net framework source with very small edits (e.g. delocalization of Exception messages) - probably needs a clean up
  • Converted all EventStore.sln csprojs to the new VS2017 format - this mean dropping < VS2017 support
  • Using package references (nuget) instead of checked-in lib dlls (upgrades necessary for .NET Core support include protobuf-net 2.0.X -> 2.2.X, Newtonsoft.Json 6.X -> 11.X, both didn't require any code changes)
  • Some minor .csproj tweaks to ensure it works nicely for multitargetting .NET Core + net46.
  • Fixed up FlushSafe to work across .NET Core on Windows and Full Framework builds - it seems FileStream now exposes the SafeFileHandle instead of having to use reflection to find it.
  • Added new 'dotnet' "visual studio version" to build.ps1 script inc auto-detect. We can probably drop < VS2017 support for the C# code but unsure about the js1 build - personally, I have never built this.
  • dotnet version smoke tested inc js1 usage in projections. Joined a cross-build cluster (2 nodes dotnet, 1 node net46 - manual smoke testing passed)
  • Unit tests need looking at, not sure why they are not runnable using standard IDE - I think aim should be to get them running under dotnet test
  • What do we need to do about Mono support? Can we drop it (eventually) if this path is chosen?
  • haven't tested anything else other than build.cmd quick and build from Rider (windows)
  • Need to look at building + running under Linux
@gregoryyoung
Copy link
Contributor

gregoryyoung commented Jun 3, 2018

Unit tests should be runnable, they are runnable with nunit on mono without issue. What problems are you running into?

"What do we need to do about Mono support? Can we drop it if this path is chosen?"
I won't comment on the rest but mono support will be there for a while at least. Show me 6-12 months of dotnetcore running successfully in varying environments and I will consider depreciating mono. As of now the mono packages are quite solid and have run many 1000s of hours in production environments, I won't throw that away because we suddenly run on dotnetcore without any observational data from production environments.

@jen20
Copy link
Contributor

jen20 commented Jun 4, 2018

Hi @megakid! I've had a look at your repo, this definitely looks like promising work.

.NET Core is something we've been interested in for a while, and @jageall has done a lot of work around porting (including the pre-1.0 days when attempting to build led to something like 700k build errors...)

There are a few things that are important in the list of things there that I think will need more attention:

What do we need to do about Mono support? Can we drop it (eventually) if this path is chosen?

We cannot drop Mono support any time soon. Apart from the fact that it's in production in a lot of places, .NET Core does not yet appear to offer an equivalent of mkbundle which allows us to ship statically linked binaries for a variety of Linux distributions. We are unlikely at this point to want to pick up the support burden of not having the runtime statically linked.

Using package references (nuget) instead of checked-in lib dlls

We need to be able to build any commit of Event Store without relying on a third party (including NuGet) - so we need to find some way of committing all of the necessary dependencies to the repository. I don't know enough about .NET Core to know what is common practice there, but we deliberately take very few external dependencies so it shouldn't be too bad.

Unit tests need looking at, not sure why they are not runnable using standard IDE - I think aim should be to get them running under dotnet test

A large part of the work that @jageall did was to port the unit tests from NUnit to XUnit, which I still probably think is a reasonable path. They will definitely need to run via dotnet test for CI purposes, but it would be nice if they also ran under Rider and whatever other IDEs people are using (I think @shaan1337 and others are using VS Code?

Copied [UrlTemplate] verbatim from .net framework source

This will need investigation as to the licensing implications.

@megakid
Copy link
Contributor Author

megakid commented Jun 4, 2018

Thanks for looking it over- very early days but appreciate the assistance.

I don’t know enough about Mono or mkbundle but does this bundle up the C# IL with the native js1 lib? Or if it’s just the js1 you’re talking about, could we separate the js1 build/compilation to a different repo/project/solution?

We can definitely check in the dependencies, I’m thinking we could have a NuGet.config that points to a NuGet feed located within the repository - needs testing.

Since updating the NUnit test runner to the latest version, I’ve had more success running them within Rider & Visual Studio but it’s interesting you say converting them to xUnit as it seems much more mature in the .NET Core world. I wrote a NUnit to XUnit project converter so could run that over the test projects if that is something that would be accepted.

Licensing did cross my mind when porting UrlTemplate - there are a bunch of 3rd party libs which maybe complete enough to use instead of this.

Sometime down the road if we’re on .NET Core, there should be some nice performance improvements waiting for us, I’m thinking in particular Span<T> usage as well as general speed ups (this series- https://blogs.msdn.microsoft.com/dotnet/2018/04/18/performance-improvements-in-net-core-2-1/)

@gregoryyoung
Copy link
Contributor

mkbundle releases a full binary (including the runtime etc)!

@megakid
Copy link
Contributor Author

megakid commented Jun 4, 2018

mkbundle releases a full binary (including the runtime etc)!

Nice, dotnet publish does runtime bundling (obviously at this point you need to target a platform/runtime e.g. ubuntu.16.04-x64) but not sure it can do it to a single file. But you can definitely have a single folder that is self-contained (inc runtime + native dlls).

@gregoryyoung
Copy link
Contributor

yes its a single native binary that gets released. Try this: pull the ubuntu package of eventstore and look what gets installed.

@latop2604
Copy link

CoreRT is what you need to replace mkbundle but I think it's only a preview for now

@bklooste
Copy link

bklooste commented Jul 1, 2018

I spent half a day on Core 2.0 in parallel.. as I am looking at running it on Android. Its compiling and tests are running but a few issues.. See below.. I just spend 20min writing a new post for some help but chrome died on me, so this will be more brief,

First comments on original.
Some of my initial changes/observations/questions:
The big API gap was UrlTemplate - Copied it verbatim from .net framework source with very small edits (e.g. delocalization of Exception messages) - probably needs a clean up

BK> I copied it from Mono since that was being used already.

Converted all EventStore.sln csprojs to the new VS2017 format - this mean dropping < VS2017 support
Using package references (nuget) instead of checked-in lib dlls (upgrades necessary for .NET Core support include protobuf-net 2.0.X -> 2.2.X, Newtonsoft.Json 6.X -> 11.X, both didn't require any code changes)
BK> Mostly the same and moved most to nuget.
Some minor .csproj tweaks to ensure it works nicely for multitargetting .NET Core + net46.
BK> Moved it all to netstandard 2.0 and duplicated the ClusterNode project to create a coreapp version. Tests and Cluster node were set to 461

Fixed up FlushSafe to work across .NET Core on Windows and Full Framework builds - it seems FileStream now exposes the SafeFileHandle instead of having to use reflection to find it.
BK>Not done..

Rest op not done..
I did get all the tests working in VS and command line in build via dotnet vstest
Bumped the histogram project which had some changes.

https://github.com/bklooste/EventStore

I got issues around http returning not found all the time looks like its around routing / httplistener which isn't the fastest area to debug.. Im comparing to @megakid atm and will keep people posted.

@bklooste
Copy link

bklooste commented Jul 1, 2018

"We need to be able to build any commit of Event Store without relying on a third party (including NuGet) - so we need to find some way of committing all of the necessary dependencies to the repository. I don't know enough about .NET Core to know what is common practice there, but we deliberately take very few external dependencies so it shouldn't be too bad."

This is a big issue because the key concept of core is a minimal framework and delivers the rest through small nugets eg ComponentModel , System.Http , even the compiler parts (Microsoft.Csharp) etc etc - think js-npm /go / rust cargo/ C includes in a large project. What is even worse is nuget packages are no longer just dlls but have scripts they run , the good thing is it solves most of the 64bit/32 bit dll pain eg libuv . You can however build against specific versions of the nuget package but your no longer looking at 4-5 externals but a few hundred each with its own dependencies - I don't think dropping a dll and dealing with its dependencies is manageable by a person without tools.. See attached

image

If its that critical you will need to setup your own nuget server , I have seen some companies do this ( we do it for our own stuff but we trust the Microsoft rep)

Also suggest get the basics / minimal set working well before nice to haves like xunit , perf improvements, Kestrel , pulling out mono , scripts changes etc. Don't want to make it bigger than it already will be . Obviously if your most of the way with xunit

@latop2604
Copy link

We can still use nuget for framework dependencies (eg:micrisoft) and use a local directory for other nuget package.

@megakid
Copy link
Contributor Author

megakid commented Jul 1, 2018

@bklooste nice work - I am happy to help out if you have a list of outstanding tasks?

@bklooste
Copy link

bklooste commented Jul 2, 2018

@megakid I saw your work with ServceModel it works.. The mono one I tried is less coupled just 2 files but does not work.. We can start with the ServiceModel classes and later look at the mono one. Core has tons of stuff around routing which someone will likely look at in future.

@latop2604 yes you can there is only a few things outside of there but note the version you use must be compatible with the Microsoft libs you pull in..so it does mean bumping version of externals a bit more , especially initially..

I don't have a huge amount of time and a bit new to the project , but we need

  1. Agreement core is a direction the projection will go in eg v5 , or will it be a fork EventStore.Core
  2. A branch to work on ..
  3. Some place to talk , is there a slack channel?
  4. Agreement on a path..If Core is "v5" then a small number of small releases . Suggest netstandard 2.0 for safe libs and project conversion first without touching anything else .. We can verify how this works with mono.. If a fork we can take bigger bites.

@bklooste
Copy link

bklooste commented Jul 2, 2018

Assuming v5 approach

v1 Core Milestone- Smallest bite

  • Project Conversion to Vs2017
  • Upgrade All projects to net 4.61
  • Change transport and Buffer projects to netstandard
  • Upgrade Json.net
  • Ensure build works for mono. Needs Mono 5.4.0
  • Make sure all tests pass and run in VS 2017 , command line and via dotnet vstest
  • Release Beta ?

v2 Core Milestone - Move key store functionality accross
Change EventStore.Core and Common projects to netstandard2.0
Sort out Service Model dependency , suggest use megakids port and improve later ( it may disappear if we bring in Core Mvc routing) . We could also fix the mono code as it has lower dependencies and should be what mono is currently using..

  • Release

v3 Core Milestone - Core release

  • Move all non entry non test project Projections to netstandard , projections means looking at build with js1.dll
  • Move Clusternode to compile as coreapp2.1 and net461.
  • Repackage embedded nuget package as netstandard2.0
  • Revisit build scrips.

Then we can make separate improvements .. eg

  • Core allows you to get much closer to native than mono/.net but Analysis of key points to improve should be done first.. I'm sure the older hands are familiar with them.
  • Core logging abstractions , its a pain having to write a Nlog to core adapter.
  • Kestrel web host
  • MVC core routing.
  • Reduce GC and allocations eg passing System.ValueTuple instead of objects
  • Ahead of time compiled Linux and Arm Android variant ( that's my interest)
  • Improved System.Memory.dll, System.Threading.Channels.dll, and System.IO.Pipelines.dll. For better performance. eg Use a Memory for chunks .. (rent or mutate) . i.e.
public void Worker(Memory<byte> buffer);
private static MemoryPool<byte> pool = MemoryPool<byte>.Shared;
public void UserCode()
{
    using (IMemoryOwner<byte> rental = pool.Rent(minBufferSize: 1024))
    {
        Worker(rental.Memory);
    }
    // The memory is released back to the pool
}

@bklooste
Copy link

bklooste commented Jul 2, 2018

That smallest bite is the smallest coherent milestone I can think of it has quite a bit of risk still due to all the versions bumps ( eg small http handling and json changes ) ..but will provide a solid platform to move forward from.. note its not even using Core ( eg its net461 ) but it is using the newer msbuild tooling, project files and shared libs. You could do a bump to 4.61 first.

A pull for that should not take long verifying the mono parts will be the biggest issue. A PR for v1 Core Milestone- Smallest bite would take a few hours .,

@bklooste
Copy link

bklooste commented Jul 3, 2018

@latop2604 CoreRT is a bit different than mkbundle ( which is just mono + code) as it removes the jit/.net its fully AOT compiled using .NET native..

To get a self contained exe like mkbundle ( sans native libs like js1.dll)
dotnet publish -c Release -r win10-x64
dotnet publish -c Release -r osx.10.11-x64

See https://ianqvist.blogspot.com/2018/01/reducing-size-of-self-contained-net.html ( make 55Meg Hello world 14 Meg) . This is how you make a binary for Android as well.

@IvanFarkas
Copy link

@megakid @latop2604 Thank you very much for your contributions.
Please let me know how I can help the transition to .NET Core 2.1.1 or perhaps a preview.

@bklooste Thank you very much for handling this with an open mind.
Current customers are important. Future .NET Core clients might outnumber your current base.
I like the fork idea a lot!

@bartelink
Copy link
Contributor

bartelink commented Jul 14, 2018

@IvanFarkas regarding the client part, #1664 (comment) may be of interest

@IvanFarkas
Copy link

Thx @bartelink. Multitarget is great.
Since .NET Core is multi-platform and multi-architecture already and outperforms Legacy .NET Framework by far, that would be my focus.

@bartelink
Copy link
Contributor

@megakid I think with 5.x released, this can be closed now ?

@shaan1337 shaan1337 added platform/.NET Core Issues relating to ES on .NET Core impact/high High prospect of negative impact detectability/hard Negative impact will be hard to detect labels Mar 8, 2019
@mat-mcloughlin
Copy link
Contributor

Closing this issue in favour of #1716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
detectability/hard Negative impact will be hard to detect impact/high High prospect of negative impact platform/.NET Core Issues relating to ES on .NET Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants