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

Migrate to .NET Core RTM #34

Closed
wants to merge 9 commits into from
Closed

Migrate to .NET Core RTM #34

wants to merge 9 commits into from

Conversation

ninety7
Copy link

@ninety7 ninety7 commented Jun 30, 2016

Here's my try on moving this sink forward. Let me know what you think!

With the new .NET Core configuration paradigm, the user should be responsible of loading configuration from different sources and passing ColumnOptions. Hence the removal of loading configuration directly from extensions and of configuration classes.
@@ -0,0 +1,189 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to include this file here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I can delete it. Was actually copying from the Console sink.

@nblumhardt
Copy link
Contributor

Hi, thanks for the PR! Looking good.

To make this move, we need to maintain the current functionality under .NET 4.x, so instead of removing all of the System.Configuration-dependent pieces, we need to #if around the parts not available on Core.

Conditional compilation can be controlled with the buildOptions.define section in project.json - adding an APPCONFIG conditional symbol here would allow the config support to be conditionally excluded.

Where possible, when conditionally compiling features we've tried to keep the public API identical between the core and full framework versions, which seems worthwhile given the future alignment in functionality between these platforms.

configuration: Release
install:
- ps: mkdir -Force ".\build\" | Out-Null
- ps: Invoke-WebRequest "https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.ps1" -OutFile ".\build\installcli.ps1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we want this to be consistent?

Others point to preview2 and specify the exact version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the Console sink as a guide for it. Which others point to preview2 so I can update it accordingly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/serilog/serilog-enrichers-thread is a good starting point I think

@nblumhardt
Copy link
Contributor

Hi @ninety7,

I've merged Serilog 2 support into dev and brought the build scripts there up-to-date so we can get a (non-.NET Core) package supporting Serilog v2 out.

This should help with the build infrastructure side of these changes. There's no .NET Core support in there however, so still looking forward to progressing with this one! :-)

All the best,
Nick

@ninety7
Copy link
Author

ninety7 commented Jul 11, 2016

I made an upstream merge and started working on conditional compiling but stumbled upon the code that creates the sql table, which uses a lot of System.Data.* classes which are not available on .NET Core. I'll be working on this as I have time to migrate that code into a .NET Core compatible one.

Sergey Komisarchik and others added 4 commits September 9, 2016 09:48
@colin-young
Copy link
Contributor

This PR is getting a little messy with the changes made on the dev branch. Is it possible to rebase it from the latest dev?

Also, @nblumhardt I've been thinking about a staged move to .Net Core, with the first step being to get rid of System.Data.DataColumn from the ColumnOptions since that is going to be a breaking change. Then we can make the internal fixes for Core without affecting the external interface.

@nblumhardt
Copy link
Contributor

@colin-young that sounds like a good way forward 👍 @ninety7 what do you think?

@colin-young
Copy link
Contributor

See #53 for getting DataColumn out of the ColumnOptions.

@masduo
Copy link

masduo commented Sep 28, 2016

Hi guys, any chance this ending up in stable version soon? thanks

This was referenced Oct 14, 2016
@nblumhardt
Copy link
Contributor

Hi all,

The System.Data support we need in order to target .NET Core without major breakage just landed upstream:

dotnet/corefx#12426

I believe this will be .NET Standard 2.0/.NET Core 1.1.

With those features available, this PR should turn into a simple case of conditionally excluding the System.Configuration stuff.

Do we want to continue with this PR, or open a fresh one against the new platform definition? I think the latter will be easier, given there's a bit of work involved here just in getting back to parity with (the configuration code on) dev.

@colin-young
Copy link
Contributor

I vote for a fresh start...

@nblumhardt nblumhardt closed this Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants