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

using Paket and slim fsproj for main solution #43

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

smoothdeveloper
Copy link
Collaborator

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
Copy link
Contributor

rkosafo commented Mar 29, 2019

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

@smoothdeveloper
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

@rkosafo
Copy link
Contributor

rkosafo commented Mar 30, 2019

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
Copy link
Contributor

rkosafo commented Mar 30, 2019

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

@smoothdeveloper smoothdeveloper force-pushed the paket-and-new-dotnetsdk-for-main-solution branch from badeb20 to b996371 Compare March 31, 2019 09:21
@rkosafo
Copy link
Contributor

rkosafo commented Mar 31, 2019

@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
Copy link
Contributor

rkosafo commented Mar 31, 2019

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
Copy link
Collaborator Author

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.

* bare convert-from-nuget
* fix up demos, keeping them as plain nuget, for sample / end-user / doc purpose
* simplifying paket.dependencies to remove demo stuff
* add solution items to .sln
* TypeProviderUsers project touched by paket install (looking further if we can move those projects to new SDK)
…ket we are interested in netstandard as well
* define IsTestProject in test projects for "dotnet test" (need some more config to work)
* use nuget references for non rezoom assemblies in type provider user projects
@smoothdeveloper smoothdeveloper force-pushed the paket-and-new-dotnetsdk-for-main-solution branch from b996371 to 9a6ce64 Compare April 7, 2019 08:08
@smoothdeveloper
Copy link
Collaborator Author

@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
Copy link
Collaborator

rspeele commented Apr 13, 2019

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.

Copy link

@Swoorup Swoorup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by me, most of these changes look generated by the paket converter tool.

<ProjectReference Include="..\Rezoom.SQL.Mapping\Rezoom.SQL.Mapping.fsproj">
<Name>Rezoom.SQL.Mapping</Name>
<Project>{6b6a06c5-157a-4fe3-8b4c-2a1ae6a15333}</Project>
<Private>True</Private>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it shouldn't be omitted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swoorup just above: <ProjectReference Include="..\Rezoom.SQL.Mapping\Rezoom.SQL.Mapping.fsproj" /> I don't see it omitted, am I missing something?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the <Private>True</Private> does have some influence on the packaging it seems.

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.

4 participants