Refactored range type handler#2060
Refactored range type handler#2060YohDeadfall merged 1 commit intonpgsql:devfrom YohDeadfall:CoreRTCompilationFix
Conversation
|
I may be missing something, but I don't see how this PR solves the CoreRT issue (theoretical endless nested Some of the other changes you've done (unrelated cleanup/refactoring) could be fine though, but it's better to separate them out into another smaller PR. |
|
In short, you are missing that the As you described #2042, the issue was caused by an instance of the |
There was a problem hiding this comment.
Move it to an additional PR?
There was a problem hiding this comment.
I think it's small enough to leave in...
|
OK, I now understand the idea. I'm just not sure I completely understand precisely what CoreRT is doing and precisely what makes it fail on the current code. Can I ask you to simply test CoreRT on your PR? If the error goes away I agree that it is a much better way to solve it than my PR... |
|
The code below compiles without any error. static void Main(string[] args)
{
using (var conn = new NpgsqlConnection("Server = localhost; User ID = npgsql_tests; Password = npgsql_tests; Database = npgsql_tests; Timeout = 0; Command Timeout = 0"))
using (var cmd = new NpgsqlCommand("SELECT int4range(10, 20)", conn))
{
conn.Open();
using (var rdr = cmd.ExecuteReader())
if (rdr.Read()) Console.WriteLine(rdr.GetFieldValue<NpgsqlRange<int>>(0));
}
}PS D:\Source\Repos\npgsql\test\Npgsql.CoreRT> dotnet publish -r win-x64 -c release
Microsoft (R) Build Engine version 15.7.179.6572 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.
Restoring packages for D:\Source\Repos\npgsql\src\Npgsql\Npgsql.csproj...
Restoring packages for D:\Source\Repos\npgsql\test\Npgsql.CoreRT\Npgsql.CoreRT.csproj...
Restore completed in 219.55 ms for D:\Source\Repos\npgsql\src\Npgsql\Npgsql.csproj.
Installing runtime.win-x64.Microsoft.DotNet.ILCompiler 1.0.0-alpha-26718-02.
Generating MSBuild file D:\Source\Repos\npgsql\test\Npgsql.CoreRT\obj\Npgsql.CoreRT.csproj.nuget.g.targets.
Restore completed in 42.22 sec for D:\Source\Repos\npgsql\test\Npgsql.CoreRT\Npgsql.CoreRT.csproj.
Npgsql -> D:\Source\Repos\npgsql\src\Npgsql\bin\release\netstandard2.0\Npgsql.dll
Npgsql.CoreRT -> D:\Source\Repos\npgsql\test\Npgsql.CoreRT\bin\release\netcoreapp2.0\win-x64\Npgsql.CoreRT.dll
Generating native code
Npgsql.CoreRT -> D:\Source\Repos\npgsql\test\Npgsql.CoreRT\bin\release\netcoreapp2.0\win-x64\publish\ |
|
OK, and just to be 100% sure you're also seeing an infinite type recursion issue when trying to build without your changes? (FYI I actually tried to test with corert myself but there was a build failure trying to publish for linux-x64). |
fe46200 to
1f156c8
Compare
|
Yes, I see it without that fix. |
roji
left a comment
There was a problem hiding this comment.
OK great, good to merge them!
It also means there are no other issues with corert, which is good news...
Do you think we should release this for 4.0.3? It seems like a pretty low-risk refactor...
|
For now it compiles using the AOT compiler, but it doesn't work because of heavily used reflection in Npgsql. At least we should provide a basic rd.xml file or notify about it in the documentation. I will work on it, but it's not a blocking issue to release the fix in 4.0.3, I think. |
|
@YohDeadfall thanks for your work on this. Maybe open an issue to track adding rd.xml? |
|
Opened #2073. |
|
Backported to 4.0.4 via 6487cc6. |
This is my idea how the range handler should be refactored to fix #2040. The only one error I encountered is shown below. Will investigate this tomorrow.