-
Notifications
You must be signed in to change notification settings - Fork 934
NH-3807 - Support for .NET Core 2.0 #633
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
Conversation
8fc2f04
to
5b8148d
Compare
src/NHibernate/NHibernate.csproj
Outdated
<Compile Include="Param\IExplicitParameterSpecification.cs" /> | ||
<Compile Include="Param\IParameterSpecification.cs" /> | ||
<Compile Include="Param\NamedParameterSpecification.cs" /> | ||
<Compile Include="Para |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be no "System.Reflection.Emit" (and it's friends below) for .NetStandard 2.0, only for .NetCoreApp 2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'm not sure if this is a legitimate use of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazzik I guess I'm not sure what you mean... System.Reflection.Emit is a package reference and is available for .NETStandard 1.1 and that means applies to all larger versions. When a .netstandard package gets imported to a full framework project, dependencies like this NuGet package results in a no-op, so the framework version gets used. If the API is included in .NetCoreApp 2.0, then a similar no-op would take place in the NuGet package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Reflection.Emit is available only for netstandard1.0 + platform extensions (whatever it does mean)
I pulled down this PR and ran the tests against the PostgreSQL83Dialect and .NET Standard 2.0 . Here are the results:
Which is roughly what is reported above against SQL Server but a bit lower pass rate. I tried analying the failed tests a bit and didn't see a clear failure pattern. @ngbrown Is it possible for you to characterize what code functionality still needs work in the PR? Is there a specific type of functionality that hasn't been worked on yet? Or is there a bunch of issues in a variety of areas? I am trying to gauge if I could pick up a build based on this PR in order to start seeing if NHibernate on .NET Core 2.0 will work for us and am unsure at a glance if the functionality we need intersects with the unfinished areas. |
@shallowtempo, hello, and thanks for showing interest in helping. I'm guessing that with so few tests failing, there's a high likelihood it will work fine in a real-world application. If you could try it out and if you come across any problems, let me know. |
src/NHibernate/NHibernate.csproj
Outdated
</PropertyGroup> | ||
<PropertyGroup> | ||
<AssemblyOriginatorKeyFile>..\NHibernate.snk</AssemblyOriginatorKeyFile> | ||
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why do we need to do .netstandard instead of .netcore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazzik, I would like to understand what you think the benefits of targeting .netcore would be?
From what I understand, netcoreapp2.0
is for the resulting application or executable (and a specific runtime), while .netstandard is for libraries that will be compatible with current and future .netstandard compliant runtimes... That means not just .NET Framework 4.6.1 and .NET Core 2, but also other implementations such as Mono, Unity, Tizen, etc.
All libraries I know of target .netstandard. Entity Framework Core does, ASP.Net Core 2 will (they had some issues with the preview 1, which is why they didn't initially).
Often when I use NHibernate in an application, I'm not linking it just to the final Web project, but to an intermediate class library that contains the domain layer and database mappings that is used in a variety of ways, including for unit tests and adjacent console task projects. I think it would be in the best interest to have that layer as portable as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might interest you; Scott Hanselman just addressed a very similar question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in short, we could theoretically even only target netstandard, no more net461. But since we still have a bunch of missing features in netstandard, better keep the explicit target on the full framework for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I understand what is the difference between .NetStandard and .NetCore.
From what I understand, netcoreapp2.0 is for the resulting application or executable (and a specific runtime)
Not quite. There are plenty of libraries targeting .NetCoreApp.
I think it would be in the best interest to have that layer as portable as possible.
It will not if we have to cut a lot of things because they are not supported in a standard.
other implementations such as Mono, Unity, Tizen, etc
I doubt if anyone would use NHibernate on these platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, but does .NetStandard lacks things NHibernate requires and are available in .NetCore?
We have some dependencies which are not pure .NetStandard, but .NetStandard + platform extensions (System.Reflection.Emit & System.Reflection.Emit.Lightweight)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not if we have to cut a lot of things because they are not supported in a standard.
Very little has been cut out, and none on full .NET Framework. What is not lost is the richness that NHibernate has for legacy databases. Having that available on upcoming platforms like in Docker Containers (both Windows and Linux) is what my goals are in doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazzik the correct way to go is .NetStandard... the problem is that if i would like to use .net 462, i cant because its only .net core. With standard its possible. If you see the packages on nugget, 90% is targeting netstandard. Its a point for no problems on the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danilobreda if you want to use 462. You reference 461. No problemo here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with targeting .NetStandard is that we depend on some packages which are not .NetStandard, but .NetStandard+Platform Extensions. It means that it would fail on the platforms which do not have these extensions.
In my branch there are 2 separate builds: .NET461 and .NetCoreApp. the .NET Core build does not have any dependency on a full framework. |
I have compiled a private version of ANTLR. It works and is capable of building the NHibernate assembly... But |
Prefer to completely purge N/Ant and switch to MSBuild |
Need to switch to dotnet native versioning |
@@ -1,3 +1,5 @@ | |||
#if !NETSTANDARD2_0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supported through System.ServiceModel.Primitives (.NetStandard + Platform Extensions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-enabled it, However, I don't think there are any tests that cover the use of WcfOperationSessionContext
.
@@ -1,3 +1,4 @@ | |||
#if !NETSTANDARD2_0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
As I can see the System.Transaction is supported on .netstandard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this class enabled makes my failure count go from 67 to 1,676.
Simply calling new TransactionScope()
causes: System.Transactions.TransactionAbortedException : The transaction has aborted.
.
Calling transactionContext.AmbientTransation.EnlistVolatile(transactionContext, EnlistmentOptions.EnlistDuringPrepareRequired);
results in: System.Transactions.TransactionException : The operation is not valid for the state of the transaction.
.
Calling System.Transactions.Transaction.Current.EnlistDurable(Guid.NewGuid(), this, EnlistmentOptions.None);
results in: System.PlatformNotSupportedException : This platform does not support distributed transactions.
.
Maybe it's supported, but it doesn't seem to be working for me. And this is Windows. What does a distributed transaction even look like on Linux?
This is the implementation: dotnet/corefx#10089 Maybe they are missing something we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start reading from here: https://github.com/dotnet/corefx/issues/15046 => dotnet/standard#157 => dotnet/standard#168
Basically it is supported in the standard, but not yet implemented everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazzik, so what do you suggest? From what I gather, there is a very narrow scenario that may work, but lots of NHibernate's tests fail with this factory.
I have never used distributed transactions, but at this point I would prefer to say that they are only supported in the 4.6.1 framework version, not in .net core. Mainly because I don't know what it will take for distributed transactions to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mind that TransactionScope
is not only about distributed transaction. I believe its most frequent usage is not distributed at all. Its main purpose is to be a transaction handling pattern. It additionally supports distributed transaction with full framework, but as far as I know, not within the standard for now. Distributed transaction are dependent on MS DTC, which is an old Windows only technology. Your test with the durable ressource showcases this, since enlisting a 2PC only durable ressource always enforces promotion to distributed. Only SinglePhasePromotable durable ressource can be used without enforcing promotion to distributed, provided their are not enlisted along with another durable ressource. Database connection are normally enlisting as durable ressource. Some data provider (like Npgsql) do enlist them as volatile, but that is not a normal case. Maybe some other data provider enlist them as 2PC only durable ressource, and then those ones will not be usable with .Net Standard.
Tests about distributed transaction (those ones forcing escalation to distributed) should be disabled if not running under the full framework. (In #627, I have made a bunch of renaming for ceasing to imply distributed where nothing was distributed: this should help having a clearer view.)
But we should have all the other TransactionScope
use cases working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fredericDelaporte I got it mostly working. Do you have any idea how to implement ITransactionPromoter.Promote()
or if it even needs implemented? It's part of the IPromotableSinglePhaseNotification
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promote
is for going distributed. In the mono case, it seems likely to me it has to throw NotSupportedException
. But for which use do you want to implement IPromotableSinglePhaseNotification
? If it is in the "DtcFailures" tests, this could cause them to be no more distributed, which is not the aim of those tests. If it is in the AdoNetWithDistributedTransactionFactory
, it should not be needed, and I do not believe it can be done while preserving the "flush on commit" feature. The 2 PC volatile ressource NHibernate is enlisting there should not cause promotion to distributed (only a durable is supposed to do that).
I've made some progress and am now down to only 64 failures (on Windows). A big chuck of the previous failures were from I also have enlisted transactions working, that means most of the What is remaining are mostly grouped into the following categories:
|
@@ -90,7 +90,7 @@ public void ExecuteWorkInIsolation(ISessionImplementor session, IIsolatedWork wo | |||
} | |||
} | |||
|
|||
public class DistributedTransactionContext : ITransactionContext, IEnlistmentNotification | |||
public class DistributedTransactionContext : ITransactionContext, IEnlistmentNotification, IPromotableSinglePhaseNotification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has it really changed anything to add that interface while still enlisting the resource the same way at line 71? I do not think so. I think this change is unnecessary. Please tell me if undoing it causes failure for non-distributed cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct about IPromotableSinglePhaseNotification
not actually being needed. When I added the cleanup CheckEnlistedTransactionsWereDisposed
in TestCase.cs
is when most of the errors went away.
I'll revert the changes to DistributedTransactionContext
.
@@ -104,6 +104,79 @@ public DistributedTransactionContext(ISessionImplementor sessionImplementor, Sys | |||
IsInActiveTransaction = true; | |||
} | |||
|
|||
#region IPromotableSinglePhaseNotification Members | |||
|
|||
void IPromotableSinglePhaseNotification.Initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not look to me as a valid place for flushing before commit. It flushes too early here, right at the beginning of the transaction I believe, not "just before actual commit". Anyway currently I think this code here has no actual usage (never called).
Not complete or "plain wrong", as the Sybase one which registers keywords in caps while the matching of keywords is done against lower case tokens. Currently keywords are to be registered in lower case, otherwise they are ignored. (The base dialect could just do a to lower on them, but that is not currently done.) |
9c0259e
to
3b68599
Compare
Here's a summary of the failures left:
I'm going to concentrate on finding the root cause of the unknown category. The others are going to end up To continue including session serialization, it basically has to be able to serialize as JSON (as an example). The .NET Core binary serializer no longer supports serializing complex internal types such as |
Also, I'm re-writing the build scripts with Cake, as it can be run cross-platform, enabling building and testing on all supported .NET Core platforms. |
I would like to stick with msbuild. It's cross platform. And a part of a
platform
…On Sat, 8 Jul 2017 at 4:50 PM, Nathan Brown ***@***.***> wrote:
Also, I'm re-writing the build scripts with Cake <http://cakebuild.net/>,
as it can be run cross-platform, enabling building and testing on all
supported .NET Core platforms.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#633 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI1l6gOTdmNJOPieA42ydtnlzTTWt46ks5sLwqbgaJpZM4NoylR>
.
|
@hazzik, something needs to drive the NUnit is making quite successful use of it, while still using msbuild for building and package creation: https://github.com/nunit/nunit/blob/master/build.cake I was also going to look into branching out with a Linux build on something like Travis CI, as it is possible to have a container configured with the various database servers desired for testing. Including Microsoft SQL Server 2017. |
Yes, I would be happy to jump on a call. |
I'm not sure I can add to the call I'm just trying to connect the dots... if I can help in anyway please shout and I'd be more than happy to join the call. |
Does the documentation need to be built on Linux? |
I think that chat session is much better, because you can keep history in a text form. You can reach some conclusions in Tel-Co, but still someone has to made some kind of written overview. I ll be happy to be part of that chat. Also I think that in the past msBuild was used because there used to be some Teamcity builds. Currently teamcity does not natively support psake or cake - you have to use command line build steps. |
I do not think this is an issue in the case of the NHibernate TeamCity builds, because it uses a build agent provided by @hazzik. He can install whatever he wants on it. (I also have remote access to this build machine.)
I have done this (#1508) neither for cross platform compatibility, nor in a .Net Core perspective. I have done this because our TeamCity builds currently relies on a build machine provided by @hazzik, and I wanted to investigate the possibility to cease needing it. I have not used the Nant build scripts at all in it because it was looking to me as more work than simply rewriting what they do with PS. By the way, taking the approach of ad-hoc scripts for each CI platform would avoid changing anything to the current CI, and so it would remove the TeamCity build out of the equation for getting a .Net Core/Standard version build under Linux or Macintosh. But as you can read in #1508, no one was agreeing with me so I have just closed it. (I have not deleted its branch for letting anyone who would like see how that AppVeyor build was working.)
I do not think we should have this requirement. Furthermore, its build has two main dependencies: SHFB and DocBook. DocBook being Java based, it should be possible to build it under Linux or Macintosh. SHFB uses the Microsoft HTML Help Workshop, I doubt there is a cross-platform version of this. |
MSBuild is still a prefered option. PSake has come to a radar because the changes I've played with were based on @fredericDelaporte PowerShell changes.
MSBuild was used because it's a native build system of .NET. NAnt was used because the project has lived longer than the MSBuild existed.
It is always a better option than vendor-locking to a build system either it would be TeamCity, AppVeyor, Travis-CI, Jenkins or any other system. The incentive to move away from NAnt has come from several points:
This is simply not accurate. The only thing is vendor-locked and environment-locked is a
No, it would vendor-lock us and stop migrating to other platform when the current platform will die. And it WILL DIE eventually. |
I do not see a vendor lock in using a single 335 lines build file specific to that platform. Migrating to another platform when NHibernate will have would to will be a similar, quite short work. And it will be an opportunity to upgrade the build by the way, since by the time AppVeyor come to pass away, it is very likely new practices and tooling, potentially better, would be available. This would be true for a cross-platform, "non-vendor specific" solution too, excepted it will be more complex to evolve since it will have to retain that cross-platform capability. This means it will likely have more inertia, till it become too obsolete, a bit like our Nant scripts nowadays, causing us to have to rewrite it fully. And by the way, "non-vendor specific" is an utopia in my mind: there will be always some parts needed to get run by the platform which will be specific to it. Maybe the specific part can be reduced to a one liner just launching NHibernate build scripts, which will then doing all the work without using any tooling supplied by the platform, up to the point of installing and setting up all the db we need for our tests, publishing the tests results, .... But I do not see how going this far could overall save us work compared to being fully vendor specific with a script as simple and short the platform allows for what we build. I am still not convinced aiming at having a cross-platform, "non-vendor specific" build solution is a valid prerequisite to conceiving the .Net Core version of NHibernate. I think that renders the process more complicated, and that it will not have added value, causing more work overall. |
Out of interest we are moving away from psake to cake. Partially that was motivated by the crisis in maintainers (I know Brandon has taken over from Gary now), but also because we perceive it is easier to maintain C# over Powershell. As we go x-platform we are busy unwinding our position on PS in favor of CLI written in Python or C#. I have no idea if others are doing the same. It is easier to go x-platform with Cake today, and it appears to have most of the community momentum. Gary Ewan Park is willing to talk this through with you guys. We'd be happy to help out with some Huddle resources, if we can get clear agreement on what work it would help if we took on. |
That said, from an outsider's point of view I agree with @fredericDelaporte that you may want to determine if the Core milestone needs x-platform builds, or if you make that a later milestone. Iterative over big-bang. But I'm a chicken not a pig in this conversation, so... |
We are back with FluentNHibernate and planing to follow the update to NetCore when NHibernate release the new version. We move the build system to build.cake too and AppVeyor. Let me know if I can help on anything! Thanks for the amazing work! |
Hi guys. Any ideas about a release date? One month, three months, six months? |
As @iancooper mentioned, if there is anything that we the Cake Team can do to help with this, let me know. |
Just tried this version with the latest oracle odp DLL (which supports .netstandard 2.0) and it works :D Had to use a separate Oracle driver, because the Reflection driver loaded the full framework one and kept getting DbFactoryProvider exceptions. In short, you can connect to an oracle DB through a .net core app, at this moment. Thanks for this library and this pull request, I still think nHibernate has something to say in 2018, can't wait to see it officially in a nuget package. |
Awesome work Guys and Girls ! Reading through this ticket, is "merging" all whats left to do? (FYI: I'd be happy to chip in, but don't have the required street cred to unlock that level.... ) |
An alternate PR for .Net Core has been merged: #1523. It has less changes: drivers are not split in separated projects, binary serialization is not fully supported. |
@fredericDelaporte is this PR being abandoned or will it be updated to support these other things? |
The two things I have cited already exist as separated PR. About this PR here, @ngbrown will likely check at some point if there is still things to do with it or if it should be just closed. |
Could somebody provide demo example with net core 2.0 ? |
@Diaskhan the included web example works as well: https://github.com/nhibernate/nhibernate-core/tree/master/src/NHibernate.Example.Web |
@ngbrown is there anything else from this PR that is not included into other PRs? |
@ngbrown I want to say thanks. That probably means little but you've struggled for a year or more with little feedback from the main NH team on progress or the fact other efforts were underway. |
@penderi Thanks for the kind words. |
Since the core of this was implemented by #954, and I have pull requests for most outstanding differences, I'll close this. The major outstanding difference is #626 which would split each database provider off into individual NuGet packages. These NuGet packages and the built-in dependency resolution would enforce at build time what drivers can be used with what frameworks. Without this, all the drivers have become reflection-based for the .NET Core framework builds. The focus should shift to getting this final difference into a release. |
This is
NH-3807#954 - A Full .NetStandard 2.0 and .Net Core App 2.0 conversion. It is currently a work in progress, but is working well enough to be reviewed and receive feedback.It is my opinion that if it doesn't run on Linux/macOS, then it's not really compatible with .NetStandard. That means the default fallback of importing .Net Framework assemblies needs disabled in each
.csproj
targeting .NetStandard or .NetCoreApp:This version is different than @hazzik's NH-3807 branch in than it aims to fully delineate what parts are available on .Net Core, and what parts are going to require the full .NET Framework. This version does that by splitting the functionality up among the different database drivers. A .Net Framework 4.6.1 output is still cross-compiled and included in the NuGet packages for full traditional functionality, and to avoid the extra NuGet imports that happen with .netstandard only packages.
What's working:
dotnet build
on both Windows and Linux.dotnet test
on both Windows and Linux.What's left:
Remotion.Linq.EagerFetching
.Currently using checked-in copy of attachment available here.3110 failed tests on Windows when running as a .NetCoreApp.3120 failed tests on Linux.BuildUsing assembly versions didn't set the NuGet package version correctly.SharedAsemblyInfo.cs
on Linux.NH-3944 - Upgrade Remotion.Linq to v2.1 #568 - NH-3944- Upgrade Remotion.Linq to v2.1.NH-4043 - Complete keyword registration in dialects. #654 - NH-4043- Complete keyword registration in dialects.Compile .NetStandard 1.0 iesi.collections#3Integrate with Ant build system.Planning on using fully native msbuild instead.NHibernate.Test.VisualBasic
to .NetCoreApp(not supported in the preview 1 CLI?)NHibernate.TestDatabaseSetup
to .NetCoreAppSwitchHbmXsd usesNHibernate.Tool.HbmXsd
to .NetCoreAppXmlCodeExporter
, which is not available in .NetCoreAppNHibernate.Example.Web
to ASP.Net Core 2.0. Started with NH-4073 - Replace NHibernate.Web.Example with modern version. #675 - NH-4073.dotnet build
generated NuGet packages?dotnet build fails to build a project containing Antrl3 targets antlr/antlrcs#71will need to be resolved. What if it still won't run on Linux? Do the ANTLR definition files change all that often?There is already an open DataReader associated with this Connection which must be closed first.
Microsoft's SQLiteonly allows one transaction, so errors when trying to get HiLo id values. Allowing multiple connections results in deadlocks.Some small non-breaking changes can be merged ahead such as
#632,#634, and#695.What is different missing on .NET Core vs. full .NET Framework?
new TransactionScope()
may not work like you expect. You will get a run-time exception.Serialization of the configuration or session isn't going to work. You will get a run-time exception.(Compatible serialization pending in Manually serialize system types in preparation for .NET Core. #1425)Image
andBitmap
. You will get a compile-time error.Protocol=memory;
from the connection string and add the Port number.Other tasks? Thoughts?
To test:
These are being built in Appveyor and CircleCI. The built NuGet packeges are available on this Appveyor NuGet feed. The versions look like this: 5.0.0-ci-00138-dbg-NH-3807.