-
Notifications
You must be signed in to change notification settings - Fork 58
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
WIP: New Cross-Targeting Type Provider SDK #88
Conversation
I'm looking into these failures - thanks. Have found a few fixes. Swagger is a great stessssss case,,,, |
I'm hoping to pull down your latest round of changes after work today and see how much further I can get! Heh, and @sergey-tihon is already on top of that |
@baronfel I did push an update to latest SDK into this PR just a minute before @dsyme comment. now we have different error
Update:
|
@sergey-tihon Yes. I'll take a look tomorrow. Don't bother trying anything for a bit |
The error with the latest SDK is
it looks like Fsharp.Core version mismatch somewhere... We compile using |
@sergey-tihon @baronfel I'm working on this, I'll push updates to this PR as I go |
thank you @dsyme, it looks like we are one step closer but not done yet. |
and we cannot compile test projects on
|
@sergey-tihon I pushed another update, things feel one step closer. However I'm concerned that there might have been a significant performance regression, where compiling a generative assembly is now slow. I'm noticing in particular that the APIs.Guru tests seem slow - 2-10seconds/test. Have they always been that slow? However that applies to If there has been a further slowdown I'd like to know - it's not totally surprising if there has been - there are a number of places where code gen may be inefficient in the new implementation and Swagger might well reveal new places where we need to be more careful. The latest commit has one use of UncheckedQuotations |
Travis failure:
I'm not seeing that behaviour on Windows |
One error so far on AppVeyor for test case https://api.apis.guru/v2/specs/azure.com/arm-iothub/2016-02-03/swagger.json
|
…eems that it works now.
Yes, it feels slower, but it never was fast. We did the comparison before - fsharp/fsharp-compiler-docs#744 (comment) I tool here are the results
and here are the results from the last run on Win (38min)
We have more schemas from APIs.guru now, but |
For history, or if we decide to verify the slowest compilation samples (slower than 30sec) StarterPack
New SDK
The winner is https://api.apis.guru/v2/specs/stackexchange.com/2.0/swagger.json =) P.S. The is no guarantee that we could heavily rely on numbers from AppVeyour logs |
One of your modules expects the type 'T' to be defined within the module being emittedI can reproduce it locally
I am not sure, but it may be similar to fsprojects/FSharp.TypeProviders.SDK#150 (base type lookups) We generate TP // Create Swagger provider type
let baseTy = Some typeof<SwaggerProvider.Internal.ProvidedSwaggerBaseType>
let ty = ProvidedTypeDefinition(tempAsm, NameSpace, typeName, baseTy, isErased = false, hideObjectMethods = true) and type Arithmetic operation resulted in an overflow.Also on
|
@sergey-tihon I'm trying to repro the few remaining failures on AppVeyor/Windows first. In the log I see this:
All with
However when I try to repro this using a source file
and a command-line compile
it doesn't repro.... |
There are also some new errors appearing in CI for that list of APIs today,. e.g https://api.apis.guru/v2/specs/deutschebahn.com/stada/2.1.02/swagger.json gives |
@sergey-tihon Just to mention fsprojects/FSharp.TypeProviders.SDK#152 is a blocking issue before this should be integrated Basically we should add testing to swagger that |
This one is fixed already, I did rerun AppVeyor build. Only 5 schemas fails now with |
This is interesting, it fails on my machine when I compile using FCS, but it works when I do it manually from just a note, when we call fsc from the test, we generate a slightly different list of referenced assemblies
Diff1 we use a later version of Diff2 SwaggerTP does not have reference to |
One error on windows on last run - I don't think it is related to the PR though - looks like a network problem
|
Close/reopen to re-run CI |
Still getting this on Linux:
|
Yes, this is not related.
Any idea what it could be? |
@sergey-tihon There must be some difference in the implementation of reflection for Mono and .NET. I will need to debug it on a Linux VM I think. That might take a while before I get to that I'm afraid |
I can at least now repro this using Windows Subsystem for Linux :) So no need to switch to a Linux VM |
ok, now we're green everywhere :) Thanks for the hard work, @dsyme! |
Oh that's great |
Is this PR good as it stands, or should we go ahead and trial the proposed type-provider packaging conventions being discussed in dotnet/fsharp#3736? That issue seems to be in flux still. |
@baronfel You should treat this as separate. This PR has value in and of itself, as it means that code will be correctly generated for the target .NET Framework, e.g. if targeting .NET 4.6.1, then code will be generated that references the correct .NET 4,6,1 assemblies, regardless of the .NET version being used by the host compiler. |
I’ll merge it immediately when I back from vacation and will release new Nuget package based on new SDK. Net core support will be in separate pr. Thx. |
Merged and released as v0.9.0 |
This so far does most of the grunt work for using the new context-required Type Provider SDK.
It's not functional yet, I was hoping to get some pointers though on what I may be doing wrong.
The biggest change is that the definition and operation compilers needed the context passed so that they could create the new context-aware members. This has the unfortunate side-effect of requiring a reference to the ProvidedTypesTesting module, which is sadly internal. I'm partly opening this to point Don at this and ask if that can be opened up some.
This will compile, but the Tests and Docs solution fails for the following reason:
But I don't see easily what I'm doing wrong.