FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
822 stars 342 forks source link

Portable project includes impossible Win Phone targets #160

Closed tiloc closed 8 years ago

tiloc commented 8 years ago

In the portable project Fhir.Core.Portable45.csproj, Win Phone 8.1 and Win Phone Silverlight is being included as a target. However, the DataAnnotations library is not available on these targets. See the table on: https://msdn.microsoft.com/en-us/library/gg597391(v=vs.110).aspx

It seems my installation at work (VS 2013) didn't catch this issue, but my VS 2015 at home flags the reference to System.ComponentModel.DataAnnotations with a big yellow warning sign and fails to build the project.

tiloc commented 8 years ago

Maybe this would help? https://www.nuget.org/packages/Portable.DataAnnotations/ Or ditch Win Phone as a supported target...

tiloc commented 8 years ago

I discovered the reason for the difference in my 2 installations of Visual Studio. My Visual Studio 2013 upon loading the .csproj file pointed the reference to a Silverlight 5.0 SDK for desktop applications. My VS 2015 doesn't have this SDK installed. Removing the Silverlight 5.0 SDK from my VS 2013 resulted in the same (IMHO correct) behavior, complaining that System.ComponentModel.DataAnnotations is not a portably supported assembly.

brianpos commented 8 years ago

I have a project that runs on the phone 8.1 platform quite happily. No validation attributes though. Will look into that portable data annotations though...

tiloc commented 8 years ago

A related topic: The latest addition to EnumExtensions.cs on the develop branch introduces a dependency on DescriptionAttribute, which is not part of PCL. There are perhaps two solutions:

  1. http://stackoverflow.com/questions/18912697/system-componentmodel-descriptionattribute-in-portable-class-library - i.E. to use the fully portable "DisplayAttribute" instead.
  2. https://github.com/cureos/shim - I.E. add a full-fledged portability support library. This works, I have tried successfully, but Shim is licensed under the LGPL, which would raise some flags in many corporate environments.
brianpos commented 8 years ago

An implementation of the attribute had been added to the project to support this. Inside #define. I'm using the library in a Windows phone 8.1 application.

tiloc commented 8 years ago

I see the implementation inside template-bindings.tt/cs. This places it into the Hl7.Fhir.Model namespace. The EnumExtensions.cs lives in the Hl7.Fhir.Support namespace, and does not use the Hl7.Fhir.Model namespace.

tiloc commented 8 years ago

I think I have come a step closer to resolving the mystery :-)

The .csproj files for the Portable45 projects on the github repository contain "HintPath" entries, telling Visual Studio where the References in the project should be resolved to. The HintPath for the System.ComponentModel.DataAnnotations is ..............\Program Files (x86)\Microsoft SDKs\Silverlight\v5.0\Libraries\Client\System.ComponentModel.DataAnnotations.dll.

I.e. whenever one has Silverlight 5 SDK installed, the assembly will be picked up from there and the compile succeeds. Whether the result will run on the target platform is pure coincidence, as the Silverlight 5 SDK may give you richer APIs than the restricted .Net runtime on the target platform.

For people who don't install the Silverlight 5 SDK it will not compile, as the references from the HintPath cannot be found.

So I guess we are both correct in what we are observing. You can run on Win Phone 8.1, because you have Silverlight SDK 5 and the Fhir.Net library by coincidence sticks to the limits of the Win Phone 8.1 runtime. And I can't compile, because I am not a Silverlight developer and therefore don't have that SDK installed.

brianpos commented 8 years ago

That is correct. The only part from the assembly that we use is for the validation, and we know this doesn’t work on the mobile platform for other reasons, no problems. 😉 I do like your suggestion of using that other portable library in its place, would be interested if that has the validation stuff in it too. And given the move into the FHIRpath stuff, not sure if it is worth spending time on it just yet. Brian

tiloc commented 8 years ago

I think it has replacement code for many things which are not contained in the more limited runtimes. I see source code for re-implementations of things like "ValidationAttribute" + dozens of others. I think they really aim to enable the full scope on every platform. They list CreditCardValidator as something they cannot support on every platform. So no revenue cycle management on Fhir :-)

I have a branch with the changes required to use PortableDataAnnotations in the project plus the fixes to have the DescriptionAttribute in the right place to compile. I can provide a pull request which illustrates the changes (very small and contained). Being able to compile all projects is the prerequisite for the Mother of all Pull Requests(tm) - image you type "runbuild Package" in a PowerShell and a few minutes later a freshly built, unit tested NuGet package plops onto your hard-drive. I have this working here :-)

BTW: What is FHIRPath?

ewoutkramer commented 8 years ago

Sounds like a plan, I would not mind integrating that. Brian?

tiloc commented 8 years ago

Looks like I need another evening of hacking. I have 2 unit tests failing on the portable project which do not fail on the Net45 project. One is potentially a bug in the portable JSON library (a huge number being crammed into an Int64 instead of a BigInteger, and updating JSON to 8.0.2 which supposedly fixes such bugs didn't help). The other one seems related to a peculiarity with reflection and validation. I have a suspicion which code is responsible, but will have to run a debugging session.

ewoutkramer commented 8 years ago

Thanks Tilo. I'll wait to integrate your pull request then.

tiloc commented 8 years ago

PR #165 has landed. CAUTION: To not obscure what I did, I have not yet submitted the 74 newly generated source code files. I have touched the .tt files (this is included in the PR). If you otherwise agree with what I did, let me know, and I will add the generated files to the Pull Request. I can compile and run all unit tests now. There are 2 noticeable exceptions: The portable JSON parser is intentionally limited to Int64 and cannot do BigInteger (I checked the source, it is an intentional workaround for a bug in Mono). The other failing unit test is about Xhtml validation, which is still not there on the portable platform. All other Validation tests execute and pass.

A change which I wanted to point out in particular is my decision to move from DescriptionAttribute to a newly added EnumDocumentationAttribute. This matches the naming which you used in EnumExtensions.cs for the corresponding accessor functions. It eliminates #ifdefs and makes it 100% identical across platforms, but not sure if there was a tool interoperability or other reason to use DescriptionAttribute.

brianpos commented 8 years ago

Many third party tools use the description attribute for labeling. For example you put one on a dev express table it would use those labels. I also have a library that reads the descriptions for constructing error messages.

tiloc commented 8 years ago

Ok, will look for a way to use the original DescriptionAttribute on platforms which have it, and a replacement on platforms which don't. Will make further commits to PR #165 when available.

brianpos commented 8 years ago

Thanks, (are you on skype and can join in on the implementers chat?) I'm sgtshultzpos, ping me on Skype. You could derive the fake DescriptionAttribute from whatever you like/find.

tiloc commented 8 years ago

PR #165 amended. Back to a (new homebrew) version of DescriptionAttribute. Migrating over to DisplayAttribute might enable even richer tool support in the future.

tiloc commented 8 years ago

Googled for Dev Express, they support DisplayName (not localizable), but not Display. https://www.devexpress.com/Support/Center/Question/Details/S170031

tiloc commented 8 years ago

I think with the merging of PR #165 this issue is addressed and closed.