Skip to content

.NET Core 2.0 and .NET Standard 2.0 support #1523

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

Merged
merged 5 commits into from
Feb 28, 2018

Conversation

hazzik
Copy link
Member

@hazzik hazzik commented Jan 11, 2018

These are minimal changes required to make a .NET Core 2.0 and .NET Standard 2.0 builds.

There are no test infrastructure yet, but all tests pass locally on windows dotnet core.

Summary of changes:

  1. Assume that serialization of System.Type and co is a foreign concept to .NET Core 2.0 so the users required to write serialization surrogates. We might provide these in the future if we want to.
  2. SqlClient, Odbc, Oledb drivers are converted to ReflectionBasedDriver to avoid the extra dependencies.
  3. There are some problems loading App.config when test run from VS/dotnet test. This is a known issue in NUnit adapter/dotnet. We can avoid this issue by switching to NUnitLite to run .NET Core 2.0 tests.
  4. CallSessionContext uses a static AsyncLocal field to mimic the CallContext behavior.
  5. .NET Core 2.0 DriverBase indicates that System.Transaction is not supported (.NET Standard that it is supported).

@hazzik
Copy link
Member Author

hazzik commented Jan 11, 2018

Build is failing because NAnt is trying to tell MSBuild to build the project into the same folder.

@LightCZ
Copy link

LightCZ commented Jan 24, 2018

What is the difference between this pull request and #633 ? As i understood this one, this should be the minimal required changes but not finished and #633 have some major breaking changes also. Is that correct?

@hazzik
Copy link
Member Author

hazzik commented Jan 24, 2018

@LightCZ pretty much. This one has minimal changes. The other one is with breaking changes. Both of them lack the CI changes so builds fail.

@fredericDelaporte
Copy link
Member

Someone steals my console?

Not me, if you were referring to the TeamCity build agent.

@hazzik
Copy link
Member Author

hazzik commented Feb 9, 2018

hahaha, no. Check the actual commit. It seems that something is redirecting the console output so the timer did not write to it.

@hazzik hazzik force-pushed the netcoreapp2.0 branch 7 times, most recently from 55bad29 to 508beb9 Compare February 14, 2018 04:08
@hazzik
Copy link
Member Author

hazzik commented Feb 20, 2018 via email

@@ -31,10 +31,9 @@
<property name="dialect">NHibernate.Dialect.MsSql2008Dialect</property>

<property name="connection.driver_class">NHibernate.Driver.Sql2008ClientDriver</property> <!-- Shouldn't be necessary, but is required by some tests -->
<property name="connection.connection_string_name">TestConnectionString</property>
<property name="connection.connection_string">Server=localhost\sqlexpress;Database=nhibernate;Integrated Security=SSPI</property>
Copy link
Member

Choose a reason for hiding this comment

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

This change somewhat alters the CanGetNamedConnectionStringFromConfiguration test: previously this test was testing overriding a file configured connection.connection_string_name, while now it tests that connection.connection_string_name setting takes precedence over connection.connection_string.

And this change also leaves a now unneeded TestConnectionString connection string.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a pre-nunitlite change, so, potentially can be reverted. I'll investigate. Probably if this will be reverted, it would be [almost] impossible to test in VS.

@hazzik
Copy link
Member Author

hazzik commented Feb 22, 2018

@fredericDelaporte I've fixed most of the concerns, could you please take another look?

@fredericDelaporte
Copy link
Member

Yes but it may take a while, I am not at home and currently spend most of my (awaken) time outside.

System.Reflection.Assembly.GetEntryAssembly()?.GetName().Name == "testhost";

public static void AssumeSystemTypeIsSerializable() =>
Assume.That(typeof(System.Type).IsSerializable, Is.True);
Copy link
Member

Choose a reason for hiding this comment

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

Since Assume has a semantic implying failure on theories if a theory fail the constraint for all its cases, we likely should instead use Ignore. Otherwise using this helper in a theory will still cause it to fail, while we use it for ignoring the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I’ll change this to Ignore instead.

- When executing these methods on .NET Core on Linux they were failing with AccessViolationException crushing the application.
@hazzik hazzik force-pushed the netcoreapp2.0 branch 2 times, most recently from 21ca57c to 3ebc5b6 Compare February 26, 2018 02:12
@hazzik hazzik merged commit 45a63bf into nhibernate:master Feb 28, 2018
@hazzik hazzik added this to the 5.1 milestone Feb 28, 2018
@stevenlauwers22
Copy link

Awesome! Looking forward to the release of 5.1!

<Reference Include="System.ServiceModel" />
<Reference Include="System.Transactions" />
<Reference Include="System.Configuration" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.0'">
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that building the core library for netcoreapp2.0 is unnecessary.

Is leaving off the System.Reflection.Emit package references worth it for the increased bulk in the NuGet package?

@cloviscoli
Copy link

Any forecast when we will have a release?

@stephan-simondshealth-com-au
  1. I'm also hoping this will make it into a release soon. IMHO supporting a newer version of .NET Core (Even if its not fully) would justify a release. But I'm probably biased :-)

  2. In the mean time, are there overnight NuGet package that I can pull?
    (For instance: Is there an AppVeyor project feed?) Thanks !

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Mar 8, 2018

I'm also hoping this will make it into a release soon. IMHO supporting a newer version of .NET Core (Even if its not fully) would justify a release. But I'm probably biased :-)

So do I, but some pending PR deserve being merged in 5.1. Of course at some point those taking too long to finalize may be postponed for a later version.

In the mean time, are there overnight NuGet package that I can pull?

The CI builds include a "NHibernate (Release Package)" TeamCity build, having in its artifact NuGet packages, the binary release zip and more. Please note that they are still versioned according to latest release: we increment the version only when actually performing a release.

You can take them from master builds or from a specific PR, as you wish. Access the latest master builds from the commits page. Clicking on the green check (GitHub desktop site version, likely not available on mobile version) should show you the builds list. Then navigate on "details" link of the release build (like this one). TeamCity will ask you to login if you are not already. An anonymous logon option is available. Then go on the artifacts tab (here by example) and download what you need.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Mar 15, 2018

About:

  • MySQL on
    • Windows - 4 tests are failing (same tests fail on Full .NET Framework on Windows with 6.10.x provider, 2 of them due to the bug in MySql data adapter);

As written later by Alexander, "the bug in MySql data adapter" is here.

Now the two others are also due to a bug in MySql data adapter 6.10, now reported here. (It is closely related to another one and may be considered a duplicate.)

This other bug is due to MySql.Data 6.10 command disposal closing now any open data reader found on the connection, be it related to the command or not. So a workaround would be to close previously opened command before opening a data reader with a new command. But in the case of the failing test, this would need introducing more coupling, because the current sequence of events implies a JoinedEnumerable of EnumerableImpl holding NHybridDataReader, which are for all of them but the last read into memory by the batcher at a place where their matching command is not known (and anyway, that is not the batcher responsibility to close commands here). The JoinedEnumerable disposes of its current EnumerableImpl as soon as it finishes iterating over it and before starting iterating the next one. This dispose causes the current EnumerableImpl to dispose of the command it is bound to, and when it is a MySql command, this command then dispose any reader found on the connection, resulting in closing the reader of the last EnumerableImpl in the JoinedEnumerable.

So another workaround could be to change JoinedEnumerable for ceasing disposing an EnumerableImpl when it has exhausted it, but instead disposing all of them when it has exhausted all of them. But well, I would rather wait for MySql Connector to get fixed.

@bgrainger
Copy link

I would rather wait for MySql Connector to get fixed.

This may be a long wait...

I have created a competing OSS library, https://github.com/mysql-net/MySqlConnector, that is a drop-in replacement for Oracle's Connector/NET (aka MySql.Data). (One of the motivating factors in creating this alternative was to fix longstanding Connector/NET bugs that weren't getting addressed. It also supported .NET Standard before Connector/NET did.)

I understand that many NHibernate users are using Connector/NET, so it makes sense to test with the library your users are using; however, if you'd like a higher-quality MySQL ADO.NET library then I suggest testing with MySqlConnector (either in addition to or instead of Connector/NET). (As the primary author of MySqlConnector, I'd be happy to assist with any integration questions.)

@hazzik hazzik deleted the netcoreapp2.0 branch April 27, 2018 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants