-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add .NET core support via Windows Compatibility Pack #25
Conversation
… use the package in .NET core projects by utilizing windows compatibility pack.
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.
Thanks for the PR! Looking great. Keen to keep the changes additive - i.e. only depend on the compat package for the .NET Core targets - if possible. Cheers!
appveyor.yml
Outdated
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" | ||
- ps: Invoke-WebRequest "https://dot.net/v1/dotnet-install.ps1" -OutFile ".\build\installcli.ps1" |
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.
These shouldn't be needed on the VS 2017 AppVeyor image; there's an appveyor.yml and Build.ps1 in https://github.com/serilog/serilog-sinks-file that can both be used here as-is, without modification.
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.
Sure I'll update it.
@@ -7,7 +7,7 @@ | |||
<NeutralLanguage>en-US</NeutralLanguage> | |||
<VersionPrefix>3.1.0</VersionPrefix> | |||
<Authors>Jeremy Clarke;Fabian Wetzel</Authors> | |||
<TargetFramework>net45</TargetFramework> | |||
<TargetFrameworks>netstandard2.0;</TargetFrameworks> |
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.
Could use net45;netstandard2.0
here, to keep .NET 4.5 compat?
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 net45;netcoreapp2.0
might be another decent option to consider, since it'd be easier to constrain use/dependency on the compat package.
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.
Might need to revert 2fe88df since net45
doesn't support it.
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.
We can use #if directives. There are pre-defined symbols for .NET core and standard.
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.
Done!
@@ -26,11 +26,6 @@ | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="Serilog" Version="2.3.0" /> | |||
<PackageReference Include="System.Diagnostics.EventLog" Version="4.5.0-preview1-25914-04" /> |
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 we keep the .NET 4.5 target, this can then be placed in a Condition=
block (like the deleted one a few lines down) so that on .NET Framework we'll continue to use the built-in assembly, only referencing this package under the .NET Standard targets.
Thanks for the review. Just pushed required changes! It would be great if you can publish a preview version to Nuget if the PR in accepted level. |
Awesome! 👍 We won't be able to push a stable release of this until the compat package is non-prerelease, but a -dev package should be published in a moment. |
Yeah, that's enough for now. Thanks!! |
This should resolve issue #24.
netstandard2.0
. Package should be compatible with .NET Core 2.0 and .NET Framework 4.6.1 according to .NET standard docs.net461;netcoreapp2.0;