Add .NET Standard 2.0 build #14

chlowell · 2017-08-16T16:58:39Z

This adds a netstandard2.0 build consumable on .NET Core 2.0, .NET Framework 4.6.1+, Xamarin platforms, and the next version of UWP (this doc has a complete support matrix).

Compared to the netstandard1.3 build, it shares much more code with .NET Framework builds. Some classes still aren't available:

Everything using ASP.NET (would need rewriting for ASP.NET Core)
Everything using remoting (EventLogAppender, RemoteLoggingServerPlugin, RemotingAppender)
WindowsSecurityContext (Windows user impersonation isn't in .NET Standard 2.0)

A few classes are supported on Windows alone because they PInvoke Win32 DLLs. These throw PlatformNotSupportedException on non-Windows platforms:

ColoredConsoleAppender
NativeError
NetSendAppender

Similarly, UserNamePatternConverter is included but will raise a PlatformNotSupportedException when it tries to get the current WindowsIdentity on a non-Windows platform. It catches and reports this exception.

I also include csproj files for developing with Visual Studio 2017. The new log4net.csproj enables building assemblies for all .NET Framework and Core platforms, and packing them with NuGet. It has some limitations outside of Visual Studio 2017:

The dotnet CLI can't build the project for .NET Framework 3.5 and below due to a known issue
On non-Windows platforms, a similar issue requires workarounds to enable building net* assemblies with Mono

…uivalents

dpsenner · 2017-08-17T12:42:41Z

Jenkinsfile

@@ -61,21 +61,14 @@ pipeline {
 					// run docker container
 					builder.inside {
 						// compile
-						sh "nant compile-netstandard"
+						sh "dotnet build src/log4net.csproj -c Release -f netstandard1.3 -o ../bin/netstandard1.3"
+						sh "dotnet build src/log4net.csproj -c Release -f netstandard2.0 -o ../bin/netstandard2.0"


I would prefer this to be a separate stage.

I just now realized why you had to change this. The docker microsoft/dotnet is now on debian stretch which does no longer include nant (https://packages.debian.org/search?suite=all&section=all&arch=any&searchon=names&keywords=nant). We probably should move away from nant entirely.

dpsenner · 2017-08-17T12:44:43Z

log4net.build

-  <!-- these targets build lognet assemblies against .NET Core (.net standard 1.0.4)-->
-  <target name="compile-netstandard" description="Builds .NET Core">
+  <!-- these targets build lognet assemblies against .NET Standard 1.3 and 2.0 -->
+  <target name="compile-netstandard" description="Builds .NET Standard">


I would prefer this to become the target compile-netstandard-1.3 and a second target should be added named compile-netstandard-2.0. Otherwise too many different targets and builds are done by one target and maintaining it becomes a hard task.

dpsenner · 2017-08-17T12:48:46Z

src/Appender/AppenderSkeleton.cs

@@ -321,7 +321,7 @@ public void DoAppend(LoggingEvent loggingEvent)
 				{
 					ErrorHandler.Error("Failed in DoAppend", ex);
 				}
-#if !MONO && !NET_2_0 && !NETSTANDARD1_3
+#if !(MONO || NET_2_0 || NETSTANDARD1_3 || NETSTANDARD2_0)


I see a naming scheme difference here. According to the conventions so far, a underscore has been there before the version of a target. Thus NETSTANDARD1_3 should probably read NETSTANDARD_1_3 and NETSTANDARD2_0 should become NETSTANDARD_2_0. Of course this has slipped through when netstandard-1.3 support was added.

I used the FrameworkMajor_Minor convention because it's used by the build system, e.g. NETSTANDARD1_3 is defined when building a netstandard1.3 library (more details in the Target Frameworks doc).

Then this is fine by me.

dpsenner · 2017-08-17T12:51:23Z

src/CompatibilityExtensions.cs

+
+namespace log4net
+{
+    internal static class CompatibilityExtensions


Where are these used? docstrings are also missing.

Throughout the netstandard1.3 library, wherever a simple substitution is required. I thought it better than adding conditional compilation around every such instance. The diff is misleading here; this file isn't new but moved, from netstandard/log4net.

It is odd to me that this class in the root namespace. I would prefer a namespace log4net.extensions and probably also split up all the extensions into separate classes like MutexExtensions and so on.

It's in the root namespace to avoid sprinkling something like this throughout the project:

#if NETSTANDARD1_3 using log4net.Extensions #endif

My thinking here is that these extensions implement simple method substitutions, aren't part of log4net's public surface, and are only visible to a developer working specifically in the netstandard1.3 context (where they're unlikely to be noticed), so it's convenient to keep them together.

dpsenner · 2017-08-17T12:52:42Z

src/Util/PatternStringConverters/UserNamePatternConverter.cs

+#if NETSTANDARD2_0
+			catch(PlatformNotSupportedException)
+			{
+				writer.Write("WindowsIdentity is not available on this platform");


Could this make use of nameof(WindowsIdentity)?

dpsenner · 2017-08-17T12:54:58Z

tests/src/Context/LogicalThreadContextTest.cs

@@ -17,7 +17,7 @@
 //
 #endregion

-#if NET_4_5 || NETSTANDARD1_3
+#if NET_4_5 || NETCOREAPP1_0 || NETCOREAPP2_0


So far the constant NETSTANDARD1_3 has been used, here it becomes NETCOREAPP1_0, is this intended?

Yes, because the test project builds an executable artifact for a specific platform. The most convenient way to test the netstandard1.3 library would be with a project targeting netcoreapp1.0 or netcoreapp1.1.

hm, not obvious. Would a comment help? Or maybe change it to read as:

#if NET_4_5 || (NETSTANDARD1_3 && NETCOREAPP1_0) || (NETSTANDARD2_0 && NETCOREAPP2_0)

I find that less clear because it confuses .NET Standard and .NET Core, and suggests targeting netstandard1.3 and netcoreapp1.0 simultaneously, which is bizarre. A .NET Standard version is similar to a PCL profile here: an abstract, runtime-agnostic API contract, not a platform on which code runs. Test projects build executables because tests must run on some platform, like .NET Core.

dpsenner · 2017-08-17T12:56:19Z

tests/src/log4net.Tests.csproj

+         Currently multitargeting tests doesn't work, separate projects are required.
+         (tracked at https://github.com/Microsoft/vstest/issues/298 and https://github.com/Microsoft/vstest/issues/624)
+    -->
+    <TargetFramework>netcoreapp2.0</TargetFramework>


If this is the case, should we add multiple test projects targeting different frameworks?

I think that would add clutter for little gain. My understanding is that the csprojes, which currently only target .NET Framework 4.5, are for developing in Visual Studio. Testing everything is left to Jenkins and nant. So I think it reasonable to continue with a single test project for development convenience, at least until there's a more graceful way to multitarget.

Actually, there should be one and only one way to build the project and that way should be used by developers and CI. Right now we have nant and it looks like latest microsoft/dotnet docker image bases on debian stretch which no longer comes with nant. We could work around that for now, but my impression is that we should move away from nant as soon as possible. To me it looks like it is no longer maintained and we should no longer invest into a sinking ship. This means that ideally we would have one project file that builds all targets and another one that tests all targets. Now testing all targets does not work right now, so we need multiple projects for that usecase.

I agree about nant. However, it is nice that it's stable for the .NET Framework and Mono builds, and works well enough in your CI. Building entire releases from one project is somewhat complex here because log4net has so many targets. In particular, I don't know enough about Mono <--> .NET Framework interop to know whether MSBuild can reasonably build for both in a Linux environment.

dpsenner · 2017-08-20T21:13:19Z

I'm pleased to say that I was able to merge large parts of your work into the branch feature/netstandard-2.0 and the build passed. Unfortunately the tests are currently not run because I was not able to get that part to work.

#6 failed in stage compile-netstandard-1.3 because the test project does not target netstandard1.3
#7 failed in stage compile-netstandard-2.0 because a test failed and that caused the entire build to fail; all other compile stages aggregate test results and eventually fail in stage check test results and we should improve the netstandard stages to do that too
#8 no longer runs the tests and therefore passed and includes the artifacts both for netstandard-1.3 and netstandard-2.0

I'm now working on the branch feature/cd-pipeline to finally run the tests against netstandard-1.3, collect the test results and finally check those test results later. I do this to incrementally work towards integrating also netstandard-2.0 by first adding full support for netstandard-1.3. Would you like to join in and help me with that?

I'm not sure how we should proceed with this pull request. What do you think?

chlowell · 2017-08-22T03:27:41Z

Glad the build works. I expect CI will be blocked until there's some resolution for nunit/nunit3-vs-adapter#377, and I suppose there's also a decision to make about the exclusive locking test. The failure isn't a regression from netstandard1.3 but I'd like at least to get clearer about the root cause. I probably won't have time to get deeper into it until late next week.

Since you've merged this code into a feature branch, I think we can close this PR, and I'll base anything further on that branch. Do we need an issue somewhere to track work needed before a release supporting netstandard2.0?

dpsenner · 2017-08-22T13:18:45Z

Alright, let's move further discussion to JIRA (LOG4NET-572, LOG4NET-567). I would have to craft a commit to close the pull request, would you please close it?

NicolasPerego · 2018-01-24T20:59:50Z

I don't think it's needed anymore with Neststandard 2.0 to modify the file "log4net.Config.XmlConfigurator" like this : https://github.com/apache/logging-log4net/blob/master/src/Config/XmlConfigurator.cs#L164, because in Netstandard 2.0 you can use "Assembly.GetCallingAssembly()".

As example, it's needed to use log4net with Common.Logging (see net-commons/common-logging#167) in ASP.NET Core 2.0.

I tried successfully to remove the "#if !NETSTANDARD1_3" of the file, recompile and use the modified dll with Common.Logging and it works fine.

Maybe I'm missing something here, but it maybe is a good idea to revert this change ? Thanx..

(first msg deleted by mistake)

chlowell added 10 commits August 16, 2017 09:32

add preprocessor directives

62a10f7

update AssemblyInfo

4dadc4e

NETSTANDARD1_3 -> NETCOREAPP1_0

f8d560e

replace prior sln and csproj files with vs2017 netstandard/core2.0 eq…

1dedf87

…uivalents

delete .xprojes

3b90468

.gitignore src/log4net.xml

883fa67

replace nant with dotnet cli in build-netstandard stage

b17cdf0

throw PlatformNotSupportedException where Windows DLLs are pinvoked

0fcbc26

update compile-netstandard nant target

4f8ce10

handle PlatformNotSupportedExceptions in UserNamePatternConverter

b30a709

dpsenner requested changes Aug 17, 2017

View reviewed changes

chlowell added 3 commits August 17, 2017 10:04

use nameof

878aa0d

CompatibilityExtensions doc strings

3fa4763

separate nant targets for netstandard builds

20cb82c

chlowell closed this Aug 22, 2017

NicolasPerego mentioned this pull request Jan 24, 2018

.Net Core support Mangopay/mangopay2-net-sdk#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .NET Standard 2.0 build #14

Add .NET Standard 2.0 build #14

chlowell commented Aug 16, 2017

dpsenner Aug 17, 2017

dpsenner Aug 18, 2017

dpsenner Aug 17, 2017

dpsenner Aug 17, 2017

chlowell Aug 17, 2017

dpsenner Aug 18, 2017

dpsenner Aug 17, 2017

chlowell Aug 17, 2017

dpsenner Aug 18, 2017

chlowell Aug 21, 2017

dpsenner Aug 17, 2017

dpsenner Aug 17, 2017

chlowell Aug 17, 2017

dpsenner Aug 18, 2017

chlowell Aug 21, 2017

dpsenner Aug 17, 2017

chlowell Aug 17, 2017

dpsenner Aug 18, 2017

chlowell Aug 22, 2017

dpsenner commented Aug 20, 2017 •

edited

Loading

chlowell commented Aug 22, 2017

dpsenner commented Aug 22, 2017

NicolasPerego commented Jan 24, 2018

Add .NET Standard 2.0 build #14

Add .NET Standard 2.0 build #14

Conversation

chlowell commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpsenner commented Aug 20, 2017 • edited Loading

chlowell commented Aug 22, 2017

dpsenner commented Aug 22, 2017

NicolasPerego commented Jan 24, 2018

dpsenner commented Aug 20, 2017 •

edited

Loading