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 Netstandard 2.0 #314

Merged
merged 15 commits into from
Nov 18, 2018
Merged

Conversation

samhanes
Copy link
Contributor

No description provided.

@samhanes samhanes mentioned this pull request Oct 15, 2018
@samhanes
Copy link
Contributor Author

samhanes commented Oct 15, 2018

I see that the DTC has a dependency on Microsoft.SqlServer.TransactSql.ScriptDom and that is not available as a netstandard library - I'm trying to determine where that leaves us.

As far as I can tell, the RTC doesn't have that dependency, and so it could be compiled as a netstandard library, but I believe that would require separating the components into different binaries.

@vasily-kirichenko did you have an idea about how to work around the ScriptDom dependency? Or is the design time side simply not going to work without full framework (or Mono)?

@samhanes
Copy link
Contributor Author

It looks like the only non-netstandard dependency holdout is ScriptDom. I can't find much information about the state of that library - it's not open source and I have no idea if there is a netstandard version in the works.

I'm moving forward with the assumption that we'll have to use the workaround in order to get this working with the full framework version of ScriptDom - from everything I have read, that should be doable.

@samhanes
Copy link
Contributor Author

samhanes commented Oct 19, 2018

Making progress splitting into separate runtime and design time assemblies, but now I'm stuck on a compiler error:

A problem occurred writing the binary 'C:\code\FSharp.Data.SqlClient\src\SqlClient.Tests\obj\Debug\net451\SqlClient.Tests.dll': Error in pass3 for type FSharp.Data.EnumTests, error: Error in GetMethodRefAsMethodDefIdx for mref = ("Parse", "TinyIntMapping"), error: Exception of type 'Microsoft.FSharp.Compiler.AbstractIL.ILBinaryWriter+MethodDefNotFound' was thrown.

This may be due to updating to the latest version of ProvidedTypes, but I haven't verified that one way or the other. My guess is that the temporary assembly for the generative types is available at design time but not the compiler. (?) That is, SqlEnumProvider seems to be working fine at design time (i.e., intellisense works) but failing at compile time. Not sure how to go about debugging an issue like this, anyone have any suggestions?

Note that this branch is currently compiling everything on net451, so this is not a netstandard/netcore issue.

@dmitry-a-morozov @vasily-kirichenko @smoothdeveloper

@samhanes
Copy link
Contributor Author

samhanes commented Oct 20, 2018

@samhanes
Copy link
Contributor Author

@samhanes samhanes force-pushed the netstandard branch 2 times, most recently from f3e055d to 7e8c1bf Compare October 25, 2018 18:28
@cmeeren
Copy link
Contributor

cmeeren commented Oct 26, 2018

So happy to see things are happening in this PR. Keep it up! 👍 🌞

@samhanes
Copy link
Contributor Author

Still moving forward, but very slowly. I've now separate runtime and design time assemblies which seem to be working as before with net40. And I've cross-targeted the runtime assembly to compile on both net40 and netstandard2.0. However, testing from a netcoreapp2.0 (I'm using Lib.fsproj to start with) referencing the netstandard version of the type provider is not going well.

The setup I've got now is:

  • DTC: net451 only - has to be full framework to reference Microsoft.SqlServer.TransactSql.ScriptDom and Microsoft.SqlServer.Types
  • RTC: cross-targeting net451 and netstandard2.0

My hope is that this will work via this workaround.

I'm then copying TP assemblies to the following folders (attempting to follow guidance here):

  • /net451 - contains runtime and design time dlls and referenced assemblies
  • /netstandard2.0 - contains netstandard runtime dll, plus the net451 version of the the DTC
  • /typeproviders/fsharp41/net451 - contains DTC
  • /typeproviders/fsharp41/netstandard2.0 - contains net451 version of the DTC

I've got an assembly reference from Lib.fsproj to the netstandard2.0/[runtime].dll. I'm getting runtime errors using any provided types:

Unexpected exception from provided type 'FSharp.Data.SqlCommandProvider,CommandText="SELECT 42",ConnectionStringOrName="name=AdventureWorks",SingleRow="True"' member 'GetMethods': The type provider 'FSharp.Data.SqlCommandProvider' reported an error: The design-time type 'System.Data.SqlClient.SqlConnection' utilized by a type provider was not found in the target reference assembly set '[tgt assembly FSharp.Data.SqlClient, Version=0.0.0.0, Culture=neutral;
 tgt assembly System.Runtime, Version=4.2.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a;
 tgt assembly FSharp.Core, Version=4.5.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a;
 ...]'. You may be referencing a profile which contains fewer types than those needed by the type provider you are using.

I should point out that both the RTC and the DTC are referencing System.Data.SqlClient, and I can see the reference in the assembly manifest of the RTC, so I am not sure what to make of this error.

I also get:

The type referenced through 'System.Data.SqlClient.SqlDataReader' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Data.SqlClient'.

I'd appreciate it if someone with some expertise could a) weigh in on whether my overall approach makes sense and b) shed some light on those errors.

@vasily-kirichenko
Copy link
Contributor

So you have reached the point where I gave up fighting this monster :) Good luck.

@samhanes
Copy link
Contributor Author

I was afraid of that...

@vasily-kirichenko
Copy link
Contributor

Be strong, I believe it's possible. Look at the approach used in SQLProvider.

@panesofglass
Copy link
Contributor

To confirm, a net40 project works correctly, but netcoreapp2.0 fails at runtime, correct?

@samhanes
Copy link
Contributor Author

@panesofglass A net40 project works correctly referencing the net40 version of the TP, but I haven't been able to get any test project to use the netstandard2.0 version of the TP - the project fails to compile. I had been using a netcoreapp2.0 test project, but switching it to net461 also fails. I'm going to explore that case a bit more, it might help illustrate what's going on.

@samhanes
Copy link
Contributor Author

Interesting: I was able to get the same test project to work (at design-time, anyway) when referencing the netstandard2.0 version of the TP by simply adding a reference to System.Data.SqlClient to the test project. So at the moment the netstandard TP seems to work for a net461 project, but not for a netcoreapp2.0 project.

@samhanes
Copy link
Contributor Author

Making progress! I was just able to get netcoreapp2.0 test projects to work at compile-time and runtime just by referencing System.Data.SqlClient and System.Configuration.ConfigurationManager in the test project. I'm a bit confused as to why the latter is required (since that component is referenced in the DTC and thus shouldn't be necessary at runtime) but I don't think it should be a major issue.

Still quite a bit to iron out, but this is starting to feel possible!

@samhanes
Copy link
Contributor Author

I've made some progress:

  • Everything seems to be working from a net461 project referencing the netstandard2.0 version of the TP and both design-time and runtime. (Hooray!)
  • I'm able to use erased types from a netcoreapp2.0 project (design-time and runtime ok)
  • A netcoreapp2.0 project using SqlEnumProvider (i.e., a generative type) works at design-time, but fails at compile time with:
FSC : error FS2024: Static linking may not be used on an assembly referencing mscorlib (e.g. a .NET Framework assembly) when
generating an assembly that references System.Runtime (e.g. a .NET Core or Portable assembly). [C:\code\FSharp.Data.SqlClient\src\SqlClient.Tests\SqlClient.Tests.NetCoreApp\SqlClient.Tests.NetCoreApp.fsproj]

Note that the RTC is entirely netstandard2.0, but the DTC has framework dependencies and thus targets net461.

Can anyone shed any light as to why erased types would be working from netcore but generative types are not?

@panesofglass
Copy link
Contributor

panesofglass commented Oct 30, 2018

cc @dsyme @ncave @cartermp @7sharp9

@panesofglass
Copy link
Contributor

panesofglass commented Oct 30, 2018

@samhanes were I to hazard a guess, I would suggest that the erased types don't depend on any of the dependencies but the generated types do; therefore, SqlEnumProvider will likely always fail until the DTC is able to depend on netstandard2.0. I opened #270 in the past about converting this to an erased type, assuming very few people actually cared about true, .NET Enum values. Maybe we could just change it to an erased type?

@samhanes
Copy link
Contributor Author

@panesofglass Digging a little deeper - SqlEnumProvider works fine with a kind of CLI or UnitsofMeasure - it's only the default "enum-behaving" types that are a problem. It's true that there are some allowed types (those provided by Microsoft.SqlServer.Types) that will not be available at runtime for netcore apps, but my failing test is only using primitive base types:

type TinyIntMappingDefault = SqlEnumProvider<"
    SELECT * FROM (VALUES(('One'), CAST(1 AS tinyint)), ('Two', CAST(2 AS tinyint))) AS T(Tag, Value)"
    , cnx, Kind = SqlEnumKind.Default>
// fails at compile-time

So I'm not sure what's happening. That said, it would be easy to revert to an erasing provider if that doesn't break anyone and solves the problem.

@cmeeren
Copy link
Contributor

cmeeren commented Oct 30, 2018

I can only answer for myself, but I'm fine with an erasing TP.

@samhanes
Copy link
Contributor Author

Progress! Both CI builds are now working, and I have changed the Travis script to compile the test projects against a hosted version of AdventureWorks. We can't run the unit tests on that build, since those require a writeable verison of the database.

@vasily-kirichenko I have not looked into your issue specifically, but could you try running the build script again? I've made a bunch of changes that should fix things on linux/macos.

@samhanes
Copy link
Contributor Author

Vasily, you may need to do something analogous to this:

- export FrameworkPathOverride=$(dirname $(which mono))/../lib/mono/4.5/

Definitely will need to document some of these tricks :)

@samhanes samhanes changed the title [WIP] netstandard 2.0 Target Netstandard 2.0 Nov 13, 2018
@samhanes
Copy link
Contributor Author

I think this is ready for review and more widespread testing.

There is obviously a lot happening in this PR - not sure how the maintainers want to handle this going forward?

@thinkbeforecoding
Copy link
Contributor

I' tested it at work and it's working smoothly !
Thank you soooo much !

@panesofglass
Copy link
Contributor

@samhanes does appveyor not produce a nupkg?

@smoothdeveloper
Copy link
Collaborator

@samhanes thanks a lot for your contribution!

I'm going to allocate some time to give it a try and will review more closely the work you have done in this PR.

@vasily-kirichenko
Copy link
Contributor

@samhanes have you tested it by creating a NuGet package and referencing it from a local folder as NuGet source?

@vasily-kirichenko
Copy link
Contributor

I still cannot build it on Mac with the same error.

@thinkbeforecoding
Copy link
Contributor

I managed to reference the nuget from a local folder, and you have to add the Import of fsc.props and netfx.props for the project to build.
Once done, it works on dotnetcore on both windows and linux.

@thinkbeforecoding
Copy link
Contributor

Didn't manage to use integrated security on linux, but I think it's another issue (setting kerberos etc...)
It's working nicely with user id / password on linux anyway.

@vasily-kirichenko
Copy link
Contributor

OK, FrameworkPathOverride works, but it fails on creating the NuGet package:

NuGet 00:00:03.4737455 (Error during NuGet package creation. packages/build/NuGet.CommandLine/tools/NuGet.exe pack -Version 1.8.6 -OutputDirectory "/Users/vaskir/git/FSharp.Data.SqlClient/nuget" "/Users/vaskir/git/FSharp.Data.SqlClient/NuGet/SqlClient.1.8.6.nuspec"
Cannot open assembly 'packages/build/NuGet.CommandLine/tools/NuGet.exe': No such file or directory.)

@samhanes
Copy link
Contributor Author

@panesofglass The appveyor build does not create a nupkg, I assume publishing is a manual process.

@thinkbeforecoding Good callout on the integrated security issue on linux, makes sense that it doesn't work yet. I think I'll leave that alone for now.

@vasily-kirichenko I think I need to update the Fake script to use dotnet pack rather than relying on nuget.exe in order to make the NuGet step work cross platform. I can take a stab at that this evening.

I have (now) tested against a local nupkg - appears to be working as expected.

@vasily-kirichenko
Copy link
Contributor

@samhanes thanks. Have you tested it on a .NET 4.6+ project as well?

@vasily-kirichenko
Copy link
Contributor

...and in Rider/VS/Code?

@thinkbeforecoding
Copy link
Contributor

thinkbeforecoding commented Nov 13, 2018

A test for a project net45/net461/net472/netcoreapp2.1 is working fine here.

@thinkbeforecoding
Copy link
Contributor

VSCode editing is working good and fast !
Same think in VS 2017.
Not tested with Rider.

@samhanes
Copy link
Contributor Author

I have tested net40/net461/net471/netcoreapp2.0 projects (net40 still using the net40 version of the TP of course) on VS and VS Code, but nothing on Rider yet.

@thinkbeforecoding
Copy link
Contributor

I did a quick review.
There is a lot of changes, but it's mostly changing projects to SDK, moving code to new assembly and adapting tests. So seems good to me.

@smoothdeveloper
Copy link
Collaborator

@samhanes I've pulled the branch and checking it out, all the project refactoring (splits of files etc.) are good work on top of the new netstandard2.0 target.

I've noticed *.dll.xml & *.dll.html files such as SqlClient.SqlServerTypes.Tests.dll.xml, could you check if we need those to be checked in?

@samhanes
Copy link
Contributor Author

@smoothdeveloper I don't think we need those checked in - were they ignored before? Easy enough to fix if that's how we want it.

@vasily-kirichenko Just starting to look at the building the nupkg xplat - it appears that moving from nuget pack w/ a nuspec file to using dotnet pack is not as straightforward as I would have hoped. Unless we have a specific need to pack from MacOS/Linux, I think we can leave it as is.

@vasily-kirichenko vasily-kirichenko merged commit dabf2cb into fsprojects:master Nov 18, 2018
@vasily-kirichenko
Copy link
Contributor

Thanks!

@cmeeren
Copy link
Contributor

cmeeren commented Nov 18, 2018

Fantastic work, @samhanes! 🎉 👏

@vasily-kirichenko
Copy link
Contributor

Published as version 2.0.1

https://twitter.com/kot_2010/status/1064090662042185728

@samhanes
Copy link
Contributor Author

Whew! Great news!

@samhanes samhanes deleted the netstandard branch November 20, 2018 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants