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

Support portable libraries #5

Merged
merged 30 commits into from
Jan 28, 2013
Merged

Support portable libraries #5

merged 30 commits into from
Jan 28, 2013

Conversation

ovatsus
Copy link

@ovatsus ovatsus commented Dec 16, 2012

Don't merge this yet, it's still work in progress, I just want to discuss the approach before proceeding with the rest

I've taken a different approach compared to FSharpx and the Freebase type provider

In FSharpx we use FAKE scripts to reuse the same .fsproj for both the portable and normal build, but that hides errors until we run FAKE (which I usually forget). I've used 2 .fsprojs instead, it's a little more work because when we add a file we must add it to the two project files, but it's easier to make sure everything is working without having to leave Visual Studio

In the Freebase type provider, instead of using quotations and splicing like in all the other samples, the expressions are constructed manually because we need to reference the runtime assembly instead of the current executing assembly (the design time assembly). This makes the code much uglier because of all the reflection stuff. Instead of that, I've created a set of helper methods to traverse the quotations and do the necessary patching. It's still a little flaky, but at least for the simple expressions used in the csv provider seems to be working ok.

@tpetricek let me know what you think of this approach (ovatsus@29de451#L4R51) Are you ok with it? Do you think it might cause performance problems when compiling?

@ghost
Copy link

ghost commented Dec 16, 2012

Great to see the cross-targeting being addressed!

Here's some guidance based on many months experience doing cross-targeting (feel free to ignore it :) )

  • Keith Battocchi and I made several attempts to use quotation literals in cross-targeting type providers. In the end the guidance we formulated was "don't do it, don't even try". Now, it may be you get it to work and correctly apply fixups for some limited cases, and it may be our job was complicated by hosting the providers in Try F# as well. However, our general recommendation is to "keep it simple" and avoid the use of quotation literals in cross-targeting providers. This is painful, but once you let go it is not too bad. I've just come to accept it.
  • For Portable components and cross-targeting, be sure to test on all of NET 4.x, Mono 4.x, Win8 apps and Silverlight 5. (For .NET 4.x, the portable FSharp.Core is unused and instead redirects to the .NET 4.x FSharp.Core. On Win8 apps and Silverlight 5 the portable FSharp.Core is used. This is one reason you should test on all three).
  • It's totally up to you (and FSharpx) about the .fsproj file. However you may also want cross-compilations to .NET 3.5, Silverlight 5, Mono 2.1, Win8 and Win8Phone. That begins to be way too many project files. Hence we kept one project file.

Keith and I wrote up some full guidelines, we'll dig them out and post them.

@ovatsus
Copy link
Author

ovatsus commented Dec 16, 2012

  • My first thought was that if you didn't use quotations on Freebase was because it probably didn't work. But I figured the CSV, XML, and JSON providers are much simpler, and I didn't want to have to change @tpetricek code too much, so I'm going to try to stretch this a little further (although, it would be nice if you changed something on the language to make that more generally doable, as it is now it's a little of a disincentive to do cross-compiling type providers, it's much more work). I have to do two translations (from runtime to designtime, and then from designtime to runtime again), and I have to take care with Vars, as they aren't used by name but by ref, that's why I had to keep a table with them around to be able to get to the original ones after the two translations
  • I'm still going to test this better, I'm interested in getting the json provider working on a WP7.1 app (can't test on WP8, my CPU isn't supported by the emulator) On C# projects, we can select the portable profile to use, on F# the only choice is Profile47, and according to http://nitoprograms.blogspot.co.uk/2012/05/framework-profiles-in-net.html it supports (.NET for Windows Store apps, .NET Framework 4.5, Silverlight 5), but the portable package of FSharpx says (.NET for Windows Store apps, .NET Framework 4.0, Silverlight 4, Windows Phone 7.1), so I'm not sure if it will work or not
  • I chose to have two fsproj's, because I only wanted to support full .NET 4.x and the Portable profile. If the Portable build works well on everything, there's no need to compile to all the targets separately. I don't know if I can get all the network code to work well on Portable though, for now it's commented out. And I wanted to avoid to need to have to have Fake or a batchfile to compile all the targets, I wanted this to work simply by building the solution, to keep with the simplicity that Thomas was able to get in this package

It would be interesting to see the rest of your guidelines. There's really very little information around on how to do type providers. And if you could release any eventual tools that you might have used to debug the freebase type provider at ms it would be great. To check if it's all working, I have go through all methods and properties to make sure they don't complaining about a missingtype in the runtime dll

@tpetricek
Copy link
Member

Quotation literals

I suppose the JSON + XML + WorldBank type providers are quite simple and use uniform style, so it would probably be possible to fixup the quotation literals and make this work. On the other hand, the code that is generally included in quotation literals is not too complicated and it might be possible to get rid of it without too much pain (the code already cannot use it when using some fancy generics). I'll experiment and see... perhaps we could get something like this to work:

<@@ JsonOperations.GetFloat(%%json) @@>

becomes:

types.JsonOperations?GetFloat(json)

Of course, this is just an extreme stretch of ? instead of extreme stretch of quotation literals :-) but perhaps it would work without causing too much pain.

Testing & targets

The ideal option would be to have some automated process for testing this. Any ideas how this could be done? Otherwise, I think we need to choose a reasonable set of things to support. After reading this discussion twice, I still do not understand what the cases are :-). @dsyme can you give us some authoritative matrix with what options are used on what systems?

Ideally, we would support at least the latest Windows Phone & Windows 8 Tablets + Mono and .NET, but we might actually be able to get better mobile story with FunScript (I'm making some progress there). Silverlight would be nice for TryF#, but https://fsnotebook.net might be an alternative in not too distant future...

Multiple fsproj files

I would actually keep multiple fsproj files. I quite like using folders, so they have to be edited by hand anyway. We could also define files to compile in a single .proj file and just import it in other projects (would that work?)

One more reason is that I think having separate projects might be a good way to avoid #if which looks like a maintenance nightmare to me. I would much prefer having e.g. Network.Portable.fs file that implements some compatibility layer used by Network.fs (or have just Network.Full.fs that is not included in portable package). It is still ugly, but it feels less bad than having #ifs randomly scattered all over the place.

@ovatsus
Copy link
Author

ovatsus commented Dec 17, 2012

Quotation literals

I also tried using ? but I couldn't get it to work correctly. If you can get it to work it would be great to have several options

Testing & targets

The ideal would be to have unit tests running on the different runtimes, but that's probably not as easy as it sounds. For starting, I think the best would be to create sample projects in Windows Store, Windows Phone & Silverlight that reference the portable version, get them in the tests solution, and make sure they keep working after we make changes to the type providers. I've started doing this, I'll add it to this pull request later this week.

As for mono, I would expect the normal version to work as is, but we do have to test it. If someone can get a nicely packaged linux vm already with mono 3 and fsharp 3 already installed that would be great :)

One other thing I'd like to do is to downgrade the main version from .NET 4.5 to .NET 4.0. There's only one method call specific to 4.5 (ReadToEndAsync), and it can be replaced without much effort. 4.5 doesn't run on a bunch of OS versions (like XP, Server 2008 R1, etc), and a lot of people can have it on their dev machines but can't add a dependency to it in their applications

@tpetricek
Copy link
Member

I found some more time to take a look at this. I think it would be good to follow Don's advice and avoid using quotations (and I wanted to look into the ? trick...) but I first wanted to check what is the status of the work you did. I merged the version in this pull request with the latest version and tried running it, but I have some questions:

  • Did you have some tests for the portable version? I do not see any in the branch and when I tried to build FSharp.Data.Portable it failed (because of System.Web.HttpUtility.JavaScriptStringEncode), so I added a basic portable test project (see FSharp.Data.Tests.Portable) and I did some ugly workarounds for build errors (I would like to avoid using #if as much as possible in the final version, but let's just get it to work first...).

  • When I try to compile the F# portable library project (FSharp.Data.Tests.Portable) I get the following error message:

    error FS3033: The type provider 'ProviderImplementation.CsvProvider' reported an error 
    in the context of provided type 'FSharp.Data.CsvProvider,Sample="../../samples/docs/MSFT.csv"+DomainTypes+Row', 
    member 'get_Date'. The error: Type mismatch when building 'args': invalid parameter for 
    a method or indexer property. Expected 'Microsoft.FSharp.Core.FSharpOption`1[System.DateTime]', 
    but received type 'Microsoft.FSharp.Core.FSharpOption`1[System.DateTime]'.�
    Parameter name: receivedType
    

    ... which makes me think that the quotation transformation does not fully work (perhaps it replaces FSharp.Data with FSharp.Data.Portable but it does not replace different FSharp.Core.dll?)

I just want to make sure I'm not missing something... it might not be worth fixing if we decide to avoid quotations, but I wanted to check before I try making the changes...

My current branch is here: https://github.com/tpetricek/FSharp.Data/tree/portable

@tpetricek
Copy link
Member

BTW: If anybody can explain what assemblies am I supposed to reference in portable F# library that would be very useful - I'm a bit confused at the moment.

I know the right FSharp.Core.dll is the one in C:\Program Files (x86)\Reference Assemblies\Microsoft\FSharp\3.0\Runtime\.NETPortable

But when I wanted to add more code to the sample (like download data from the web), I was not able to get that to compile. Should I use assemblies from v4.0 or v4.5 under C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETPortable?

I tried v4.5 but then it references Task<'T> that is not compatible with the Task<'T> used in FSharp.Core.dll so that was probably wrong. v4.0 seems to work better, but project properties for my portable F# assembly say that it is version 4.5, so that confused me a bit...

@ovatsus
Copy link
Author

ovatsus commented Dec 31, 2012

I have some more bits that I haven't pushed yet, mainly some sketches of sample apps for the various platforms, but haven't had time to work on it in the last couple of weeks.

The FSharp.Core.dll reference was working ok on my branch at the time, but I haven't merged back your latest changes and tested

I haven't changed the tests to use the portable profile yet

@tpetricek
Copy link
Member

@ovatsus Hm, in that case, I'm probably doing something wrong. If you had some time to check the changes I did (or even merge your recent work), that would be great. I think it would be good to have some compiling portable library in the repository before we try changing the code (try rewriting it without using quotations).

@ovatsus
Copy link
Author

ovatsus commented Jan 11, 2013

Hi, haven't forgotten about this, but haven't had time yet to merge or check your newest changes

@tpetricek
Copy link
Member

Hi,
I did some initial work on the ? implementation that would make it possible to build quotations nicely without using quotation literals. Take a look at src/Providers/Helpers.fs in f0e2be4 (see here).

The last function in the file uses the ? operator:

// operationsTyp is System.Type and this builds a quotation representing
// static method call to ConvertFloat with first argument being a call to GetCulture 
// with string argument and second argument being another expression 'e' 
operationsTyp?ConvertFloat(operationsTyp?GetCulture(culture),e)

// It can be also called with two tuples as argument - in that case, the first tuple
// specifies type arguments (of type System.Type) for generic method calls.
operationsTyp?GetNonOptionalAttribute (typ) (message, converted) 

@ovatsus
Copy link
Author

ovatsus commented Jan 14, 2013

Nice, I'll try to use that instead of the quotations

@ovatsus
Copy link
Author

ovatsus commented Jan 15, 2013

@tpetricek I finally got some time to work on this. I was able to merge your latest change and fix that error you were having building the portable libraries, but I'm not being able to get your ? operator working correctly. I'll finish the pull request with everything working with the quotations manipulation, and then we can try replacing that with the ? operator on a second step

@ovatsus
Copy link
Author

ovatsus commented Jan 18, 2013

I got the portable library support working for almost everything, except for when a type provider needs to generate an expression that uses Some or None. I could workaround it by changing the api slightly so there's no optionals in the interface between the runtime library and the generated code of the erased types, but this is worrying.
Either with quotations or with just plain reflection, I can't find a way to create a Some value for the option type in the portable version of FSharp.Core. The only other type provider that supports the portable profile is Freebase, and it doesn't use options anywhere, it always uses nullables. So, unless I'm missing something, it seems to be impossible to generate an Expression that uses an option (or any other discriminated union) when cross targeting to a different platform. For now I can work around this because I could remove all the cases

The problem is the following:
If I call

FSharpValue.GetUnionCases(portableOptionType)

I get an exception saying portableOptionType is not a discriminated union.
If instead I do

portableAssembly.GetType("FSharpValue").GetMethod("GetUnionCases").Invoke(null, [|box portableOptionType; null|])

I get an array of UnionCaseInfo correctly, and I can look for the Some in there, but then I can't call Expr.NewUnionCase because that union case is from the portable version of UnionCaseInfo, and the type provider is running the full .Net

Any ideas? /cc @dsyme @tpetricek

I'll try to push the code tonight

@tpetricek
Copy link
Member

Funny, I was just talking with @dsyme and Keith (kvb) who is writing some type providers and warned me that standard F# types (functions, options, ...) do not work nicely with portable types - just as you say!

The workaround sounds pretty clever - would it work if the expression tree did not contain Expr.NewUnionCase, but instead directly called the Option<int>.NewSome(123) static method?

(If you update the pull request with the latest code, let me know and I can take a look too.)

Thanks a lot for the contribution, it would be really great to get the portable version out!

@ovatsus
Copy link
Author

ovatsus commented Jan 18, 2013

Your suggestion worked :)

PS: It's actually called Option.Some(123) and not Option.NewSome(123)

@tpetricek
Copy link
Member

When doing the recent work in a6dd98b, I tried to make some code in the CSV provider portable-friendly. I also improved the ? a bit, so that you can create NewObject nodes too:

csvFileTy?Parse(typeof<StringReader>?``.ctor``(source), separator)

A bit ugly, but it does the trick!

@ovatsus
Copy link
Author

ovatsus commented Jan 19, 2013

I'm almost done, only missing the world bank provider. I ended up only using the ? operator only on places that already didn't use quotations

@ovatsus
Copy link
Author

ovatsus commented Jan 19, 2013

I've pushed a new version. It's still not complete but it's getting closer.
There are two big pieces of work here:

  1. Make the providers work after splitting into a runtime + designtime assembly (which is the hardest part)
  2. Have a portable version of the runtime versions and make sure it also works (has further complications because of different FSharp.Core versions)

With the current commit 1) works for CSV, JSON, XML, but not for WorldBank, and 2) works fully for CSV and partially for JSON and XML (array types not working)

There are a couple of new projects under the samples folder. The WindowsStoreApp runs fine, but I'm not able to test the WindowsPhone8App project, because I don't have a WP8 device, and the emulator doesn't support my processor. I've also created a WindowsPhone71App project, but the only portable profile available for F# doesn't support it. While running I get the error IStructuralEquatable not found on mscorlib. F# 2.0 used to have a version of FSharp.Core compiled for Windows Phone 7.1. @dsyme is that also available on any way for F# 3.0? I'd really like to be able to use F# 3.0 to compile to this platform.

@tpetricek I ended moving the whole StructureInference to the runtime assembly, but we might only need some parts. Also, a better name for the Importing module would be nice.

In the process I've also make all the providers use readTextAtRunTime, so you can close #14 and #16 after this is merged. But please don't merge until at least 1) works for WorldBank

I haven't figured out the problem with WorldBank yet, but I know what is the remaining problem in JSON and XML, just didn't have type yet to fix. The problem is the use of makeFunc that creates functions of the wrong FSharp.Core version. If you run FSharp.Data.Tests.sln while debugging FSharp.Data.sln you can see it throw an exception later on inside makeMethod.

BTW, initially I changed all calls of makeMethod to your ? operator, as it looked nicer, but that delays errors, and makes it harder to debug, so I ended up switching to makeMethod again

To debug, in addition to running two Visual Studios, I've added Test.fsx that shows the signature of the generated types and members. If [DESIGNTIME] appears anywhere in the output, then some type is still pointing to the wrong assembly

@tpetricek I could use some hand to try to figure out what's the problem with WorldBank, which happens even on the normal profile. Just open WorldBank.fsx on the samples folder, and you'll see this error:

A reference to the type 'FSharp.Data.WorldBank.ServiceTypes.WorldBankDataService' in assembly 'FSharp.Data' was found, but the type could not be found in that assembly

This error is different from all the others I've had before, it was usually something like "type T is defined in assembly FSharp.Data.DesignTime, please add a reference to it"

PS: I did a force push, so probably if you try to merge to the portable branch git will get confused. In any cases, my commit was after all current commits, so there's nothing to merge further right now

@ovatsus
Copy link
Author

ovatsus commented Jan 20, 2013

Added a sample for Silverlight also. Two things don't work:

  • There's no System.Xml.Linq in Silverlight, so we have to keep code that uses the XmlProvider in a different function and don't call it to avoid .Net to try to load the missing assembly
  • For some reason, the webrequest calls never return when trying to connect to http://ichart.finance.yahoo.com/table.csv?s=MSFT, I even tried doing it manually instead of using the Load method, maybe it's something specific to my machine, I get some internet zone warnings in Internet Explorer

@ovatsus
Copy link
Author

ovatsus commented Jan 20, 2013

The WorldBank provider now works correctly for the version with type provider arguments (e.g. WorldBankProvider<"World Development Indicators", Asynchronous=true>), but still gives the same error when using the version without arguments, which to me doesn't make much sense as they're almost exactly the same code. It's the only thing missing for this to be mergeable to main.

@tpetricek
Copy link
Member

The changes are looking great, I merged them into the portable branch to take a look and it seems to be working fine for me.

Even though everything seems to be working fine, the quotation transformation code seems to be getting a bit complex (it would be great if you could follow the style used in the rest of the code and comment the top-level functions, it would be also nice to keep uniform indentation, at least within a single file, but that would mess up the history, so I'm not sure what to do). Do you think we should follow the advice by @dsyme and Keith and avoid using quotations in the long run?

  • I see you removed ? from the convertValue function (a change I did in f0e2be4). Is this an intentional or accidental change?
  • You mentioned changing makeMethod to ? delays errors and complicates debugging. I was hoping to switch to ? as it makes the code a lot nicer - can you give some details? Perhaps we could somehow improve ?

The testing script that detects design-time types in signatures is pretty cool! Do you think it could be turned into something that can be executed as part of unit tests? (So that we can run some tests automatically.) I suppose this would be useful even if we switched from quotations (because there might still be a bug in locating the right type...)

@tpetricek
Copy link
Member

And thanks again for all the hard work on making this possible! I'm really excited about making this work in Silverlight (and elsewhere). Once this works, we should be able to run the documentation live in www.tryfsharp.org, which should make it even more attractive!

I think once we finish this, we can mark it as a first mature release and move the main repository to http://github.com/fsharp

@ovatsus
Copy link
Author

ovatsus commented Jan 21, 2013

I finally found what's the problem with WorldBank. It's some limitation in ProvidedTypes when you have a type provider with the same name of a namespace, it get's confused. I renamed WorldBank to WorldBankData and WorldBankProvider to WorldBankDataProvider, following the same pattern used in the Freebase type provider, and now it all works. Do you agree with that change?
I think it would be a good point to close this pull request and merge this to master, as with this last commits, everything that was working before continues to work, and we have CSV fully working on portable, and JSON and XML working for scenarios without arrays and optionals. I can then create new issues for the remaining problems.

…o original namespace +

Move back StructureInference to design time assembly, leaving only the minimum needed types +
Make sure everything not supposed to be used directly is hidden under FSharp.Data.RuntimeImplementation subnamespace
@ovatsus
Copy link
Author

ovatsus commented Jan 22, 2013

I did a little bit of refactoring to make sure namespaces are consistent, implementation details are properly hidden, and to make it more close to the main branch.
I've also added some preliminary support for the Http class in the portable profile

@ovatsus
Copy link
Author

ovatsus commented Jan 23, 2013

The WorldBank provider is now also present on the portable version of FSharp.Data. There's no caching because there's no file access, but maybe later we can add and in-memory cache at least. I changed the implementation to use the JSON api instead of the XML api so there's no dependency on System.Xml.Linq for it (Silverlight doesn't have it)

@tpetricek
Copy link
Member

Excellent work! I'll take a look at the latest version and we can finish it & merge it on Friday.

@ovatsus
Copy link
Author

ovatsus commented Jan 28, 2013

Sorry, the last commits were on a local branch but when I pushed they went to master :/ Have to take a better look at git docs

tpetricek added a commit that referenced this pull request Jan 28, 2013
Support portable libraries (and lots of other changes)
@tpetricek tpetricek merged commit a533d29 into fsprojects:master Jan 28, 2013
@tpetricek
Copy link
Member

I merged the pull request (so that we finally have all the changes in the master branch here), but I will review the changes tomorrow and may do a couple of changes here and there (so if you make some changes in the meantime, we might need to resolve some conflicts).

If you can send one pull request per one issue or one logical change from now (from separate branches), that would be good, because it makes reviewing, discussing and accepting pull request a lot easier. Thanks!

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.

2 participants