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

Add Serilog (logging library) and components #28

Closed
wants to merge 3 commits into from
Closed

Add Serilog (logging library) and components #28

wants to merge 3 commits into from

Conversation

schultzcole
Copy link

The NuGet package needs to respect a few constraints in order to be listed in the curated list:

  • It must have non-preview versions (e.g 1.0.0 but not 1.0.0-preview.1)
  • It must provide .NETStandard2.0 assemblies as part of its package
  • The lowest version added must be the lowest .NETStandard2.0 version available
  • The package has been tested with the Unity editor
  • The package has been tested with a Unity standalone player
    • if the package is not compatible with standalone player, please add a comment to a Known issues section to the top level readme.md
  • All package dependencies with .NETStandard 2.0 target must be added to the PR (respecting the same rules above)
    • Note that if a future version of the package adds a new dependency, this dependency will have to be added manually as well

Also remove Serilog.Sinks.Console because it doesn't have a non "dev" version that supporst netstandard 2.0
@bdovaz
Copy link
Collaborator

bdovaz commented Apr 30, 2021

@schultzcole Please, can you update this PR and add this other packages?

Microsoft.Extensions.* integrations (this packages are already included in registry.json):

https://www.nuget.org/packages/Serilog.Settings.Configuration/
https://www.nuget.org/packages/Serilog.Extensions.Hosting/
https://www.nuget.org/packages/Serilog.Extensions.Logging/

Thanks!

registry.json Outdated
"Serilog": {
"listed": true,
"version": "2.8.0"
}
Copy link
Owner

Choose a reason for hiding this comment

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

missing , after each entry.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry about that. That should be fixed now.

@schultzcole
Copy link
Author

@bdovaz
Copy link
Collaborator

bdovaz commented May 5, 2021

@schultzcole Why do you say that? It has no dependencies other than to Serilog or Microsoft.Extensions.* packages. It has nothing to do with ASP.

If you are talking about appsettings.json files, it is a pattern that can be perfectly used in Unity, we do it.

@schultzcole
Copy link
Author

@bdovaz fair enough. I can add them to the PR but I won't be able to test them so it might be better for someone who has that capability to make a separate PR.

@bdovaz
Copy link
Collaborator

bdovaz commented May 5, 2021

@schultzcole After making this change in the registry.json file have you tried running the server? There are missing dependencies.

Is there a way that in CI you can check @xoofx this fact? Because it is easy to miss a dependency as in this case and hide the problem.

image

But before you look at it, read on because I've discovered several things.

I have made a fork to try to add the 3 packages I mentioned.

Along the way I have encountered several things.

  1. The minimum version of Serilog that you have put is 2.8.0 but in spite of targeting netstandard2.0 it depends in turn on another package (System.Collections.NonGeneric) that only targets netstandard1.3.

That is why in this case you should put as minimum version 2.9.0 instead of 2.8.0.

  1. Serilog.Enrichers.Thread: Depends on Serilog >= 2.3.0 so it is not possible to add it since these versions did not support netstandard2.0.

  2. Serilog.Formatting.Compact: Depends on Serilog >= 2.8.0 so it is not possible to add it because as I mentioned in point 1 it is not possible to add this version.

  3. Serilog.Sinks.Async: the same as point 3.

  4. Serilog.Sinks.File: Depends on Serilog >= 2.5.0 with which it is not possible to add it since these versions did not support netstandard2.0.

  5. Serilog.Sinks.File.Header: As it depends on Serilog.Sinks.File it has the problem derived from this one.

As for the dependencies that I proposed I have seen the following:

Serilog.Extensions.Hosting
Serilog.Extensions.Logging

They depend on a PR I just made of the Logging package that lifts from 2.8 to 2.9 and would make it compatible:

serilog/serilog-extensions-logging#188

Serilog.Settings.Configuration

Depends on a stable version 3.2.0 which is the one that starts to depend on a version >= 2.10.0

In short, to support the packages that extend Serilog that you propose, it would be necessary to make several PRs similar to the one I have made so that they can upgrade the minimum version of Serilog to at least 2.9.0.

Serilog.Enrichers.Thread
Serilog.Formatting.Compact
Serilog.Sinks.Async
Serilog.Sinks.File
Serilog.Sinks.File.Header

@schultzcole
Copy link
Author

@bdovaz Ah, thanks for looking in to that. I guess this PR will have to wait on the back burner until all of these libraries drop support for serilog versions < 2.9.0.

Maybe just add Serilog 2.9.0 for now, and I could make separate PRs for additional libraries as they become compatible.

@bdovaz
Copy link
Collaborator

bdovaz commented May 7, 2021

@schultzcole I've created some PR:

serilog/serilog-enrichers-thread#20
serilog/serilog-formatting-compact#43
serilog/serilog-sinks-async#70

Serilog.Sinks.File already has a package with >= 2.9.0 but it's a preview package, we need to wait:

https://www.nuget.org/packages/Serilog.Sinks.File/5.0.0-dev-00909

As you said you can make a PR with Serilog 2.9.0 only and leave this open until we can PR the other packages.

@KuraiAndras
Copy link
Contributor

Any update on this?

@bdovaz
Copy link
Collaborator

bdovaz commented Sep 16, 2021

Hi, I've created a PR #51 with the following:

Serilog
Serilog.Settings.Configuration
Serilog.Sinks.Async
Serilog.Sinks.Console
Serilog.Sinks.Debug
Serilog.Sinks.File
Serilog.Sinks.Trace

There would still be some left:

Serilog.Enrichers.Thread
Serilog.Formatting.Compact
Serilog.Sinks.File.Header

These cannot yet be added because they have a Serilog dependency lower than 2.9.0 and the dependency chain is not compatible with .NET Standard 2.0.

So as you see @schultzcole, either you close your PR or we keep insisting to the author on a release of a stable version of those packages:

https://www.nuget.org/packages/Serilog.Enrichers.Thread/3.2.0-dev-00752

https://www.nuget.org/packages/Serilog.Formatting.Compact/1.1.1-dev-00944

https://www.nuget.org/packages/Serilog.Sinks.File.Header/ (For this one we need a PR first to release the version to >= 2.9.0)

To notify the author, there is this channel:

https://gitter.im/serilog/serilog

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.

4 participants