Skip to content

Use of .NET Standard configuration from .NET Framework #161

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

Closed
MV10 opened this issue Oct 10, 2018 · 5 comments · Fixed by #170
Closed

Use of .NET Standard configuration from .NET Framework #161

MV10 opened this issue Oct 10, 2018 · 5 comments · Fixed by #170
Assignees

Comments

@MV10
Copy link
Contributor

MV10 commented Oct 10, 2018

Needs review, was opened in response to a closed issue. Copied from @EEParker #129 (comment) to this new issue for easier tracking:

I have a corner case where I have a project using aspnetcore 2.1.3, but the target framework is .net 4.7.2 due to some legacy components.

In this case, the appsettings.json column options are not applied. Since nuget defaults to use the net45 version, the compile options here change the functionality:
https://github.com/serilog/serilog-sinks-mssqlserver/blob/dev/src/Serilog.Sinks.MSSqlServer/Serilog.Sinks.MSSqlServer.csproj#L54
and
https://github.com/serilog/serilog-sinks-mssqlserver/blob/dev/src/Serilog.Sinks.MSSqlServer/Serilog.Sinks.MSSqlServer.csproj#L44

Currenly I am including this code as a submodule, and only compiling targeting this library as netstandard2.0 which seems to work. Do know of any way around this?

@MV10 MV10 added the question This issue contains a general question. Please use the discussion section for this. label Oct 10, 2018
@EEParker
Copy link

EEParker commented Oct 10, 2018

I created a commit that includes the changes I am using to make this work with my project. I'm not sure they are appropriate for all uses cases but I wanted to make them available for reference.

https://github.com/EEParker/serilog-sinks-mssqlserver/commit/f2796c1da7333dd9116fae0d14f5bcf2dd66fdd3

@MV10
Copy link
Contributor Author

MV10 commented Oct 10, 2018

Thanks, Jeff. When I get to this, I'll test with .NET Framework 4.6.1 since that's the earliest version to support .NET Standard 2.0, but hopefully it'll be that easy!

@MV10
Copy link
Contributor Author

MV10 commented Oct 17, 2018

A note to remind myself that the opposite problem also applies.

Microsoft added the XML-based ConfigurationManager classes to .NET Core 2.0, although they are not part of the .NET Standard 2.0 API specification. It currently isn't possible for a .NET Core application to use that type of configuration, NuGet automatically maps a .NET Core app to the .NET Standard build.

In reality, .NET Framework 4.5.2 support is the troublemaker here, and unfortunately Microsoft tied that to Windows 8.1 which isn't due for end-of-life until January 2023 (ugh).

Although the problems are similar, the solutions could be very different -- a core-specific TFM that somehow pulls in the XML-based configuration. I'd have to think about how to do that without breaking pure .NET Standard support.

@MV10
Copy link
Contributor Author

MV10 commented Oct 20, 2018

As Jeff observed, the main problem is that NuGet chooses the "nearest framework" and the developer has no say in the matter. The same arbitrary behavior makes it a little complicated to address the other scenario I mentioned above.

I opened a feature request in the NuGet repo to allow devs to override that behavior, but in the past Microsoft has been almost agressively opposed to other requests to address similar problems, usually going so far as to suggest instead that developers copy the package locally, unzip it, and reference the version they really want. In short, "If you don't like it, don't use NuGet." This is especially unhelpful in a professional / corporate environment where developers might not have the option to employ such hacks. Anyway, here's hoping they take pity on us and implement it (in the distant future, no doubt, the NuGet issues list is staggeringly huge).

Until that happy day arrives, we can hack the library to work around NuGet limitations because the two configuration methods are not mutually exclusive for newer frameworks. (It would be very odd to use them together, if you ask me, but not impossible.) It would require restructuring the configuration area into three sets of target frameworks, each with it's own LoggerConfiguration extension, and each referencing one or both of some new folders and files supporting the actual configuration process. I'll call this the "Hybrid" approach.

TFMs Supported Config Styles
net452 System.Configuration
netstandard2.0 Microsoft.Extensions.Configuration
net461 and netcoreapp2.0 Hybrid (System.Configuration and M.E.C)

This sink is a relatively small package, but it's still unfortunate that this doubles the size of the package by adding two TFMs -- each TFM will have a separate build in the package.

The revised folder structure would look something like this:

src/Serilog.Sinks.MSSqlServer
├── Configuration
│   ├── LoggerConfigurationExtensions
│   │   ├── System.Configuration
│   │   ├── Microsoft.Extensions.Configuration
│   │   └── Hybrid
│   └── Implementations
│       ├── System.Configuration
│       └── Microsoft.Extensions.Configuration
└── Sinks etc.

The System.Configuration extensions would apply to net452 only, the M.E.C extensions would support netstandard2.0 libraries, and of course, the Hybrid extensions would apply to any newer frameworks that support both.

The Hybrid configuration methods would look exactly like the M.E.C argument list, which means the IConfigurationSection parameter is optional (defaults to null). This allows projects targeting these frameworks to configure the sink from code without passing in a configuration section.

A strange edge case is deciding what happens if someone elects to use both types of configuration. To me that sounds like a bad idea, so I propose just making the decision that System.Configuration is always applied before M.E.C is applied. I doubt there is a good argument for using both (it would make more sense to use M.E.C's built-in multiple-config-source overlaying behavior at that point) and we certainly don't want to add more complexity to this sink's config story.

I don't have time to work on this right now (and I'm hoping Nick will do a release soon, so I don't want to add even more changes at this stage), but I think this plan should fix the problem -- and if Microsoft does address the real problem in NuGet, it should be easy to unwind again.

@MV10 MV10 added enhancement up-for-grabs This issue waits for a contributor to fix it. and removed question This issue contains a general question. Please use the discussion section for this. up-for-grabs This issue waits for a contributor to fix it. labels Oct 20, 2018
@MV10 MV10 self-assigned this Oct 21, 2018
@MV10
Copy link
Contributor Author

MV10 commented Oct 22, 2018

GitHub is having problems this morning but PR #170 fixes this. If the PR doesn't reappear, I'll open it again later when the site is stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants