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

Target netstandard2.0 and NH 5.1.0 #391

Merged
merged 10 commits into from
Mar 28, 2018

Conversation

vincentparrett
Copy link
Contributor

NHibernate 5.1 was released in the last 24hours, with support for netstandard2.0. This PR attempts to add that support. To get it to build I had to delete global.json so it would use the lastest non preview sdk version (2.1.101) - I didn't commit that.

@jrgcubano
Copy link
Member

jrgcubano commented Mar 20, 2018

Great job @vincentparrett. I will take a close look tomorrow. Thank you!

Copy link
Member

@jrgcubano jrgcubano left a comment

Choose a reason for hiding this comment

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

@vincentparrett.

I think we can safely update global.json to:

{
    "projects": [
        "src"
    ],
    "sdk": {
        "version": "2.1.4"
    }
}

And maybe update the tests and examples projects to 5.1.0 too.

Thanks in advance for your work! 👍


<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="NHibernate" Version="5.1.0" />
<PackageReference Include="System.Data.SqlClient" Version="4.4.3" />
Copy link
Member

@jrgcubano jrgcubano Mar 20, 2018

Choose a reason for hiding this comment

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

We need System.Data.SqlClient?

Choose a reason for hiding this comment

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

Yes, it seems it is needed for the MsSqlConnectionStringBuilder

Copy link
Contributor

@danilobreda danilobreda Mar 20, 2018

Choose a reason for hiding this comment

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

We could create our own implementation of the builder like the MySqlConnectionStringBuilder. Because we use the connectionString only, and have no interaction with the sqlconnectionstringbuilder for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

The use of a package that the developer does not have a interaction with it is not a good implementation. (the user only pass the connectionString as string)

Copy link
Member

@jrgcubano jrgcubano Mar 21, 2018

Choose a reason for hiding this comment

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

@danilobreda @mgoodfellow Seem right. You can take care of this in another PR when we finish this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrgcubano yes i can, i will wait for this release.

@vincentparrett
Copy link
Contributor Author

Not sure why the build is failing?

<Import Project="$(MSBuildThisFileDirectory)..\Shared.msbuild" />

<ItemGroup Condition="'$(TargetFramework)' == 'net461'">
<PackageReference Include="NHibernate" Version="5.0.3" />

Choose a reason for hiding this comment

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

I think this should be 5.1.0 as well now? Because all the tests are based on 5.1.0 so we should target this entire release to that? 5.1.0 does support net461 as well

@vincentparrett
Copy link
Contributor Author

There are a bunch of other package updates available, all minor updates except for FluentAssertions, should I take those updates as part of this PR?

@jrgcubano
Copy link
Member

jrgcubano commented Mar 21, 2018

Great job @vincentparrett! 👍

The error is due to a problem with AppVeyor. For now, you could comment the next line on "build.cake":

AppVeyor.UpdateBuildVersion(parameters.Version.SemVersion);

We will return to it when it works again..

Apart of this, we need a few more changes to get this done:

Projects:

  • Add netcoreapp2 to TargetFrameworks on FluentHibernate.csproj (to keep compability with vs2017 and nuget 3)
  • Add netcoreapp2, netstandard2 and net461 groups and libs to FluentNHibernate.nuspec
  • Add netcoreapp2 to TargetFrameworks on TestProjects.
  • Update shared.msbuild to include the constants for each framework (use https://github.com/cake-build/cake/blob/develop/src/Shared.msbuild as a reference)

Build process (build.cake)

Do not worry now about the test projects packages version updates. Leave them for another PR.

Let me know if you need help with this!

Thanks!

@vincentparrett
Copy link
Contributor Author

I have done all requested except for

Add netcoreapp2 to the "Test task" on build.cake.

Not really sure how to handle that.. beyond my experience with dotnet core or with cake.

@jrgcubano
Copy link
Member

Thanks again @vincentparrett. 👍

Just one last step and we are ready.

Projects:

  • Remove netstandard2 from the test projects TargetFrameworks

Build process (build.cake)

  • For the test task you can try with something like this or maybe DotNetCoreTest...:
Task("Test")
    .IsDependentOn("Build")
    .Does(() =>
    {                
        var frameworks = new [] { "net461", "netcoreapp2" };
        foreach (var framework in frameworks)
        {
			var testAssemblies = $"./src/**/bin/{parameters.Configuration}/{framework}/*.Testing.dll";  
			NUnit3(testAssemblies, new NUnit3Settings {
				NoResults = true
			});

			testAssemblies = $"./src/**/bin/{parameters.Configuration}/{framework}/*.Specs.dll";  
			MSpec(testAssemblies, new MSpecSettings {
				Silent = true
			});
        }   
    });

@jrgcubano jrgcubano modified the milestone: 2.1.1 Mar 22, 2018
@vincentparrett
Copy link
Contributor Author

The test projects do not work with netcoreapp2

  1. Error :
    An exception occurred in the driver while loading tests.

Server stack trace:
at NUnit.Engine.Runners.DirectTestRunner.LoadDriver(IFrameworkDriver driver, String testFile, TestPackage subPackage)
at NUnit.Engine.Runners.DirectTestRunner.LoadPackage()
at NUnit.Engine.Runners.DirectTestRunner.EnsurePackageIsLoaded()
at NUnit.Engine.Runners.DirectTestRunner.RunTests(ITestEventListener listener, TestFilter filter)
at System.Runtime.Remoting.Messaging.StackBuilderSink._PrivateProcessMessage(IntPtr md, Object[] args, Object server, Object[]& outArgs)
at System.Runtime.Remoting.Messaging.StackBuilderSink.SyncProcessMessage(IMessage msg)

Exception rethrown at [0]:
at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke(MessageData& msgData, Int32 type)
at NUnit.Engine.ITestEngineRunner.Run(ITestEventListener listener, TestFilter filter)
at NUnit.Engine.Runners.ProcessRunner.RunTests(ITestEventListener listener, TestFilter filter)

Test Run Summary
Overall result: Failed
Test Count: 0, Passed: 0, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 0
Start time: 2018-03-23 12:15:34Z
End time: 2018-03-23 12:15:34Z
Duration: 0.310 seconds

An error occurred when executing task 'Test'.

@vincentparrett
Copy link
Contributor Author

Seems like the nunit console runner doesn't work with netcoreapp2, I tried updating nunit to the latest version, that didn't help, tried updating nunit console but that made things worse.
Perhaps it might be better to focuse on netstandard2.0 anyway, since thats where things are heading?

@vincentparrett
Copy link
Contributor Author

I have not been able to get the unit tests running for netcoreapp2.0 - perhaps someone else would have better luck, I don't have any more free time to spend on this.

@mgoodfellow
Copy link

@vincentparrett I have found with NUnit + netcoreapp2.0 you also need Microsoft.NET.Test.Sdk added to the project. Depending on the environment it is running in it also needs "runners". To run from within Resharper in VS, you need NUnit3TestAdapter. There is a runners package that includes a bunch of them which might be a good idea considering it will be running on CI servers.

@vincentparrett
Copy link
Contributor Author

@mgoodfellow The runners package is marked as deprecated, and suggests to use the console. The error I'm stuck on now is :

Invalid platform type:AnyCpu. Valid platform types are x86, x64 and Arm.

@cfletcher
Copy link

cfletcher commented Mar 26, 2018

@mgoodfellow nunit/nunit-console#364 link to issue on the nunit-console

@cfletcher
Copy link

cfletcher commented Mar 26, 2018

@vincentparrett it looks as though DotNetCoreTestSettings takes an "ArgumentCustomization" that would let you specify a Platform: https://cakebuild.net/api/Cake.Common.Tools.DotNetCore.Test/DotNetCoreTestSettings/
But I've had no success with ArgumentCustomization = args=>args.Append(" --Platform:x64")
I see:

MSBUILD : error MSB1001: Unknown switch.
Switch: --Platform:x64
For switch syntax, type "MSBuild /help"
An error occurred when executing task 'Test'.`

@jrgcubano
Copy link
Member

jrgcubano commented Mar 27, 2018

@vincentparrett Great job!

It seems that there are external libraries pending to upgrade to netcore2.0, so I agree with you, let's move forward and we will be adding tests for netcore2.0 on other version.

  • Machine.Specifications and NUnit do not work yet with netcore2.0.
  • In general, we should not worry about the platform (we need portable with netcoreapp2.0). So just add the netcore constants and portable debug type to Shared.msbuild and we are ready to go!
  <PropertyGroup>
    <NetStandardImplicitPackageVersion Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">2.0.0</NetStandardImplicitPackageVersion>
  </PropertyGroup>
  
  <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
    <DefineConstants>$(DefineConstants);NETCORE</DefineConstants>
    <DebugType>portable</DebugType>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
    <DefineConstants>$(DefineConstants);NETCORE</DefineConstants>
    <DebugType>portable</DebugType>
  </PropertyGroup>

Apart of this, I'm planing a tests refactor to move all (specs and unit) to xunit on other PR.

@jrgcubano jrgcubano merged commit 1f95680 into nhibernate:master Mar 28, 2018
@fabriciomrtnz
Copy link

fabriciomrtnz commented Apr 16, 2018

Hola @jrgcubano,

How is this coming along? ETA?

Gracias!

@jrgcubano
Copy link
Member

We finish the release in #400 @fabriciomrtnz

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants