jcheng31 / DarkSkyApi

An unofficial C# library for the Dark Sky weather service. Targets .NET Standard 1.1.
MIT License
32 stars 18 forks source link

Compiling with .NET Native toolchain causes InvalidDataContractException during serialization #13

Closed anthonyhenderson closed 8 years ago

anthonyhenderson commented 8 years ago

Using this library in a Windows 10 app which compiles against the .NET Native toolchain, I'm getting an InvalidDataContractException in ForecastApi.ParseForecastFromResponse when the call to serializer.ReadObject is being made:

Type 'ForecastIOPortable.Models.Forecast' cannot be serialized, serialization code for the type is missing. Consult the SDK documentation for adding it as a root serialization type.

I've tried adding various runtime directives but so far none of them have worked. Adding @MattWhilden on the .NET Native team to take a look.

MattWhilden commented 8 years ago

Hey. Sorry about the delay. The long weekend and being a bit short staffed here has keep me busy with another issue. I'm checking this out now and will report back in a bit.

MattWhilden commented 8 years ago

So I pulled down this repo and built my own Forecast dll as per another issue you had open. I've added it to a new UWP project in VS Update 1 and added the few lines from the main page to get started. With that setup I hit a MissingMethodException and it looks like some issue with the .NET Native tools resolving System.Net.Http.Primitives and System.Private.Networking... So I'll have to take a look at that. One step forward one step back. :-)

It's not what you've reported through... what build of VS are you using?

anthonyhenderson commented 8 years ago

@MattWhilden I'm using VS 2015 14.0.23107.0 and I too had some issues with missing network APIs. I believe all of that should be resolved if you add the Microsoft.Net.Http NuGet package.

MattWhilden commented 8 years ago

Interesting. I've added that nuget and I still get the same error. I'll keep fighting with it.

Additional information: Method 'HttpClientHandler.set_AutomaticDecompression(DecompressionMethods)' was not included in compilation, but was referenced in ForecastApi.GetCompressionHandler(). There may have been a missing assembly.

If there is a handler for this exception, the program may be safely continued.

anthonyhenderson commented 8 years ago

Ah yes, that was quite annoying. Looking at my solution I'm not sure how I got around that. Do you have Microsoft.Bcl.Build? I'll keep looking when I have some time.

MattWhilden commented 8 years ago

So this is a real bug in .NET Native that this hits because this isn't building against .NET Core. This can be worked around by adding a reference to the System.Runtime.Serialization.Json contract which is where these types all live in a non PCL world. They used to live off in System.ServiceModel.Web. As a somewhat cleaner possibility, this library could be retargeted to the .NET Core profile. I don't actually know what's best for this particular project and it'll probably need some input from @jcheng31.

It's a bit unfortunate because we make it like 95% of the way to this working perfectly correctly out of the box. We correctly analyze this line (https://github.com/jcheng31/ForecastPCL/blob/master/ForecastPCL/ForecastApi.cs#L513) and make a note that you're totally going to want to serialize Forecast. We then write that information down into one of our intermediate files but when we go to spin up the serialization generator (sg.exe) there's a bit of overly clever code that says, "Hey, this library totally doesn't depend on System.Runtime.Serialization.Json. That must mean all this stuff I figured out about using DataContractJsonSerilaizer is totally wrong and I won't write it out." The fix on our side will be to remove this extra bit of fancy state tracking and will be available in Update 2 of Visual Studio sometime early next year.

Let me know if you have any questions. Happy to help.

anthonyhenderson commented 8 years ago

Thanks for the info @MattWhilden. So in the meantime, if I want to get my app compiling against this library how do I add this reference? I tried adding the System.Runtime.Serialization.Json NuGet package but that didn't help.

jcheng31 commented 8 years ago

Thanks a lot to both of you for digging into this! Sorry for not being more active here.

Regarding the two options - I'm not entirely sure what the trade-offs are between them. I suppose the priority for this library is to ensure it'll still work across all platforms (including mobile) going forward; if targeting .NET Core allows that, then I'm happy to just go with that. I'm still somewhat new to all this, so I'm not sure what that exactly entails; I took a brief look at the introductory blog post, which I interpreted as saying .NET Core is the way to go for cross-platform stuff in the future.

MattWhilden commented 8 years ago

@jcheng31 You're right, that's the surface area we're targeting for the future .NET platforms including cross plat and .NET Native. Not a bad thing to head that way.

@anthonyhenderson Let me see how much work it is just to get this targeting .NET Core and building. Can't be too hard right?

MattWhilden commented 8 years ago

Alright. So I've learned a valuable lesson about hubris on the internet. Getting this project to be retargeted to .NET Core (aka ASP.NET 5 profile) got me a bit stuck... you have to migrate to nuget 3 and then I ended up with a busted project.json etc etc.

What does seem to work is starting a new portable library project targeting Windows 10, .NET 4.6, and ASP.NET 5. After importing the code into that project, it builds just fine, seems to know what's up and has the correct assembly reference to make .NET Native happy. I'll have to get someone from the framework team to help me actually sort this out tomorrow but that's what you get when you let a compiler dev fight with libraries and build stuff eh? :-)

I've pushed my changes into my fork here: https://github.com/MattWhilden/ForecastPCL/tree/PortToCore/

Perhaps that'll unblock @anthonyhenderson

anthonyhenderson commented 8 years ago

I can confirm that the portable library targeting .NET Core makes my app run as expected :)

Wasn't very hard to create the new project but I suppose it all depends on what framework versions @jcheng31 wants to support moving forward.

MattWhilden commented 8 years ago

It does look like the suggested way to get moved onto .NET Core (aka ASP.NET 5 profile) is to melt down your project file and start a new one. :-(

It's possible that the correct thing here is just "do nothing". Folks that have an issue can use my hacked up fork and when we release the next version of .NET Native, I expect this issue to be fixed (early next year some time).

cbordeman commented 8 years ago

I'm using Visual Studio 2015 Update 2 and I'm still getting this error. My code is at https://github.com/cbordeman/gameofgo. If I enable .Net Native I get the crash, on the type GoGameState. I've added all the types used in my OnRegisterKnownTypesForSerialization(), which uses DataContractSerializer. All types have empty constructors.

If I disable .Net Native, all works fine, but when I generate the .appxupload file for the Store via Visual Studio, a _Test folder is created, but the necessary .appxupload file is not generated. I'd be happy to disable .Net Native if I could just get the .appxupload file without the app crashing.

MattWhilden commented 8 years ago

There's more than one reason you may be seeing this error. It may be that this wasn't completely addressed by the work in Update 2 or some other issue. Hard to say at first glance but we're happy to help. Can you start a mail with us at dotnetnative@microsoft.com. It helps with our internal tracking here at MS.

MattWhilden commented 8 years ago

Happy to hear you're unblocked. Do let us know if you see other oddities. Happy to help.

cbordeman commented 8 years ago

Sorry, this issue actually isn't resolved. I will email the team.

MattWhilden commented 8 years ago

We've sync'd up and the issue is being investigated. It looks similar to this issue but I suspect it has a different root cause. As this doesn't have to do directly with ForecastPCL I'll just report back when we have a solution (don't want to leave anyone hanging).

jcheng31 commented 8 years ago

Alrighty - it's been quite a while, though I think I've finally gotten this resolved. I've (sadly) been away from the .NET world for a while, so I'm not sure if VS2015 Update 2 would have fixed it on its own; I went and created a new solution/project, added ASP.NET Core 1.0 as a target (losing Silverlight in the process), and copied the existing code into there.

I tested the NuGet package locally in a UWP app with .NET Native enabled, and it worked in both Debug and Release mode. Will leave this issue open for a little longer just in case I missed something, though!

MattWhilden commented 8 years ago

Perfect. Update 2 should contain the fix for ForecastPCL. Thanks for letting us know.