net-commons / common-logging

A portable logging abstraction for .NET
http://net-commons.github.io/common-logging
Apache License 2.0
649 stars 204 forks source link

Add support for DNX #108

Closed bradwilson closed 8 years ago

bradwilson commented 9 years ago

Because the PCLs built here are not System.Runtime PCLs, they cannot be used in DNX. There are projects using Common.Logging and Common.Logging.Core that I would like to port to DNX, but cannot do that without either System.Runtime PCLs, or DNX-specific libraries.

I have a quick prototype (https://github.com/bradwilson/common-logging) using the existing portable source code which seems to imply that it should be fairly simple to go the DNX-specific route.

Thoughts?

JohnFlyIII commented 9 years ago

This would be a good enhancement to common-logging, I'd like to include it in a number of DNX projects.

sbohlen commented 8 years ago

This makes total sense to me; let me take a look @ this tonight/tomorrow in more detail, but I'm 100% behind this goal...

sbohlen commented 8 years ago

Merged in https://github.com/net-commons/common-logging/pull/110 ; will review shortly.

bchavez commented 8 years ago

+1 for this. Trying to use Microsoft.Framework.Logging and it's a total mess. Would much rather stick to one dependency than switch between Common.Logging (for full .net) and MS.Logging (for Core CLR).

https://github.com/bchavez/RethinkDb.Driver/issues/12

Hoping for a release soon! :+1:

sbohlen commented 8 years ago

I've actually already started to look into this and it looks like its entirely feasible to do. Right now my immediate blocker is actually some arcane failure of the DNX Beta 8 installation that is keeping me from completing my review of this. Assuming I can solve this in the next few days, I'd be planning a release of this within about a week (preferably before we head into the Thanksgiving Holiday here in the States)...

bchavez commented 8 years ago

Great! ... just today, at connect 2015 conference they just announced .NET Core RC1:

http://blogs.msdn.com/b/dotnet/archive/2015/11/18/announcing-net-core-and-asp-net-5-rc.aspx

And has GoLive licence. :+1:

sbohlen commented 8 years ago

Yeah, thanks. I'm already contemplating abandoning troubleshooting whatever is wrong with my beta 8 install and just falling over to RC1 to more easily facilitate my review of this...

-Steve B.

-----Original Message----- From: "Brian Chavez" notifications@github.com Sent: ‎11/‎18/‎2015 3:32 PM To: "net-commons/common-logging" common-logging@noreply.github.com Cc: "Steve Bohlen" sbohlen@gmail.com Subject: Re: [common-logging] Add support for DNX (#108)

Great! ... just today, at connect 2015 conference they just announced .NET Core RC1: http://blogs.msdn.com/b/dotnet/archive/2015/11/18/announcing-net-core-and-asp-net-5-rc.aspx And has GoLive licence.
— Reply to this email directly or view it on GitHub.

JohnFlyIII commented 8 years ago

Curious if there was any progress here. -- Just checking, not trying to pester

sbohlen commented 8 years ago

No worries; its a fair question. I've been doing a crazy amount of traveling but have the week betw XMAS and New Years off with the intent to finish on this during that coming week.

-----Original Message----- From: "John Fly" notifications@github.com Sent: ‎12/‎22/‎2015 2:17 PM To: "net-commons/common-logging" common-logging@noreply.github.com Cc: "Steve Bohlen" sbohlen@gmail.com Subject: Re: [common-logging] Add support for DNX (#108)

Curious if there was any progress here. -- Just checking, not trying to pester — Reply to this email directly or view it on GitHub.

trayburn commented 8 years ago

Hello, joining the thread of :+1: on really wanting this. We've traditionally used Common.Logging in the Highway Framework as our general abstraction for logging, and are working on updating to support CoreCLR ourselves right now, and this is a stumbling block.

sbohlen commented 8 years ago

@trayburn I'd actually put this effort to the side owing to all the recent churn in .NET Core fundamentals (everything from naming conventions to APIs to the right NuGet feeds to get the packages from). Now that its all (more or less) stabilized with the effort reaching RC2 recently, I can come back to this and hopefully finish it in short order (next few weeks at the latest, I'd be hoping!)

karldodd commented 8 years ago

Really looking forward to this, as many of our libs depend on Common.Logging (just like @trayburn ), which blocks our transition to .NET Core.

sbohlen commented 8 years ago

Agreed; its definitely coming!

sbohlen commented 8 years ago

OK, I've now pushed the recent changes to MASTER in https://github.com/net-commons/common-logging/commit/78c3b1b19d9e7c6d9e44dd57375483abe368f977 and https://github.com/net-commons/common-logging/commit/2d4eaf0ce36e7f6ddee110f60c70b56c4f6b6c54. I'd appreciate it if people would take a look at this and comment before I go much farther with it.

Its only an initial implementation, but AFAICT it should (?) work. A caveat: its missing any ability to configure it via any external config file of any sort. I'm still exploring the options here (e.g., don't support config-by-file at all, support via some sort of JSON construct that mimics what's in the XML-based config for the non-.NET Core implementation) and would be interested in any feedback/input anyone cares to offer on this point.

As usual, all feedback welcome; hopefully I can devise a config-by-file story for this in the next week or so and push an initial release of it to NuGet subsequently.

Thanks!

brantburnett commented 8 years ago

@sbohlen

I'm also interested in the DNX support for Common.Logging, as I'd love to implement it for the Couchbase .Net SDK. We've recently started working moving towards .Net Core support in the SDK with a variety of changes, and this is one of the blockers.

If it's of any assistance, I found an approach for dealing with configuration files that works across .Net 4.5 and .Net Core, though it does require a some compiler directives. You can see the current work in progress here if it's of any help: http://review.couchbase.org/#/c/63815/

The basic premise is to have both old-style XML Section and Element classes, as well as new style POCOs. Then let them both share an interface, and let your higher level classes accept the abstract interface. For the .Net Core build, just exclude the old-style Section and Element classes from the build using compiler directives, so that the POCO configuration approach is the only option.

Brant

sbohlen commented 8 years ago

Thanks for the suggestion; I'll definitely take a look at that approach shortly.

FWIW, in my research into this on and off over the past several weeks I've also found an MSDN Article that suggests that there is a pretty strong concept in .NET Core around configuration, even if its presently (like much of .NET Core) quite poorly-documented. For example, there's little (no?) documentation re: just what the XML config would look like under this model. And while there's at least a small amount of indication re: what the JSON might look like, there's no guidance (at least that I can find) re: just what conventions might be around even simple things like the name of the .JSON file that should be loaded/read/parsed.

Based on what I've seen in samples it seems like it should just be config.json, but that means that Common.Logging settings could (potentially) collide with other settings and also that the consumer of this config would have to ensure their object model of their config properly accommodated Common.Logging's settings. Right now I'm sort of leaning towards a common.logging.config.json approach where unlike the app.config in 'full' .NET where all settings are jammed into a single file, in .NET Core Common.Logging would be configured entirely separately from anything else in the consuming app (and this would permit Common.Logging to have its own config object model that the consuming app needn't concern itself with accommodating when it reads its 'main app' config out of some other .json file).

Common.Logging does have its DefaultConfigurationReader and its LogSetting classes that do already provide some abstraction points for reading config settings and reporting them to the remainder of the library. You can even see where the (potential) tie-in to this is already stubbed out here.

Right now my tentative plan is to implement a .NET Core-specific IConfiguration/ConfigurationBuilder-based version of the settings reader that would be "in line" with the proposed way that config seems to want to be handled for .NET Core. I'm still experimenting with both the feasibility of this approach and the degree to which it offers sufficient flexibility, but initial investigation seems to be (quasi-)conclusive that this should be a viable strategy.

brantburnett commented 8 years ago

@sbohlen

Yes, that's the article I based most of my work on. Based on my reading, I think the lack of standards you are referring to is actually a part of the design. It seems like the premise is to allow the user to structure the file however they like. Meaning they could put the Common.Logging configuration at any spot in the file they want, in any format they want (XML/JSON/etc) so long as they know where it's at and it follows your POCO class structure.

So, if they want to use "config.json" they can do something like this:

{
  "someConfig": {},
  "logging": { "settings": "here" },
  "someOtherConfig": {}
}

Then they could load your POCOs like this to pass in for configuration:

// Load all their configuration stuff, might be any filename
var builder = new ConfigurationBuilder();
builder.AddJsonFile("config.json");
_jsonConfiguration = builder.Build();

// Do other stuff with configuration

// Send configuration
Common.Logging.SomeInitFunction(builder.Get<Common.Logging.ConfigPOCO>("logging"));

Alternatively, they can define their own POCOs that include the Common.Logging ones:

public class MyConfiguration
{
  public SomeConfigPOCO SomeConfig { get; set; }
  public Common.Logging.ConfigPOCO Logging { get; set; }
}

// To send configuration
Common.Logging.SomeInitFunction(builder.Get<MyConfiguration>().Logging);

Of course, the downside to all of these is that it requires some action on application startup to do the logging configuration. Because the definition is so flexible, it can't be automatically loaded. The upside is that the configuration can be done however you like, even built in memory from a Dictionary, which is great for flexibility and testing.

One option might be to expose the ability to configure like this, but if it isn't configured during startup then look for a common.logging.json file as a fallback.

Brant

sbohlen commented 8 years ago

Interesting (and quite helpful!), thanks!

I completely agree that the flexibility provided by the various ways to 'override' the reading of the config values offers benefits in re: testing, etc. I also like the concept of offering some sort of 'formal front door' for the consumer to pass the config data into Common.Logging and then for us to also provide a fall-back 'default' file should the system be called absent some sort of 'config method' having first been invoked.

We'll explore an approach sim. to that and see how far it takes us...

jeffrymorris commented 8 years ago

@sbohlen -

Any update on when there will be a Common.Logging .Net Core package available?

Thanks!

-Jeff

sbohlen commented 8 years ago

My plan is to get this configuration story sorted out by approx. mid-month (AUG).

This feature (along with https://github.com/net-commons/common-logging/issues/127) are the two remaining blockers to our being able to release 3.4 GA so I'd expect this to follow shortly after that. FWIW, the 3.4.Alpha1 release should be functional for .NET Core 1.0 now assuming you config your logging in code instead of via a config file.

jeffrymorris commented 8 years ago

@sbohlen -

Awesome, thanks for the update!

-Jeff

brantburnett commented 8 years ago

@sbohlen

One issue we're having is that the Common.Logging.Portable nuget doesn't appear to support one of the netstandard versions. This is requiring us to import the old PCL profile instead, which I think may give problems for some consumers. Any plans to include a netstandard compatible PCL?

https://docs.microsoft.com/en-us/dotnet/articles/standard/library

Thanks, Brant

sbohlen commented 8 years ago

@brantburnett

The plan (at least at this time) isn't to have the Common.Logging.Portable package support netstandard. In our (near-term) estimation, this isn't feasible b/c of the differences in e.g., project system, tooling, etc. between netstandard and all other PCL targets.

Instead, we've already spun up a NetCore-specific project here https://github.com/net-commons/common-logging/tree/master/src/Common.Logging.DotNetCore to address this target. As of now, this project is functional from a logging PoV but it presently offers support only for configuration via code (no support for config-via-external-file as of yet).

We're working on an external config-by-file story now, but the fact that this isn't (yet) complete is the reason that this package isn't available on NuGet as part of the 3.4-Alpha1 push. Would you be willing to pull the source, build this assembly for yourself, and perform some tests with it to help ensure that its indeed headed in a direction that will support your needs --?

brantburnett commented 8 years ago

@sbohlen

I'll give it a shot and see what we can do. We're actually building a netstandard compatible library, so I'm not sure if it will work or not. Depends on how we can make the dependencies work and vary them based on the target.

sbohlen commented 8 years ago

@brantburnett

Our objective/goal for that assembly is that it properly support NetStandard, so if you discover otherwise, please do let us know ASAP. This is still in active development so we've not (yet) done full compat-testing, but my expectation is that in its present form it should work for you (minus externalized/file-based configuration, which as mentioned is coming but not yet complete).

jgowdy commented 8 years ago

If there's any way you can publish a pre-release package that contains the NetStandard binaries, that would be awesome.

brantburnett commented 8 years ago

@sbohlen

I think I may have gotten Common.Logging.Portable to support a netstandard1.3 distribution as part of the nuget package. I've uploaded a proof-of-concept here: https://github.com/brantburnett/common-logging/commit/293598e03222a5da8ad8a9d0e13c227b08b1b190

As it turns out, you can actually create PCLs that target Net Standard without switching to the new (and soon to be obsolete) xproj system. It can be done the same old way as before, using a csproj for each framework target. Add to that a dependency on System.Reflection.TypeExtensions, and most of the reflection issues go away.

This POC probably isn't fully integrated into the build system, but hopefully it's a big step in the direction of Net Standard support without changing your process up too much.

Brant

brantburnett commented 8 years ago

@sbohlen

Just checking to see if you had a chance to look at the proposed netstandard approach? If you'd like, I can work on making a pull request from it, I just wanted to make sure you thought the approach would work before I go further.

Thanks, Brant

sbohlen commented 8 years ago

@brantburnett

Apologies for the delay -- I'd been traveling on business and didn't see your comment re: your PoC. Let me review shortly and let you know my thoughts before you spend the time to craft a PR from it.

Standby (and thanks for the input)!

-Steve B.

lahma commented 8 years ago

While checking what it would require to port Spring.NET towards .NET Core I found Common.Logging to be the first show-stopper. I've created a pull request https://github.com/net-commons/common-logging/pull/130 to address the immediate need of having Common.Logging.Core and Common.Logging to contain also the netstandard1.3 artifacts.

Might this be something to sneak into prerelease feed for testing?

sbohlen commented 8 years ago

@lahma

I'll merge this all in and create a pre-release NuGet package based on it during this coming weekend.

lahma commented 8 years ago

Thanks, sounds good. I'll check if I have time to add Serilog and Nlog building support before that too. They also seem to have .NET Core support already.

sbohlen commented 8 years ago

As you can see from the CI output from this merge, the build breaks due to the missing .snk file. Any ideas how best to address that --? It will build locally on my system where I have the .snk file present, but of course I don't want to commit that to the repo just to make the CI output GREEN.

Any thoughts --? The only thought that immediately comes to mind would be for us to put a 'dummy' .snk file in the repo and then have the build script copy it from somewhere 'special' on my HD into the expected path when I'm building locally so its all signed correctly. But that's both brittle and error-prone: once the .ignore is tweaked to include .snk files, any commit after a local build where I 'forget' to revert the .snk contents from the 'official' values back to the 'dummy' values would result in the not-for-public .snk being pushed to GitHub :-/

Any ideas how others are addressing this seeming conflict between needing the .snk for a .NET Core build like this and not wanting the .snk to be public in the repo?

brantburnett commented 8 years ago

@sbohlen

What about using build configurations to control whether it looks for the file or not? The example segment in project.json below should work if you use a special "Publish" configuration only when building to make the nuget packages.

{
   ...
    "configurations": {
        "Publish": {
            "buildOptions": {
                "keyFile": "../../Common.Net.snk"
            }
        }
    }
   ...
}

https://docs.microsoft.com/en-us/dotnet/articles/core/tools/project-json

lahma commented 8 years ago

Yes this was the pitfall I mentioned... @brantburnett 's solution might be the way. I decided to make Quartz's key file public as it really offers no special protection to keep it secret, anyone can build a "valid" copy now. Even Microsoft has stated that whole strong-naming is a bit obsolete: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/strong-name-signing.md .

sbohlen commented 8 years ago

@lahma yeah, I recalled that in your earlier message to me, so I wasn't surprised to see the failure TBH.

FWIW, I'm not sure I agree re: checking the .snk into the repro -- that post you reference is CORRECT that strong naming is used for identity rather than security, but IMO allowing any binary someone arbitrarily creates to 'pretend' to be Common.Logging (or NHibernate, or anything else for that matter) is one more step down a road to untrustable binaries.

Once the .snk is public, what keeps someone from compiling and signing their own Common.Logging.dll that contains all of the functionality of Common.Logging plus phones home to share all PII that might appear in the logs? Even if its not PII, logs often contain info that would be valuable to me as a hacker (e.g., IP addresses, DB conn string). I'd argue that validating the identity of a binary from an OSS project is even more important than for a closed-source one because once the code is open/available the above becomes trivial to create and more challenging to defend against (without using a decompiler to inspect the IL to see exactly what the binary might be doing).

Imagine this starts with a nefarious hacker submitting a 'Commn.Logging' package (note the intentional misspelling) to NuGet. Some subset of people will mistype the name and get that package erroneously installed and I'm off to the races as a bad guy :(

All of this is a round-about way of my saying that I think @brantburnett is probably right in his recommendation .... perhaps I'll try using the .snk file only on RELEASE builds and not DEBUG...that way I needn't specficy a whole other build configuration and CI builds DEBUG builds anyway (IIRC).

Will try that approach next ....

lahma commented 8 years ago

@sbohlen sure, I understand the reasoning and I'm happy that you take the extra mile with protecting the signing key :)

If I reflect to Java land there's no concept of signing the jar files, still they seem to cope with the fact. Information security is getting harder and harder and I guess any measures that can be done to protect malicious acts is good.

I hope that you can tweak the build to work with and without the key. Just something I took a shortcut with.

In the long run it might be easier to use project.json/the-forthcoming-replacement for all building. Now it's a hack and hopefully works. I'm not familiar with creating PCL packages but might be something to consider in v. 4 to just build all with .NET Core tooling.

sbohlen commented 8 years ago

Yeah, agreed; my anticipation is that with Common.Logging 4 we'll both do that as well as drop most (all?) of the older VS version support so that we've got a much cleaner build system and a (hopefully?) lighter maintenance burden for it.

I just pushed updated project.json incorporating @brantburnett 's suggestion -- FAICT it "works-on-my-machine"; let's see if it gets the CI build to GREEN too -- FINGERS CROSSED :)

sbohlen commented 8 years ago

OK, as of https://github.com/net-commons/common-logging/commit/91244f1da8110aed530f89d10696d4275dbc8fd5 we're only building DEBUG (and not also RELEASE) on the CI build, meaning I've opted us out of needing to add the snk file to the public repo for at least the time being.

We're GREEN on the CI server again and I've just packaged and pushed 3.4-Alpha2 to NuGet.

We still need to address the (unresolved) configuration story for Common.Logging in DotNetCore, but I've got that on my backlog for prior to the 3.4GA (unless someone else wants to take stab at in the interim...)

lahma commented 8 years ago

Great, thank you!

brantburnett commented 8 years ago

@sbohlen

Just wanted to let you know that NetStandard binaries in 3.4.0-alpha2 appear to be working great. I have seen successful logs using the DebugLogger in ASP.Net Core on Windows, ASP.Net Core on Linux in Docker, and in Xamarin using MonoAndroid.

Now we just need more adapters supporting NetStandard. For that, we need more loggers on NetStandard. Looks like NLog and log4net are both close. :)

:+1:

Brant

brantburnett commented 8 years ago

@sbohlen

I also took a swing at a configuration system that supports .Net Core.

https://github.com/net-commons/common-logging/pull/133

Brant

sbohlen commented 8 years ago

Closing this issue now that we have both direct support and a viable configuration story for .NET Core.