search

fsprojects / Rezoom.SQL

Statically typechecks a common SQL dialect and translates it to various RDBMS backends
MIT License
670 stars 25 forks source link

using Paket and slim fsproj for main solution #43

Open smoothdeveloper opened 5 years ago

smoothdeveloper commented 5 years ago

I've left the samples as is (and runned them after my changes) so it doesn't impact end user documentation / tutorials, it looks to work fine AFAICS and will help for multitargetting later on.

rkosafo commented 5 years ago

@smoothdeveloper Thanks for the contribution. Can you do this against the standard branch instead? That is where recent develop effort is.

smoothdeveloper commented 5 years ago

@rkosafo no problem, I'm rebasing my work on that branch.

I'm noticing few things in the fsproj in that branch, one uses netcore instead of netstandard, using netcore seems wrong to me.

For the targets, I think it makes life easier for consumers if we also ship net45 binaries as it used to be available in previous releases, so I'm adding both targets to the fsproj, that doesn't add any clutter to the project files but requires a bit more work when paket solves the dependencies for all platforms.

Please let me know if there are pending changes to proj files as it might interfere with my fixes & testing.

smoothdeveloper commented 5 years ago

Ok, so figuring out a bit more about changes in the standard branch, here is what I recommend:

For the last item, there is no need to copy the project folders / source files (like https://github.com/rspeele/Rezoom.SQL/tree/standard/src/Rezoom.SQL0.Compiler) or duplicate the project files.

I've contributed to similar effort in FSharp.Data.SqlClient and I think following the first steps (first, migrate to new SDK, then add targets) is the safest and best bet to eventually target netstandard20 along net framework that should still be targetted.

rkosafo commented 5 years ago

Please let me know if there are pending changes to proj files as it might interfere with my fixes & testing.

There are no changes to proj files. Only one fix to address a TP issue.

rkosafo commented 5 years ago

Ok, so figuring out a bit more about changes in the standard branch, here is what I recommend:

  • keeping this PR on master, and getting it merged, it brings new SDK but doesn't attempt to target netstandard20 at the same time
  • once reviewed / tested, we can make one official release to get more feedback / make sure nothing breaks for the clients
  • in meantime, make work on extra netstandard target off master, which shouldn't be too hard, assuming the dependencies already target it

For the last item, there is no need to copy the project folders / source files (like https://github.com/rspeele/Rezoom.SQL/tree/standard/src/Rezoom.SQL0.Compiler) or duplicate the project files.

I've contributed to similar effort in FSharp.Data.SqlClient and I think following the first steps (first, migrate to new SDK, then add targets) is the safest and best bet to eventually target netstandard20 along net framework that should still be targetted.

Noted. However, 0.7.x works correctly on 4.5+ and the code in the standard branch work as expected. NetStandard20 support is kinda a big priority now. If an official one is released, what version will it take as v1.x.x is expected to be at least .net standard compliant?

I think we should work off standard, add full .net support, test and release. Later, we can migrate to the new TP starter files. Tried it with a lot of wonderful error messages so reverted to the current ones.

For the duplicate projects, I think it was because @rspeele may be started it off as an experiment. The extra files will be removed later.

Does this work for you?

smoothdeveloper commented 5 years ago

@rkosafo thanks for the feedback, I've pushed few more changes, and this PR is now ready, to get a fresh build out of a checkout:

cd src
dotnet restore
dotnet build

Then, I'm able to run 

cd TypeProviderUsers
dotnet restore

but then dotnet build fails, opening the solution in VS, and building works, and I'm able to run the SQLite tests, not sure about setup of PGsql ones but I'll try to run them locally as well.

This PR does bring all the code which is not under demos to the new sdk, which is first step needed for completing work on netstandard. It also integrates paket, as it makes it much easier to manage dependencies across the board.

To support netstandard, the first issue I have is in Rezoom.SQL.Compiler project, the types related to configuration (System.Configuration assembly) don't seem to exist, but I need to look at it more and look more at the commits under the standard branch.

smoothdeveloper commented 5 years ago

Later, we can migrate to the new TP starter files.

AFAIU this is not possible, to support the netstandard target, the TP has to be using the more recent TP SDK, and most probably the provider assembly has to be split into two like it was necessary on FSharp.Data.SqlClient.

rkosafo commented 5 years ago

@rkosafo thanks for the feedback, I've pushed few more changes, and this PR is now ready, to get a fresh build out of a checkout:

cd src
dotnet restore
dotnet build

Then, I'm able to run

cd TypeProviderUsers
dotnet restore

but then dotnet build fails, opening the solution in VS, and building works, and I'm able to run the SQLite tests, not sure about setup of PGsql ones but I'll try to run them locally as well.

Noted. Will pull the code and check it out.

This PR does bring all the code which is not under demos to the new sdk, which is first step needed for completing work on netstandard. It also integrates paket, as it makes it much easier to manage dependencies across the board.

To support netstandard, the first issue I have is in Rezoom.SQL.Compiler project, the types related to configuration (System.Configuration assembly) don't seem to exist, but I need to look at it more and look more at the commits under the standard branch.

netstandard is already supported in the standard branch. 4.5 is however on the master. Do pull the code on standard and check it out. I'll checkout the paket integration as well.

rkosafo commented 5 years ago

Later, we can migrate to the new TP starter files.

AFAIU this is not possible, to support the netstandard target, the TP has to be using the more recent TP SDK, and most probably the provider assembly has to be split into two like it was necessary on FSharp.Data.SqlClient.

Does that mean we cannot have the same project target .net fx and standard at the same time? I'll check out FSharp.Data.SqlClient as pointed out.

smoothdeveloper commented 5 years ago

netstandard is already supported in the standard branch. 4.5 is however on the master. Do pull the code on standard and check it out. I'll checkout the paket integration as well.

Yes I'll spend more time reviewing that branch, and we don't have to merge #43 if you guys feel the better way is to start from standard branch.

It's just that I generally prefer to do new SDK + paket migration first, then adding new targets, I'm not yet comfortable with making TP netstandard + I don't know this codebase.

Does that mean we cannot have the same project target .net fx and standard at the same time? I'll check out FSharp.Data.SqlClient as pointed out.

So, a single fsproj can target multiple frameworks, using <TargetFrameworks>target1;target2;...</TargetFrameworks> instead of <TargetFramework>target1</TargetFramework> so this will work fine.

Normally, for the non TP projects, in this PR, adding netstandard20 to the target framework list is supposed to be what's needed, beside the code changes.

For the TP project, it is most probably going to be required to have a separate Rezoom.SQL.Provider.DesignTime assembly, for reasons related to the tooling IIRC; the details: https://github.com/fsprojects/FSharp.TypeProviders.SDK#making-a-net-standard-20-tpdtc

If you prefer working on standard branch and add net45 in the projects, I'll make a separate PR when it gets merged to master to bring paket if that's still something you want.

I'll probably spend more time looking at the whole codebase (which looks great!) and shouldn't have more to push in this branch for now.

smoothdeveloper commented 5 years ago

@rkosafo / @rspeele, wondering if you could give a look at using this branch and tell me if you have any issue with the restore / build?

It's difficult for people to review the standard branch for now due to the duplicated folders making big diff online, and it seems that the branch has more stuff than what's needed to get TP SDK upgrade and adding netstandard target.

Maybe we can consider again getting this PR merged and work on a new nestandard branch by porting the required code changes in a clean branch.

In this PR, there are 0 code changes, and only projects / solution adjustments to use new .NET sdk and paket, it should easily merge with work on the code.

@rspeele, are you fine with usage of paket? I see you are not using it in your other repositories, so if you'd prefer to have a "just new .NET sdk" PR instead of along with paket, I can give it a look.

rspeele commented 5 years ago

I'm not strongly opinionated about using Paket vs using NuGet. At this point I am not doing development though, it's all @rkosafo and yourself.