search

fsprojects / FSharp.TypeProviders.SDK

The SDK for creating F# type providers
https://fsprojects.github.io/FSharp.TypeProviders.SDK/
MIT License
298 stars 94 forks source link

[okay to merge now] - move to .NET Standard exclusively #339

Closed cartermp closed 3 years ago

cartermp commented 4 years ago

This simplifies the repo and how Type Providers build moving forward. The downside is that older .NET Framework and older F# consumers won't be able to use newer type providers build with the SDK in this state. That is also largely true today, though, as many type providers are updated and do not support these older environments.

dsyme commented 4 years ago

This is great. We should probably create a branch marking the point just before we intergrate this in case people need to get the TPSDK for .NET Framework

cartermp commented 4 years ago

https://github.com/fsharp/FAKE/issues/2530 prevents updating fake-cli for now until we figure out a different way to automate packing the template

cartermp commented 4 years ago

Unfortunately it appears that doing any kind of packing with FAKE is busted, so I'm going to comment that out and move on. It can always just be done manually

cartermp commented 4 years ago

someone was being very naughty and allowed zero tests to run in CI for a long time

cartermp commented 4 years ago

Templates are turning into a bit of a nightmare. Something is wrong in the environment where it is tested; the wrong paket.lock file is used and even forcing an install causes it to still not have the same graph as what the template specifies. Feels kinda janked up

cartermp commented 4 years ago

Hmmm. I confirmed that in teh test for the template, it definitely just pulls whatever the latest was on nuget. definitely wrong behavior. Was this ever doing the right thing?

dsyme commented 4 years ago

Hmmm. I confirmed that in teh test for the template, it definitely just pulls whatever the latest was on nuget. definitely wrong behavior. Was this ever doing the right thing?

sorry about this

Maybe only when a new version number is bumped and nothing yet pulled to nuget?

cartermp commented 4 years ago

I managed to figure it out :)

The main issue now is that for some reason, erasing type providers just don't seem to work. Generative ones do though

schauerte commented 3 years ago

May I kindly ask, what the state of this MR is? The erasing type provider seems to work for me (using Ubuntu w/o Mono).

I'm thinking of using this as base for "fixing" the problems, people (like me) seem to have with the existing SQLProvider (when using dotnetcore).

cartermp commented 3 years ago

Aside from merge conflicts, the current state is that the erasing providers in the samples and template won't have their provided types recognized by the consuming test projects. The build "works" because of things like this: https://github.com/fsprojects/FSharp.TypeProviders.SDK/pull/339/files#diff-cc8d677b59f95e6653717fb44bf170ed2a9ba92e75f134636b59bc11ce44da67

I decided to comment them out just to continue to work on everything else make sure it worked.

dsyme commented 3 years ago

@cartermp I need to take a careful look at this again

Type providers must continue to run in devenv.exe, which uses .NET Framework, and also the desktop fsc.exe and fsi.exe. So realistically we have to continue to test on .NET Framework at least.

We don't have to build on .NET Framework.

We do have to consider what happens when TPs are referenced in .NET Framework projects

dsyme commented 3 years ago

I tried to review this to see what's causing the test failures, but the changes are too large - the 3000 lines of (largely formatting?) change in ProvidedTypes.fs in particular

@cartermp what's the right way to progress this ? Should we chop this into smaller pieces, or revert some of the formatting changes to minimise the diff, or start again doing the removals bit by bit?

dsyme commented 3 years ago

@cartermp what's the right way to progress this ? Should we chop this into smaller pieces, or revert some of the formatting changes to minimise the diff, or start again doing the removals bit by bit?

I think we should minimise our the diff in ProvidedTypes.fs{i} and get this in. I'll do that now

cartermp commented 3 years ago

Much of the diff is necessary as far as I recall. Some things had to get moved around to enable things to still build. If you use this setting you'll see it's a small diff:

image

dsyme commented 3 years ago

Much of the diff is necessary as far as I recall. Some things had to get moved around to enable things to still build. If you use this setting you'll see it's a small diff:

Looking at that diff I think there are no actual substantive changes in ProvidedTypes.fs/fsi. Which is good as it means the cause of the failing test isn't there.

I don't think any of the changes in those files would have been necessary for what you're trying to achieve but it's decent cleanup so can stay

cartermp commented 3 years ago

I recall them now, it is due to repo simplification.

There was some #if def magic in the tests to get a reference to AssemblyReady that were better resolved by an internal API. I moved testing to just take a project reference here.

dsyme commented 3 years ago

Move TPDSDK to a proper project that can be published to NuGet

The TPSDK is currently done via source inclusion because different type providers internalise their own appropriate copy of ProvidedTypes.fs/fsi in order to avoid version conflict nightmares when multiple type providers try to load incompatible versions of FSharp.TypeProvider.SDK.dll.

Now, each type provider can in theory ship its own private copy of FSharp.TypeProvider.SDK.dll in typeproviders/fsharp41/... alongside the TPDTC and hope each of them get loaded separately like other dependencies. That may work. But I'd be careful about forcing that on consumers.

So, for example, with this change we will expect to see many different copies of FSharp.TypeProvider.SDK.dll loaded into devenv.exe, one for each loaded type provider TPDTC, and we expect type provider authors to always place FSharp.TypeProvider.SDK.dll into typeproviders/fsharp41/....

That's ok, we can try, it is better. Howere we should at least continue to ensure that

  1. the TPSDK itself has no dependencies
  2. that source-including ProvidedTypes.fs/fsi continues to be a viable option for internalising the dependency.
dsyme commented 3 years ago

So about AfterBuild... you are correct the tests pass without it. This is what's going on:

It'll do for now but I'm dubious:

  1. I think this TPRTC to TPDTC project reference is a suspect technique - the Runtime and DesignTime framework targets could be substantially different. For example FSharp.Data has historically had TPRTC for, say, net45, and TPDTC for netstandard2.0 - it's just that in the current convergence these microexamples just happen to both be netstandard2.0.

    IMHO these projects should not cross-reference, and I suspect making them do so is a false simplification due to temporary converge between reasonable runtime and design time assumptions, and we might just have to undo it again at a later point (e.g. when we have TPRTC for netstandard2.0 and TPDTC for net5). Many practical type providers have to vary those assumptions by taking dependencies on either other runtime or design time components.

    So yes, we can get away with it right now, but these components have distinct dependency chains.

  2. The way the files are copied into the TPRTC bin is mixing up the TPDTC and TPRTC. They should always ideally be in separate folders, the TPDTC is meant to go in a "typeproviders" folder, even when doing testing. It doesn't matter in practice but it's a bit weird. I guess I had hoped we could eventually get rid of this behaviour where TPDTC are found alongside TPRTC but this is baking it in.

BTW I'm still not understanding these settings:

    <FSharpToolsDirectory>typeproviders</FSharpToolsDirectory>
    <PackagePath>typeproviders</PackagePath>

I guess they're both to do with nuget packaging.

dsyme commented 3 years ago

This should now basically be ready (assuming it goes green). My one concern is the deletion of a chunk of important testing, I'll look into that now

The downside is that older .NET Framework and older F# consumers won't be able to use newer type providers build with the SDK in this state.

This should be clarified precisely in the README. For example

I don't really mind if the answers are "no" (though we should really know the answer for VS for Mac and Mono). But we should give some clarity since there are actual users with the above constraints who can't update to latest F# tooling.

Historically we've also often had enterprise users using back versions of Visual Studio. I so much wish there were ways of preventing even the referencing of a design time component based on certain constraints, but there isn't AFAIK.

I don't expect any problem from Ionide since it's updated so regularly.

dsyme commented 3 years ago

Just to mention VS for Mac probably loads FSharp.Core 4.7.0.0 (package 4.7), see https://github.com/mono/monodevelop/blob/master/main/external/fsharpbinding/paket.lock#L14. It will load .NET Standard 2.0 components.

Mono tooling (fsc.exe etc.) looks like it will be using FSHarp.Core 4.5.0.0, see the commits in https://github.com/fsharp/fsharp. It will load .NET Standard 2.0 components.

So an FSharp.Core dependency of 4.5.2 makes sense to me after all. There's no reason TPDTC built with that dependency won't load, even if we're not actively testing those configurations.

cartermp commented 3 years ago

Mono tooling (fsc.exe etc.) looks like it will be using FSHarp.Core 4.5.0.0

This will no longer be the case with https://github.com/mono/mono/pull/20511 - both mono and VSMac will have the latest F# by VSMac 8.9 release, or a patch of 8.8

dsyme commented 3 years ago

This will no longer be the case with mono/mono#20511 - both mono and VSMac will have the latest F# by VSMac 8.9 release, or a patch of 8.8

That is fantastic - is the VS for Mac PR public?

dsyme commented 3 years ago

testing in BasicProvider

This is done, for BasicProvider, the relevant fix is here: https://github.com/cartermp/FSharp.TypeProviders.SDK/pull/1/files#diff-c540bf1e311c246e1a3b5149f3a17c045b769a48ae9b07e699fb98d62b4298a0R11

I'll look into the template now, it's possible those tests actually pass

dsyme commented 3 years ago

OK the tests we want are all re-enabled and everything is green.

I think we can merge this. Next steps would be to

  1. push the pacakges
  2. update the README to be as simple as possible
  3. manually test the template

It's great to see how simple dependency management for TPDTC has become.

dsyme commented 3 years ago

I checked the log and we really do seem to be instantiating the template and running tests (though output from running the tests is suppressed in the log it seems)

  Determining projects to restore...
  Restored D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\src\tp250.DesignTime\tp250.DesignTime.fsproj (in 339 ms).
  Restored D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\src\tp250.Runtime\tp250.Runtime.fsproj (in 15 ms).
  Restored D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\tests\tp250.Tests\tp250.Tests.fsproj (in 1.99 sec).
  tp250.DesignTime -> D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\src\tp250.DesignTime\bin\Debug\netstandard2.0\tp250.DesignTime.dll
  tp250.Runtime -> D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\src\tp250.Runtime\bin\Debug\netstandard2.0\tp250.Runtime.dll
  tp250.Tests -> D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250\tests\tp250.Tests\bin\Debug\netcoreapp3.1\tp250.Tests.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:24.78
D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp\tp250> "C:\Users\runneradmin\AppData\Local\Microsoft\dotnet\dotnet.EXE"  test -c debug (In: false, Out: false, Err: false)
D:\a\FSharp.TypeProviders.SDK\FSharp.TypeProviders.SDK\temp> "C:\Users\runneradmin\AppData\Local\Microsoft\dotnet\dotnet.EXE"  new -u FSharp.TypeProviders.Templates (In: false, Out: false, Err: false)
Finished (TagStatus.Success) 'TestTemplatesNuGet' in 00:00:44.7358328
Starting target 'All'
Finished (TagStatus.Success) 'All' in 00:00:00.0001154
dsyme commented 3 years ago

@cartermp I will merge this now. Thanks you so much for these simplifications, it's miraculous. Also thanks to @KevinRansom for getting IsFSharpDesignTimeProvider and friends into the SDK support, it makes a huge difference

dsyme commented 3 years ago

I ❤️ merging PRs that say "DO NOT MERGE"

cartermp commented 3 years ago

Lemme retro-actively adjust the title

dsyme commented 3 years ago

Too late, it's in the git log for all eternity

Author: Phillip Carter <pcarter@fastmail.com>
Date:   Mon Oct 26 17:53:58 2020 -0700

    [Do not merge] - move to .NET Standard exclusively (#339)
isaacabraham commented 3 years ago

That's how we roll in F# land.

schauerte commented 3 years ago

@dsyme

I checked the log and we really do seem to be instantiating the template and running tests (though output from running the tests is suppressed in the log it seems)

I think, this assumption may not be correct. Please take a look at #347.

dsyme commented 3 years ago

@schauerte Thank you so much for double checking. That silent test failure is very disturbing!