add targets netstandard2.0 and netstandard2.1

[lg2de]

The discussion on #407 is not yet completed.
I hope this PR can revive the discussion.

BTW: The white space changes were automatically done by Visual Studio.

[lg2de]

The build fails because the build environment is not ready for .NET 3.0 (which I used as test platform for netstandard).
Locally all tests (on Windows) were successful. (The only exception is one test based on localized system error message, which differs on my German PC...)

[stakx]

@lg2de, thanks for trying to revive this! While I believe it would be simplest to introduce new targets by first getting rid of old, obsolete stuff (DictionaryAdapter, as well as the net35, net40, and netstandard1.x targets), any move forward is a good thing.

A few brief inputs -- I haven't yet reviewed in detail:

• .NET Core 3.0 has already reached its end of life. It would be good to have the test project target netcoreapp3.1 instead which is a LTS (long-term support) version.

• IIRC, netstandard2.0 does not officially support System.Reflection.Emit. We should target netstandard2.1 instead. Yes, this excludes .NET Core 2.x, but given that .NET Core is a much faster-moving world than the traditional .NET Framework was (and that .NET Core 3.x is quite simply lightyears better than 2.x 😄) this shouldn't be too much of a loss.

• If we got rid of net35 and net40 first, as well as DictionaryAdapter (in separate preceding PRs), we could get rid of a lot of conditional compilation symbols, simplifying the introduction of this new target. It wouldn't even be necessary to introduce new ones for the netstandard2.1 target, it could use largely the same symbols as <CommonDesktopClrConstants> minus a few ones like FEATURE_{SERIALIZATION,REMOTING,ASSEMBLYBUILDER_SAVE,APPFOMAIN,SECURITY_PERMISSIONS} (IIRC). That's more of a hint than a requirement.

[stakx]

Thank you for working on this!

I've tried reviewing as much as I can tonight, however my review may not be complete. (You may want to wait for the issue discussion to come to a conclusion before fixing things.)

Additional to the review comments below, you'd need to make a few more changes:

• Get the CI to succeed. For a start, in appveyor.yml, try switching from the Visual Studio 2017 image to the Visual Studio 2019 one; the latter includes the .NET Core 3 SDKs (see https://www.appveyor.com/docs/windows-images-software/#visual-studio-2019).

• You'd need to mention the new target framework in the CHANGELOG.md (under a newly added Unreleased > Enhancements heading).

[lg2de]

The Windows build is running now using VS2019.
The Linux build (still) fails with some crashes. I'm not familiar with mono/.NET on Linux. Please have a look.

[lg2de]

Would be great to have the new version (4.5.0?) quite soon, at least a test version (beta?) on myget or similar.
I would like to request update reference e.g. in NSubstitute.

[stakx]

The Linux build (still) fails with some crashes. I'm not familiar with mono/.NET on Linux. Please have a look.

There appears to be a regression inside the Mono runtime, see mono/mono#18970.

You could try replacing image: Ubuntu with image: Previous Ubuntu in appveyor.yml, which might result in an older (non-broken) version of Mono getting used during CI runs.

(If that doesn't help, we'd have to manually install a specific Mono runtime before running our actual CI script... if we have to go that far, we should fix the Mono CI run in a separate PR.)

[lg2de]

I switched to "Previous Ubuntu" as recommend, but the build still fails, with another error message.
Please get the Linux build running in another PR.

[jonorossi]

Thanks @lg2de for the pull request, and apologies that we haven't actioned it. I'm not going to make specific comments on this pull request at the moment because as @stakx mentioned we've got a bunch of things we really need to do first.

[lg2de]

Ok @jonorossi. So far I think existing comments are resolved here. Please drop me a note still changes are required here.
According to recent comments on #407 you want to have netstandard2.0 AND 2.1. Should I add it here?
Do you need further assistance? Please list required issues.

[stakx]

According to recent comments on #407 you want to have netstandard2.0 AND 2.1. Should I add it here?
Do you need further assistance? Please list required issues.

@lg2de I think it would be great if you could add both netstandard2.0 and netstandard2.1 targets in your PR, like @jonorossi suggested.

I've submitted a separate PR (#495) for removing the old target frameworks (also as per @jonorossi's suggestion), so those tasks are basically ticked off the list. However, your PR will have to be merged first—because if we removed all current netstandard1.x targets first, we would have no remaining targets left for .NET Core and could no longer build & test there.

I'll next look at restoring the CI build to working order. Once that's done, we can proceed with your PR, then finish up with mine.

[lg2de]

I've added netstandard2.0 again.
Please check whether the tests on netcoreapp2.0 (for netstandard2.0) should be added in the script.

[stakx]

@lg2de, our CI Linux build is now fixed, and there has been a release published so there are some merge conflicts now. Could you please rebase your PR on top of current master? I'll try to review the latest changes in your PR as soon as time permits (might take me 1-3 days, though).

[stakx]

Please check whether the tests on netcoreapp2.0 (for netstandard2.0) should be added in the script.

@lg2de: In my personal opinion, I would say let's skip this part, and just run the tests on .NET Core 3.x.

@jonorossi: My thinking here is that since we're considering (in #497) a move of our CI build over to GitHub Actions, it's perhaps not worth the effort to invest in our AppVeyor CI setup at this time. If we decide to stay on AppVeyor, we can still add more target platforms in the test run. And if we move to GitHub Actions, we'll need to rewrite the whole CI scripts from scratch anyway.

P.S.: To be more precise, I think testing on netcore2.0 would be a good idea in principle, just that it's not worth doing anymore on AppVeyor. I'd rather work on the migration to GitHub Actions before version 5 gets released, and add an additional .NET Core test run at that time. That'll likely be a lot easier to do.

[stakx]

Apart from the whitespace issue and the change in appveyor.yml this looks mostly good!

Regarding the failing unit test:

1) Failed : Castle.PublicApiTestCase.PublicApi
  ref/Castle.Core-netstandard2.0.cs does not match Castle.Core.dll
  Expected string length 296899 but was 298832. Strings differ at index 51204.
  Expected: "...tyAttribute() { }\r\n    }\r\n    [System.AttributeUsageAttrib..."
  But was:  "...tyAttribute() { }\r\n    }\r\n    public class SetProjection<T..."

I don't yet fully understand what happened here. If I find out, I'll report back about it later.

[lg2de]

Now I fixed some white spaces.
I hope you can live with the remaining.

Are you planning to do squash merge?
Otherwise I would squash (and force push) my branch before merging.

[stakx]

It would have been nice if you could have fixed all whitespace-only issues... but since the changes done by your commits are generally for the better, let's drop this point and get on with the actual work here. 😉

The CI builds still fail due to two problems, once we get those fixed I think we can (squash) merge this.

On the Windows CI build:

1) Failed : Castle.PublicApiTestCase.PublicApi
  ref/Castle.Core-netstandard2.0.cs does not match Castle.Core.dll
  Expected string length 296899 but was 298832. Strings differ at index 51204.
  Expected: "...tyAttribute() { }\r\n    }\r\n    [System.AttributeUsageAttrib..."
  But was:  "...tyAttribute() { }\r\n    }\r\n    public class SetProjection<T..."
  ------------------------------------------------^
at Castle.PublicApiTestCase.PublicApi()

The reason for this failure is that the ref/ files you've included in your commits aren't up-to-date. If you delete all files under ref/ and re-run build.cmd, they should get updated. The changes should include some added lines involving SetProjection. Commit those changes, then next time you push to GitHub, this error should be gone. (Those files are basically just documentation that hint at which parts of the public API contract are affected by some commit(s).)

On the Linux CI build:

1) Error : Castle.Core.Logging.Tests.DiagnosticsLoggerTestCase.SimpleUsage
System.PlatformNotSupportedException : EventLog access is not supported on this platform.
TearDown : System.PlatformNotSupportedException : EventLog access is not supported on this platform.
   at System.Diagnostics.EventLog.Exists(String logName)
   at Castle.Core.Logging.Tests.DiagnosticsLoggerTestCase.Clear() in /home/appveyor/projects/core/src/Castle.Core.Tests/Core.Tests/Logging/DiagnosticsLoggerTestCase.cs:line 34
--TearDown
   at System.Diagnostics.EventLog.Delete(String logName)
   at Castle.Core.Logging.Tests.DiagnosticsLoggerTestCase.Reset() in /home/appveyor/projects/core/src/Castle.Core.Tests/Core.Tests/Logging/DiagnosticsLoggerTestCase.cs:line 45

From what I can tell, EventLog is only supported by .NET Core on Windows, but not by .NET Core on Linux. It also isn't supported by .NET Standard (see .NET API docs)... which perhaps is why building your branch in VS2019 fails (at least on my machine).

I think you need to remove the FEATURE_EVENTLOG symbol from both of your <NetStandard2xConstants> in common.props. I think you can then also remove (from Castle.Core.csproj) the <PackageReference Include="System.Diagnostics.EventLog"/> for both netstandard2.x targets.

(Oh, one final thing — sorry for not realising this earlier: Could you let the test projects target netcoreapp2.1 instead of netcoreapp2.0? 2.0 is already end-of-life, while 2.1 is an LTS version.)

After those changes we should be good to go.

Thanks!

[lg2de]

After switching test projects from netcoreapp2.0 to netcoreapp2.1 the log4net integration fails:
src\Castle.Core.Tests\Services.Logging.Tests\log4netIntegration\Log4netFactoryTestCase.cs(44,43,44,56): error CS1501: No overload for method 'GetRepository' takes 0 arguments

I've no idea what happens there.

[stakx]

@lg2de, it appears that log4net had a breaking change between versions 2.0.5 and 2.0.6 in its netstandard1.3 version – they removed the LogManager.GetRepository() method overload (see API diff).

Not sure why that never showed up until now, I can only guess. I remember that at some point, the .NET guys thought it would be great if .NET Framework assemblies could automagically run on .NET Core, too. This is possibly an effect of that and NuGet decided to use the net45 version of the library (where the overload still exists) even when compiling for netcoreapp1.1 and netcoreapp2.0.

Let's get this right so that problem is out of the way in the future.

Instead of downgrading (which might get us into another NuGet version swamp), I suggest you try rewriting LogManager.GetRepository() as LogManager.GetRepository(Assembly.GetCallingAssembly()); AFAICT this is how the net45 version of log4net implements the now-removed overload.

[stakx]

Almost there!

I noticed a couple of things resulting from the netcoreapp2.0 → netcoreapp2.1 retargeting. There's still some netcoreapp2.0s lying around, would be nice to get those cleaned up, too.

Then I think we'll be really done. 😜

[stakx]

Looks good to me @lg2de. Thanks for being patient will all my small requests. 😃

@jonorossi, while your review is still pending, I hope you won't mind me going ahead and merging this now; on the whole, this PR does what it set out to do. While there are still a few unresolved details here (such as missing documentation for a newly introduced conditional compilation symbol), I think focusing too much on those details at this time would effectively be a waste of time since I'm very likely going to work on much the same things in PR #495 again which counteract changes made here (e.g. many of the conditional compilation symbols will get removed).

[jonorossi]

@stakx looks good to me

@lg2de thanks for persisting, I know that the codebase can be painful to work with sometimes, there is well over a decade of history and a bit of a mess from recent years with different .NET Core tooling.

[dschwartzni]

@stakx
I am migrating a library to net standard 2.0 and found that the latest Castle.core library supports this. However, the castle.core-log4net does not. Will there be a release soon with that support? Is there a target that I can add and submit as a pull request?

[stakx]

@dschwartzni:

The log4net integration package targets the same platforms as the main package, including netstandard2.0 (see here). So no further code changes should be necessary.

Regarding the release schedule, /cc @jonorossi.