-
Notifications
You must be signed in to change notification settings - Fork 936
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 .NET 6 and .NET Framework 4.8 targets #3000
Conversation
Looks like ANTLR generator has some concurrency issue as github build now throw from time to time exceptions like: Error: /home/runner/.nuget/packages/antlr3.codegenerator/3.5.2-rc1/build/Antlr3.CodeGenerator.targets(146,5): error AC1000:
Unknown build error: : error 10 : internal error:
System.IO.FileNotFoundException: Could not find file '/home/runner/work/nhibernate-core/nhibernate-core/src/NHibernate/obj/Release/netcoreapp2.0/Hql__.g'. File name: '/home/runner/work/nhibernate-core/nhibernate-core/src/NHibernate/obj/Release/netcoreapp2.0/Hql__.g'
at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory, Func`2 errorRewriter)
at Interop.CheckIo(Error error, String path, Boolean isDirectory, Func`2 errorRewriter)
at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String path, OpenFlags flags, Int32 mode)
at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize) at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize)
at Antlr3.Tool.GrammarSpelunker.Parse()
... |
Why do we need net48 target in the library? In test I understand, and wanted to implement it, but in the library? |
Why not? Since it's already used in tests, I thought it wouldn't hurt to have it for lib too. And some optimizations are still possible on this target (like here) |
@fredericDelaporte Seems hazzik made it up to you (enabled auto-merge). Any objections? |
I wanted to merge it after some other PR and after 5.3.x merge, so enabled auto merge. Also, I’m a little hesitant on adding .NET4.8 target. |
I think it makes sense to add that 4.8 target, provided we consider the NuGet size is not an issue. .Net Framework is far from its end of life: enabling some more optimizations already done for other targets should be beneficial to lots of users. (I have seen a 4.8.1 has been released in August, but we should not target it in my opinion. It adds arm support, which users can still get by targeting their application on 4.8.1, without needing NHibernate targeting it too. Moreover that version is available only on most recent versions of Windows OSes.) |
What exactly do you mean, @fredericDelaporte? Optimizations in the NHibernate, or in the Framework? If later then it is all runtime dependent and does not depend on the target. |
I mean the former, like mentioned by Roman.
|
@fredericDelaporte please approve and merge if you're happy with the changes |
No description provided.