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 targets netstandard2.0 and netstandard2.1 #485

Merged
merged 8 commits into from
May 13, 2020

Conversation

lg2de
Copy link
Contributor

@lg2de lg2de commented Mar 10, 2020

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
Copy link
Contributor Author

lg2de commented Mar 10, 2020

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
Copy link
Member

stakx commented Mar 10, 2020

@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.

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

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).

buildscripts/build.cmd Outdated Show resolved Hide resolved
buildscripts/common.props Outdated Show resolved Hide resolved
buildscripts/common.props Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Show resolved Hide resolved
@lg2de
Copy link
Contributor Author

lg2de commented Mar 11, 2020

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 lg2de changed the title add target netstandard2.0 add target netstandard2.1 Mar 11, 2020
@lg2de
Copy link
Contributor Author

lg2de commented Mar 11, 2020

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
Copy link
Member

stakx commented Mar 11, 2020

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
Copy link
Contributor Author

lg2de commented Mar 12, 2020

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.

Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

lg2de commented Apr 29, 2020

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
Copy link
Member

stakx commented May 1, 2020

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 lg2de changed the title add target netstandard2.1 add targets netstandard2.0 and netstandard2.1 May 2, 2020
@lg2de
Copy link
Contributor Author

lg2de commented May 2, 2020

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

@stakx
Copy link
Member

stakx commented May 6, 2020

@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
Copy link
Member

stakx commented May 6, 2020

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 stakx added this to the v5.0.0 milestone May 6, 2020
Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

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.

appveyor.yml Outdated Show resolved Hide resolved
buildscripts/common.props Outdated Show resolved Hide resolved
src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
@lg2de
Copy link
Contributor Author

lg2de commented May 9, 2020

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.

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

lg2de commented May 12, 2020

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
Copy link
Member

stakx commented May 12, 2020

@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.

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Almost there!

I noticed a couple of things resulting from the netcoreapp2.0netcoreapp2.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. 😜

src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
@stakx
Copy link
Member

stakx commented May 13, 2020

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).

@stakx stakx merged commit 3a663b5 into castleproject:master May 13, 2020
@jonorossi
Copy link
Member

@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.

@lg2de lg2de deleted the NetStandard2 branch May 14, 2020 11:00
@dschwartzni
Copy link
Contributor

@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
Copy link
Member

stakx commented Jul 12, 2021

@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.

@pmorelli92 pmorelli92 mentioned this pull request Apr 14, 2022
6 tasks
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