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

Support NetStandard 2.0 #12

Merged
merged 15 commits into from Apr 14, 2018

Conversation

Projects
None yet
2 participants
@jpenniman
Contributor

jpenniman commented Mar 18, 2018

I updated the target to netstandard2.0 for the NHibernate.Mapping.Attributes.csproj. All the tests pass. I also added a license file and updated the pack.cmd script to use dotnet pack. I wasn't sure about what to do with the teamcity.build stuff.

There's a package available on our MyGet feed:
https://www.myget.org/feed/milestonetg-public/package/nuget/NHibernate.Mapping.Attributes

Fixes #11.

jpenniman added some commits Mar 18, 2018

Changed build tartget to netstandard2.0
* Changed build target to netstandard2.0
* Modified pack.cmd to use dotnet pack instead of nuget pack
@fredericDelaporte

Some undue additions have sneaked in, please remove them. (See comments + NuGet.exe.) Licence is quite unrelated too but well, why not.

There are also some adjustments to do.

So you choosed to supply only a netstandard 2.0 build. Why not, but I (or another contributor) have to check if this may have any downside for usage with full .Net framework.

About the TeamCIty build, well, it is broken since quite some time, so I do not think anything should be done about it in this PR.

@@ -0,0 +1,51 @@
<GhostDoc>

This comment has been minimized.

@fredericDelaporte

fredericDelaporte Mar 19, 2018

Member

Please remove.

//[assembly: AssemblyVersionAttribute("5.0.0")]
//[assembly: AssemblyInformationalVersionAttribute("5.0.0")]
//[assembly: AssemblyFileVersionAttribute("5.0.0.0")]
////[assembly: AssemblyKeyFileAttribute("../../src/NHibernate.snk")]

This comment has been minimized.

@fredericDelaporte

fredericDelaporte Mar 19, 2018

Member

Remove them rather than commenting them.

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Mar 20, 2018

So you choosed to supply only a netstandard 2.0 build. Why not, but I (or another contributor) have to check if this may have any downside for usage with full .Net framework.

Well, one likely downside is for user of older VS, which may not handle correctly adding netstandard dependencies in a netFx framework. So the project should provide a multi-target instead.

The test project csproj format would also have to be migrated to the new csproj format in such case, in order to have test both net461 attribute build by targeting net461, and netstandard build by targeting netcoreapp2.0.

@jpenniman jpenniman referenced this pull request Mar 21, 2018

Closed

Support NetStandard 2.0 #11

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Mar 21, 2018

Targets

There seems to be an issue that relates to the project system and nuget: dotnet/sdk#1561

It appears there is issues when a multi-targeted library is used by another multi-targeted library when more than one satisfies the query. This is causing issues for folks who want to multi-target their testing library. It also causes an issue for any multi-targeted library referencing another multi-targeted library.

FWIW, VS2015 supports netStandard2.0: dotnet/cli#6739. Net461 requires VS2015 anyway (to use C#6 features). Tooling below VS2017 is no longer supported by Microsoft.

Targeting netstandard2.0;net461 feels a bit redundant, since net461 is an implementation of the netstandard2.0 spec and attributes doesn't need to toggle features based on the target like nhibernate did (ie binary serialization). As you said, the only hangup might be older tooling, but net461 requires VS2015 and VS2015 supports referencing netstandard2.0 libraries, so we're really not supporting older tooling by targeting netstandard2.0 and net461. Anyone using NHibernate 5.1 most likely will not be using anything older than VS2015 since nhibernate also requires net461+.

Microsoft themselves only targets netstandard2.0 for libraries that support net461+ and netcoreapp2.0+. The only time you really see multi-targeting is if you need to support net45 and netstandard2.0 for example.

I'll target both if you really want. It just feels like it's going to cause more issues than it's going to solve. Just targeting the netstandard2.0 keeps everything else much simpler--testing, build/release automation, etc.

Nuget.exe

Nuget.exe was already there, just a very old version (2.6). The new csproj format requires Nuget.exe >=4.3.0 and >=4.1.0 is required to publish to nuget.org, so I upgraded it as part of the solution supporting core.

Alternatively, we could just delete the nuget and teamcity stuff all together and clean up the solution. If you want, I can host an automated build/release in my company's Visual Studio Online account and integrate it with Github.

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Mar 21, 2018

It appears there is issues when a multi-targeted library is used by another multi-targeted library when more than one satisfies the query. This is causing issues for folks who want to multi-target their testing library. It also causes an issue for any multi-targeted library referencing another multi-targeted library.

The linked issue dotnet/sdk#1561 is about being able to test all targets of the dependent library. We do not have this trouble with a multi-target of just one netFx and one netStandard, and it can be tested without issues, as done in NHibernate-Caches currently. NHibernate-Core targets furthermore netcoreapp, and so yes, it does not test netstandard indeed. We just accept this.

It also causes an issue for any multi-targeted library referencing another multi-targeted library.

Can you provide precise example for us to check if this may apply to our case here?

Net461 requires VS2015 anyway (to use C#6 features).

This does not prevent using the 4.6.1 framework in VS2013 or 2012, and I know of many companies which do. Of course if some guy works with a newer VS and commits C#6/7 syntax, he will causes troubles to his colleagues, but generally that will be the guy not using the expected tooling in the company that will be blamed.

Tooling below VS2017 is no longer supported by Microsoft.

Quite wrong. The latest update of the tooling must be checked in this site, not the "All edition".
VS 2015 end of support is in 2025.
VS 2013 end of support is in 2024.
VS 2012 end of support is in 2023.
Indeed even VS 2008 is still supported.

Anyone using NHibernate 5.1 most likely will not be using anything older than VS2015 since nhibernate also requires net461+.

No, definitely wrong. Many corporate organizations are still using older VS than 2015 while using NHibernate and net461.

It just feels like it's going to cause more issues than it's going to solve. Just targeting the netstandard2.0 keeps everything else much simpler--testing, build/release automation, etc.

It keeps things simpler for contributors, but it will likely deny the library usage to many developers working at companies having perpetual licences of VS 2013 or older. Those companies are understandably quite reluctant to upgrade their tooling.

Nuget.exe

My bad for NuGet. I am used to other projects like NHibernate-Caches or NHibernate-Core where it is downloaded by the build scripts. So ok for its update.

If you want, I can host an automated build/release in my company's Visual Studio Online account and integrate it with Github.

Thanks for the proposition, but better do this as an AppVeyor build by example, which can be bound to NHibernate GitHub organization, instead of being bound to a third party account. I can activate AppVeyor for NHibernate.Mapping.Attributes if you wish to handle this (but please do it within another PR in such case).

Thanks by the way for your work, it is in any case appreciated.

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Mar 21, 2018

Makes sense. I'll setup the multi-target for net461 and netstandard2.0. I'll follow NHibernate-Core as an example for setting up the test projects so it's consistent.

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Mar 22, 2018

Should DomainModel.hbm.xml actually be consumable by NHibernate?

The HbmWriter behaves a little differently in corefx. Because there is no GAC, type string returned from HbmWriterHelper.GetNameWithAssembly(Type) will always follow the not GAC'd path, so its tests fail in core because it is expected that GAC'd assemblies include the version and publickeytoken attributes.

I wanted to test that the resulting mapping document is still valid, but DomainModel.hbm.xml nor the reference one seem to work in net461 or netcoreapp. The error is;

Error Message:
 NHibernate.MappingException : These classes referenced by 'extends' were not found:
FullName:Guid - Name:Guid
FullName:Guid - Name:Guid
FullName:Guid - Name:Guid
FullName:Guid - Name:Guid
Stack Trace:
   at NHibernate.Cfg.MappingsQueue.CheckNoUnavailableEntries()
   at NHibernate.Cfg.Configuration.SecondPassCompile()
   at NHibernate.Cfg.Configuration.BuildMappings()
@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Mar 22, 2018

This is likely a trouble requiring a fix, maybe on NHibernate side, or maybe here. I am not an user of Attributes myself, so it may take me some time to check what is going on. (Moreover I am still working on Caches currently.)

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Mar 22, 2018

I'll dig into a little more. I'll try version 5.0 of each and see what happens. I'll also try 5.1 with one of our projects that's using NHibernate and Attributes and see if the same issue is there.

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Mar 27, 2018

Well now that I had a look on sources, it looks to me like DomainModel.hbm.xml is not valid. It should have classes extending the Guid struct. This has likely been done solely for testing the extend feature which allows to map subclass in a different hbm file than the one declaring the base class. But it is not done not in a valid way. It should extends a mapped base class for this to be valid.

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Mar 27, 2018

And at the start of the domain file:

/*\
 * The mapping used in the following classes has no meaning for NHibernate;
 * it is here for the sole purpose of testing NHibernate.Mapping.Attributes
 * in all possible usage cases.
\*/

So fixing that Guid issue is likely just fixing a first trouble, with many more to handle after that.

I think the test trouble just need to be fixed by adding .CoreReference.hbm.xml version of the reference files.

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Mar 30, 2018

Yeah, that was my sense... the mapping doesn't actually work in NHibernate. I will create a Core version of the reference for testing serialization.

I also have an RI for DDD with NHibernate that has some common scenarios. I want to test the core version with that project just to make sure the resulting doc actually works. I'm sure it does, I just want to be sure.

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Apr 4, 2018

Since I am done with porting Caches to Core (and even Validator), I can spare some time to finish the work on Attributes, if this suits you. But I believe you have already done some additional work without pushing it there. So let me know if you wish me to finish this PR or not, from its current state or after some additional pushes from you.

jpenniman added some commits Apr 4, 2018

Cleaned up AssemblyInfo.cs
Removed assembly attributes that are now generated from csproj properties.
Multi-trargeted NHibernate.Mapping.Attributes
Package now targets netStandard2.0 and net461 to support older tooling.
Changed test project to target net461 and netCoreApp2.0
Multi-targeted the test project so both runtimes can be tested.
@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Apr 4, 2018

I committed some changes. The tests fail under core, but it's more of how the tests are written.

dotnet test -f net461 passes.

dotnet test -f netCoreApp2.0 fails because there is no GAC and the Assertion is incorrect when dealing with core.

I tested an actual domain library from one of our applications, and it seems to work so far--at least our unit tests pass. So, I think the library is good, we just need to figure out how to properly test it.

Updated tests to support differences in core.
* Core doesn't have a gac, so the test for gac'd assemblies is ignored in core.
* New reference mappings have been added to reflect the gac's vs non-gac'd serialization in netfx vs core.
@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Apr 4, 2018

I updated the tests so they pass. I took your recommendation and generated a reference mapping for core and use it in the serialization tests for core. I also ignore the GAC'd assembly serialization helper test since there is no GAC in core.

Make sources consistent with others nhibernate repositories
 * Have projects common properties
 * Treat warnings as errors
 * Set and use NETFX directive
 * Fix tests for working within IDE
 * Migrate custom tool to new csproj
 * Remove obsolete assets

@fredericDelaporte fredericDelaporte added this to the 5.1.0 milestone Apr 5, 2018

@fredericDelaporte fredericDelaporte changed the title from #11 Support NetStandard 2.0 to Support NetStandard 2.0 Apr 5, 2018

@fredericDelaporte

This comment has been minimized.

Member

fredericDelaporte commented Apr 13, 2018

@jpenniman, I was believing to have asked here if the changes I have added were suiting you, right after having pushed them. And I was waiting an answer. But it seems I have failed to post the question somehow...

Anyway, are my changes ok for you?

@jpenniman

This comment has been minimized.

Contributor

jpenniman commented Apr 14, 2018

@fredericDelaporte, yes, I agree with the changes.

@fredericDelaporte fredericDelaporte merged commit ad6b759 into nhibernate:master Apr 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment