Support dotnet core
Closed this issue · 21 comments
Hello, I'd like support for dotnet core, and I am willing to help achieve this goal.
Before I do anymore work I obviously want to know, if I help do this, that it will be pulled into the project.
I have had a good look through the codebase, and I believe I have identified the major blocks for moving to core.
Proposal
Convert projects to PCL, still using csproj. This is so we maintain assembly signing, the build should work as it does now, and the work is more minimal than trying to convert to project.json. Infact I have already changed the csproj files here.
There are a few stumbling blocks to getting the code base on core.
We will have to update the Nlog, and Log4Net nuget packages, but its only minor version updates.
System.Threading.Thread does not exist in core land, so any uses of thread will have to be changed to System.Threading.Task. Tasks can be long running with Task.Factory.StartNew(()=>{} , TaskCreationOptions.LongRunning);
[MethodImpl(MethodImplOptions.Synchronized)] also does not exist in core, but could be swapped out for a lock statement (if memory serves me correctly).
System.Timers does not exist in core, but System.Threading.Timer does. However System.Threading.Timer is ironically not thread safe so when we swap them out, we will have to be careful to produce proper thread safety.
System.Net.Mime does not exist in core, but the only usage in the code base is for MediaTypeNames which could be replaced with our own string constants
👍 This would be super helpful as we are undergoing more and more dotnet Core development
Great stuff @TerribleDev, thanks for taking a first look. We would definitely support this with review and help testing, and would be happy to ship and updated package targeting core.
I used to work heavily in the .NET space and could keep up with all the new incarnations of portabl/core/etc, but at this point I have serious cobwebs.
Would you propose moving entirely to PCL and/or Core? Or simply adding an additional target output in our package, while maintaining desktop versions? Any thoughts on the new .NET Standard hotness I've heard about?
@latkin ok so this is a complex question in the dotnet space to be honest.
There are a few ways you can target multiple projects:
Shared code files
You can have multiple csproj files that compile down a dll for each platform (nuget moniker) you wish to that place their compiled outputs into a folder structure that is like /bin/<target>/MyProject.dll so for example dotnet 4 would be /bin/net40/MyProject.dll, which is then picked up by nuget to target specific platforms. Typically you use the shared files, or shared projects to make this happen. The big downside to this is the maintenance of a lot of csproj files.
PCL's
We all know what these are about, basically profiles that target specific runtimes. I bolded runtimes, because if you believe your project would be consumed by another class library then in all likely hood netstandard is the way to go.
Essentially with PCL's you can target win phone, dotnet core 1.0, dotnet framework 4.5, etc.
The big win for PCL's generally is how little change we have to make to the csproj to get it working. Which is critical for this project since all the CI stuff is baked into msbuild.
Net Standard
Net standard is basically a standard set of api, that once runtimes implement, the package will automatically work. The nice part about net standard is, if another framework comes out tomorrow that targets netstandard 1.4, and your package targets 1.4 then everything should just work. As of this writing, I know no way to target netstandard in a csproj file. If you havn't heard, you use a project.json file to declare everything about your app. This breaks the msbuild model dramatically, and things like setting targets for builds and what not don't really work out. For most projects its not a big deal, but since this project has big use of csproj/msbuild this causes some issues.
Now it was announced a little while ago that project.json will be replaced by csproj once again, so if we just have patience eventually this project could target netstandard, but since this is mostly used by apps that target runtimes it probably won't matter all that much (unless you want logging on xbox 2, or ).
ok so what will work for this project
Well anything really would work for this project. Porting to project.json is going to be a headache to re-do the build, and since project.json will go away for csproj again, its likely to be duplicated effort for no reason.
Shared projects are hard to manage. The biggest benefit is the ability to do compiler directives in code . For example we could add async methods to net45 but keep sync for 40, etc. lots of things that probably don't matter for this project
PCL's are not perfect, but they will get us what we need in the short-to-mid term. Basically, a target to netcore 1.0, while maintaining existing csproj things. Once netstandard can be used by csproj files, I'm sure retargeting to netstandard will be a smart move,
Thanks for the detailed writeup. I agree we don't want to go down the project.json path, and as a general principle we want to minimize the churn/refactoring needed in code and build.
So PCL/netcore seems reasonable to me. If you start a PR we could certainly help.
@latkin I have ran into a problem. I didn't notice, or realize that System.Console is being used in this project.
System.Console is not available in PCL projects, and IMO shouldn't be used in the appenders anyway. We can either wait until project.json goes away for csproj, or just keep going down the PCL road.
Considering both nlog, and log4net have targets to console could we remove this reference? I would assume that would mean a major version increment though.
Hi @TerribleDev apologies that this slipped over the past weeks.
I'm ok with dumping the Console output for portable libraries, or whatever new targets we are introducing. However for compatibility's sake I'd prefer to keep them in place for desktop targets. Can we do a simple #ifdef? Thanks again for your work on this.
@latkin We can't do that in portable libs....since the csproj system is close to being released (vs 17 is in RC right now with that tooling set) I think we should just wait for it to come out, and instead of going portable lib route (which made sense in November) just port the existing csproj's over to the new csproj system. Less of a breaking change, would allow #ifdef's and its not far to wait...
Thoughts?
Why can't we do that for portable libraries?
I do not want to change the build so that it is only compatible with VS 2017. This is a fairly simple library project and we should aim to keep it compatible with older versions.
I would vote to abandon porting for PCLs since the new standard is targeting .NET Standard (no pun intended). Log4Net has released their update targeting .NET Standard and NLog is in beta.
In order to not wait until the new csproj structure is in place, I proposed pull request #23 that uses the existing project.json structure. It will be an easy port once csproj updates are released. This approach is similar to how others like, Log4Net and MongoDB, have ported their .NET libraries for .NET Standard, thus allowing developers to target .NET Core.
@TerribleDev I have a few comments on your initial post.
System.Threading.Thread does not exist in core land, so any uses of thread will have to be changed to System.Threading.Task. Tasks can be long running with Task.Factory.StartNew(()=>{} , TaskCreationOptions.LongRunning);
System.Threading.Thread is targeting .NET Standard 1.3 and will available for use as is. See the nuget package here
[MethodImpl(MethodImplOptions.Synchronized)] also does not exist in core, but could be swapped out for a lock statement (if memory serves me correctly).
You are correct and has been updated here
System.Timers does not exist in core, but System.Threading.Timer does. However System.Threading.Timer is ironically not thread safe so when we swap them out, we will have to be careful to produce proper thread safety.
I believe this should be fine as there should only be one Timer used in a buffered log target
System.Net.Mime does not exist in core, but the only usage in the code base is for MediaTypeNames which could be replaced with our own string constants
Agreed. System.Net.Mime will be coming with .NET Standard 2.0, however the use and dependency is overkill in this situation. "text/plain" wont be changing anytime soon :)
@latkin you can't do an #ifdef in a pcl since the artifact of a pcl build is a single dll that is cross platform. This is talked about on this stack overflow post. Doing #ifdef's implies that we would have a build for .net full and a build for netcore which is 2 seperate projects, thus not a "portable" artifact.
If we want compatibility with older VS versions then AFAIK pcl might be the better supported route long term since project.json is going away, but we would have to abandon Console.Write's (or do crazy reflection back flips to just console out).
@billpratt You are right about System.Threading.Thread
The problem here is that there are alot of CI code in this repo that is setup to use the msbuild/csproj system, so porting to project.json is non-trivial, and a lot of work for something that will be abandoned.
Also, PCL is going to be around for good, but the future of project.json is less clear. Obviously NetStandardLibrary would be a better option as its not runtime specific, but if @latkin wants this repo to work across many older vs versions then we may have no choice to use PCL
Here's a high level summary of where we need to go:
- We need to continue shipping a .NET desktop version, with full binary compat (i.e. no breaking changes to public API or behavior)
- Devs should be able to work with this code and build this with VS 2015
- I would love to ship additional versions of the library which target the newer .NET variants or portable platforms
- Breaking API changes or unavoidable behavior changes are acceptable here
- If we need to wait for VS 2017 to ship in order to get proper tool support, that's ok with me
All of above should be achievable with a single codebase, and perhaps even a single solution. If types need to be changed or features taken away to support .NET Standard, then this can be done within #ifdefs so that the .NET desktop build can remain compatible.
@latkin Understandable. With that said, besides the locking PR submitted in #27, are you proposing not moving forward with the additional changes I submitted as separate PRs? These changes will work with VS2015 and .NET full framework. For .NET Core, there is a separate solution and projects that hook into the full framework projects and allow you to build against .NET Standard and .NET Core. VS2017 and .NET Standard 2.0 release dates are coming, but I believe they are unknown at this time.
The company I work for heavily depends on Sumo Logic and we would love to start using .NET Core.
@billpratt I've merged one of your PRs and am happy to merge the other, just had a small comment.
We can move forward with adding .NET standard support any time, as long as devs with VS 2015 can still build and test the .NET desktop code, and the desktop binaries remain fully compatible.
I am still not clear on why a totally new solution or projects are required, but maybe that will be made clear once I see the proposed changed. In my head I assume we can simply do this with the standard TargetPlatform kind of thing, which is all done with a single solution/project.
@latkin I'm finding it a bit tricky at the moment to get the current csproj projects to target .NET Standard and it was easier to do it as a separate solution for PoC. I'm trying out a possible solution and will response on this thread.
Sounds good. Thanks again for bearing with me and spending time on this, it's really appreciated.
Seems like we are mostly holding off for Core 2.0. Has anyone already done the conversion to project.json and xproj? Could we throw that it in a new branch if so?
Ive started down the path of project.json and xproj with good success. @latkin has requested that we keep it in one solution if possible. From my findings in order to do this for VS 2015, the projects have to be converted to portable and target .NET 4.5 and .NET Standard. It would take some work but wonder how much easier it will be in VS 2017. For now I created a Serilog sink for Sumo Logic and published to nuget. This supports full .NET framework and .NET Core. You can find the repo here: https://github.com/billpratt/serilog-sinks-sumologic
Feedback welcomed.
So the more I think about this the more I believe its not possible to get this together with PCLs. Since this thread started the dotnet team introduced netstandardlibrary, and from what I can tell a PCL cannot import a netstandardlibrary package. Nlog, and Log4Net have both moved to netstandardlibrary, and not targeted netcore directly (something which back in October was more normal).
I'm with @billpratt here, we can't support core, and vs2015 at the same time. @latkin either you open up to vs17, otherwise this issue will die, and your customer base will be unable to properly use their applications with your products.
I have no doubt that for sumologic to support vs17 as the minimum, is better than loosing customers. Especially since the DLL will work on older versions of VS, just users will need 17 to edit the code in this repo.
VS 2017 is now RTM, so I suppose there is no longer a strong reason to require compat with 2015.
@latkin I'm actively working on this now, quick question. I see there is a seperate build called DebugCA I think CA stands for code analysis. Do we still want/need this? We could in theory just do code analysis on all builds. Up to you of course. For now I will carry it forward (its not difficult to keep). I ported the common project, and it seems to have gone well. However it was a good amount of manual labor so it might be a few days...
Code analysis on all builds is fine with me, feel free to trash that configuration.