-
Notifications
You must be signed in to change notification settings - Fork 385
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
UnitsNet types are not Windows Runtime Component Compatible #150
Comments
Hi, thanks for reaching out. I'm not familiar with developing towards WinRT, but from this SO answer I understand that UWP C# apps can interact with Units.NET just fine, but the CLR must project the managed types to Windows Runtime types when consumed by Windows Runtime Components. I read your link and see there are some constraints to the types used. Below are some initial thoughts, please correct me if I am wrong.
So basically, stick to Int32, Double, DateTimeOffset and a small set of .NET types. We should be near this, I think.
All unit types are
Not a problem.
I believe we are good here.
We use very few interfaces today, I think we are good here.
We have a few exception types, but other than that we should be good. I'll try to set up an UWP app with a Windows Runtime Component and reproduce the behavior you describe, and try to get a better picture of what refactoring is required to be compatible. If it is feasible, then I'd be happy to put in some effort to get there. The strategy might involve modifying the code generator scripts to output two sets of source code; .NET and WinRT. I'll get back to you when I know a bit more, likely by tomorrow or the day after. |
Thank you so much for getting back so quickly anjdreas. You are very very much on the right track in your thinking above. Honestly, here's the easiest way to see what would need to be changed:
The compiler will tell you each and every item that isn't compatible and actually gives fairly helpful links and messages about making the conversion. The main value in performing this work is that it allows your library to be projected natively into any language supported by the Windows Runtime. Therefore your library can be used not only by C# and VB, but also by C++ and JavaScript. This is possible through the magic of language projection, but language projection comes at the cost of the restrictions mentioned in that link. In addition to your library being able to be used by those languages, it also allows your library to be used as part of other libraries that target those languages. In my case, my library is about making it easy to access sensors and devices on Windows IoT Core. I could have chosen to implement my library as a Class Library instead of a Windows Runtime Component, but I know that a lot of IoT developers use C++ and I know that a lot of students find it interesting that they can write full fledged apps on IoT devices using JavaScript. If I wrote my library as a Class Library, it couldn't be used by those folks. Porting to a Runtime Component also allows your library to be used in those languages on new devices like XBOX One and HoloLens. To get me by in the meantime, I've added a Temperature class to my own library that's based very much on your implementation and I gave you credit in the source. My implementation is nowhere near as robust as yours but it's a start and if your library ends up being Runtime Component compatible I'd like to yank out my implementation and just take a dependency on UnitsNet. (Thank you for publishing your code under a very non-restrictive license, BTW. I wouldn't have been able to do that otherwise.) As you investigate making the move to a Runtime Component you may or may not find features of your library that won't work under the Runtime Component restrictions. If everything moves over easily, AWESOME. Then it's just about changing the way the library is compiled and changing the binary that NuGet deploys for UWP apps. However, if you find features that have to be removed I recommend using conditional compile statement and creating a new NuGet package called something like UnitsNet.UWP (Universal Windows Platform). You may want to do this anyway, for example, if you want to keep your core types as structs instead of classes on non-UWP platforms. If you have any questions about this process whatsoever please don't hesitate to reach out. I've already endured the pain - err, I mean, enjoyed the learning exercise - of porting several Class Libraries to Windows Runtime Components. I'm more than happy to share knowledge or help with translation. Thank you again for such a quick response and for looking into it! Jared |
I think this should be doable. I got a working prototype with a Windows Runtime Component where I copied in Temperature and its dependencies, and modified them until they compiled. Branch here: https://github.com/anjdreas/UnitsNet/commits/uwp The features we lose are:
|
I'll take a look at this tomorrow again, getting late here now. It's possible we can do with some conditional compile statements, but there were enough changes to get this prototype working that I'm worried it will be a bit of a mess to keep both variants in the same files. We will see. Either way, conditional compile or separate code for WinRT and .NET, I think I should be able to pull this off over a couple of evenings. Thank you for your assistance and input, it is much appreciated. |
I'd love to help if you need it. I would never try to tell anyone how to maintain their own library, but I would love to see this done with conditional compile statements. The main reason is all too often I've seen wonderful libraries like yours get forked for one odd reason or another and the forks tend to fall behind when new features get added to the main branch. You appear to be super active on your project, which is amazing, and I'd hate to see the UWP fork fall behind. If it would help for me to jump in and do some of the work, I'm happy to contribute some time. You could just put everything in the UWP fork and we could divide the work and keep plugging away until we get something workable that we can merge back into the main branch. Let me know what you think after you've had some time to tinker with it. You've created an awesome library. |
Thanks, I will probably pick you up on that offer. I propose I scramble together an initial draft with modifying generator scripts and using conditional compile statements, then you can review that and we take it from there. To be clear, my intention was never to fork this out in such a way that it would fall behind. It will be part of the main library and main repo, hopefully even the same nuget, but seeing we are already generating most of the code using powershell scripts it may or may not be better to simply generate a second set of source code targeted at WinRT - purely as an alternative to conditional compile statements. Just a heads up, we are awaiting a newborn anytime today or in the next few days, so I may be unavailable at least a couple of days when that happens. |
First and foremost, congratulations!!! I'm quite familiar with that time of life, so no worries at all and enjoy it! I also didn't realize how much code is being generated. If that's the case, conditional compiles aren't nearly as important for keeping things in sync. I'll totally defer to you on which approach you think makes the most sense, and I'll help however I can. The only part I think we need to be careful with is putting it all in the same NuGet package. At least at the moment I'm not thinking that's a good idea. To your point above, this change does require dropping some valuable features from the library like operator overloads and custom exceptions. If we crate a new NuGet package (something like UnitsNet.WindowsRuntime) then Windows 10 developers who write their apps in C# and VB still have the option to use the more fully featured version of your library. However if we add the Uap10.0 folder to your existing NuGet package, that folder will have a higher precedence over all other versions in your package in Windows 10 apps. And that will force the usage of the Runtime Component version on Windows 10, even if the more fully featured version could be used. I don't think we want to force this change when it's still completely valid to use the Portable / Net40 version in Windows 10 apps that are written with C# and VB. What do you think? |
Nice to see more people interested in the library. While I have no intermediate use for this feature I like libraries supporting as many platforms as possible (and I do have some small Windows IoT projects on the side). Hopefully most of the functionality could be kept. As @anjdreas has suggested I would suggest to generate another set of code for WinRT but I would also suggest to completely share the "handwritten" code and if necessary use conditional statements in these files. That way we'll get the best of both worlds... |
So I'm back and all is well with the new family member. I took a stab at this and there is now a WIP nuget out based on the uwp branch. Rambling thoughtsCode refactoring went pretty much as expected. I did stick with conditional compile statements, and I think I'm fine with how it turned out. I mostly wound up I first attempted a single nuget with just a new I must say, I did struggle a bit with the new nuget system. Apparently packages.config files are on the way out, replaced by Request for comments
|
@anjdreas Great work!
|
Thanks, I'll split into a separate solution then. @jbienzms Have you had time to look over this yet? I still don't know if it actually works in an JS and C++ app. |
Well shoot, I was replying to the e-mail bot last week and apparently it didn't actually post my replies here. Let me first start by posting those messages that didn't make it: Monday, March 14th @ 3:28 PM
Monday, March 14th @ 3:34 PM
Now, on to your more recent questions:
Actually, this is probably due to everything getting compiled for .Net Native. There's an awesome article on what .Net Native is and what it means for your apps and libraries here. Though technically this shouldn't be happening for your library, just for any test applications or harnasses that use the library. Libraries can still be distributed as Any CPU, though UWP applications must always be compiled to a CPU target (x86, ARM, etc.)
I would say probably yes. Mainly because some of your developers may not have the latest Visual Studio and may not have the tools for Windows Store development installed. In those cases they will see errors. Since you've done all the hard work to do conditional compiling where it makes sense, obviously those two solutions can share the same files. The only downside to this approach is that a community contributor could fix a bug or add a feature in a pull request that compiles in the NET solution but doesn't compile in the UWP solution. Meaning, as you accept pull requests you'll want to do your own compile against both solutions to make sure they work or refactor them.
If it compiles and can be referenced from another UWP component, it should work in all languages. The issues you've already dealt with (like default overloads, etc.) are what allow the code projection to work. We could build some WinJS test harnasses, but honestly I think that's not necessary. I'm a bit under water coming back from being out of the office last week but let me do a mini code review real quick. Vector3.cs UnitsNetException.cs UnitClasses/Temperature.g.cs Conditionally compiling out the nullable types is all correct. Unfortunately. I see that a lot of your conditional compile statements are there because of changing the access modifier (changing public to internal). I wish there was a shorthand for this but I don't see one. There is no such thing as creating an alias for an access modifier. For all of the ToString methods you might consider using a type alias at the top of the class. For example:
Then you could write a single method body like this:
Of course you would still have to use conditionally compiled code within the body because the code paths are pretty different, but at least there would only be one method body instead of duplicating the whole blocks of code just to change the signature. The one thing this doesn't account for though is your use of [CanBeNull] when Culture is a string. That appears to be a ReSharper thing so I don't know if that's important. Please let me know if there's a particular piece of code you'd like me to review. So far it looks great. And when you have a theoretical NuGet package published let me know that too and I'll reference it in my IoT library to see how it works out. When you're ready to go live, I can push an update on my library to start taking a dependency on yours. Thanks for all the hard work man! I'm super impressed!!! |
1. Nuget name: UnitsNet.UwpI did a quick search on packages on nuget.org: Search for tag:uwp returned 294 packages. These are the first ones with any relevant term in their ID. By browsing a bit, very few recent packages seem to follow the winrt naming. UAP has few packages and the two last terms even fewer, which makes UWP my favorite. 2. Modified nuget descriptionI modified the description/summary to be even more clear about being a stripped down version. Does this work?
3. Added solution UnitsNet.Uwp.slnMoved UWP projects out of main solution. 4. No UWP specific testsI do note we have no tests explicitly for UWP, but I'm not sure if it's worth a lot of effort. The shared code is quite tested and I don't see a lot of potential for bugs in the conditional compile statements, knock on wood, so unless you have anything in particular in mind I vote we leave it without tests for now. 5. Building UWP nuget with AnyCPUYou did mention this, but I just wanted to make sure that AnyCPU is the way to go? 6. Type alias Culture for string/IFormatProvider paramsThanks for the tip, that was clever and saved me a lot of duplicate code. 7. Nuget is outhttps://www.nuget.org/packages/UnitsNet.Uwp/3.30.0-alpha2 8. Test on a JS or C++ UWP appIt would be really good if you could test the nuget on a JS or C++ app to make sure it works as advertised, but if you are confident about that part working then I'm fine with that. 9. PR is ready to mergeI'd like some feedback on this last work and the numbered sections, before merging in the PR. |
Don't forget that all of those naming conventions were valid at different times. WinRT was Windows 8.0, UAP was Windows 8.1 and Windows Phone 8.1 and UWP is Windows 10 and Windows Mobile 10. Just looking at the total number of packages that use a particular suffix is not going to tell you which suffix is the right suffix for your project.
You absolutely can use the UWP suffix if you want. Just be aware that when a Windows 10 developer searches NuGet for packages they will see one entry for UnitsNet and one entry for UnitsNet.UWP. Windows 10 developers have sort of been "trained" that when they see UWP they see "that's the package for me". In your case, though, that's actually probably NOT the package they want the majority of the time. The only time a developer wants the this version of the package is if they're writing a Windows Runtime Component to target all possible languages. The average UWP developer is building an app, not a library. So the average UWP developer is likely to pick the wrong package if they see this one listed as "UWP". That's specifically why I'm recommending using a suffix that is uncommon. We don't want the average developer to select this version. Your main version offers many more features and is compatible with most UWP apps. We want them to pick your main version by default. Again, if you feel strongly about using the UWP suffix, please consider making sure the very first line of the package description starts with something like "Most UWP developers should NOT use this version and instead use the NuGet package called UnitsNet. This version of UnitsNet is compiled as a Runtime Component to support all UWP languages and other runtime components. It has reduced functionality compared to the main UnitsNet package."
Yes. Though "stripped down" does sound kinda bad now that I read it back (I know I suggested something like that previously). That's why I suggested the text above. But whatever you choose, you're on the right track. The primary concern is letting them know that if they're picking this version over the other one they should be doing it for a specific reason.
Nope, I'm fine with that. Your other version is well tested and this version is mostly just hiding some things. You could possibly write some unit tests for string conversion because that path is different, if you wanted to, but all your other test cases seem well covered.
Excellent. Let me know if any of my notes above change your mind or if you decide to stick with the UWP suffix. When you've settled on the final naming convention for the package I'll update my library to consume it.
I can do that when we have the final package naming convention nailed down. I'll build a sample app off of that package out of NuGet.
Sure. Anything in particular? Looking at the Temperature class I see that a couple overloads of ParseUnit use CultureInfo explicitly (either (CultureInfo)null or new CultureUnfo(cultureName)). I just wanted to make sure that was the right approach now that you're using the alias trick to switch between CultureInfo and string. It looks like the main implementation of ParseUnit takes an IFormatProvider. Honestly I haven't done anything much with cultured string parsing so this may be the exact right way of doing it. I'm just pointing it out mainly as a note for you to to double check that's still the right way with the alias. It very well may be. Let me know if there are any other areas or things you'd like me to review specifically. I'm more than happy to help. |
Whoops, I missed one:
Yes. Your NuGet library should be built as Any CPU. Only applications and C++ libraries must be processor compiled. What actually happens with your code (believe it or not) is it gets merged into the application binary at compile time using the same processor architecture that the application is targeting. Fascinating stuff. That link I provided above on .Net Native explains in more detail if you're interested. |
Thanks for the detailed feedback. 1. Nuget namingYour point is well taken and I agree it might be good to distance the nuget from the UWP naming. I then propose 2. Nuget descriptionAssuming we go with the naming in point 1, here is something in-between our two suggestions:
3. Type aliasingThanks for pointing that out, I did double check those overloads and found a couple ones that could be improved to use Culture alias more. The main exceptions are ParseUnit() methods and UnitSystem constructor methods, which complicated things by being public and also used internally. That is why the 4. TestsThese are definitely needed. I created a JS UWP app to test this thing and right off the bat I'm hitting bugs due to ToString() parameter being string instead of CultureInfo. I've added some of the existing tests that did not require any modifications, mainly Parse() and ToString(). These are also the ones with the most conditional compile statements in them, so that was good to add. 5. Can't install nuget on JS appI got to wrap up for today, but I hit a wall trying to add the nuget to the JS app. Adding the project reference works well, but not the nuget. Not sure what I'm doing wrong. Any ideas? I've pushed the latest bits to the branch. Also here is the latest nuget: I'm leaving for easter holidays for a couple of days, so I will probably not be very active before Saturday or so. |
Beautiful! Only a very minor suggestion: instead of (JavaScript and C++) perhaps (including JavaScript and C++)? When I read it I just felt it was saying only those languages were supported. Who knows if other languages will be supported in the future and of course C# and VB are already other options today.
Understood. Thank you for re-checking just to be safe.
Uh oh. That's honestly kind of unexpected. Can you share what your test code is, what the expected result was and the actual result? I'll see if I can explain why, make suggestions or dig into it more.
First and foremost, make sure you've selected a CPU architecture from the solution dropdown. I've noticed NuGet can get wonky if the application project type is set to Any CPU. With x86 selected, when I try to reference your NuGet package I get the following error in the Error window:
I believe the problem is somewhere in your .nuspec file. For example, it's worth noting that your .nuspec lists the following dependencies:
In the .nuspec for my Runtime Component package, the only other dependencies listed are other Runtime Component packages I depend on:
I don't have any group or targetFramework elements but I could be totally doing it wrong. At the very least I think you at least need to add a targetFramework for uap10.0 to your dependency list. Here's a good article on NuGet 3.1 and UWP support: http://blog.nuget.org/20150729/Introducing-nuget-uwp.html That's where I found that targetFramework string. The only other difference between our nuspec files is you have a language tag and I don't. But I doubt that matters. |
Agreed.
The bug in this case was trivial and my bad, but it was only triggered in the UWP code path and underlined the importance of having tests for both code paths in general. The tests I added should cover ToString and ParseUnit to some extent, but conversions and things like no decimal type are still untested in UWP code paths. My point is just that I proved myself wrong in assuming it was fairly safe to not test the UWP code, and it's probably worth adding a few more tests towards UWP code paths to improve coverage.
This section was actually just an experiment, based on the GoogleAnalyticsSDK nuget that worked. It did not help. Originally I did not have any dependencies at all, which I believe is correct for this lib. |
Yes this is the error I got as well. Haven't had time to research it yet, but I will read your link. Hopefully this is just a matter of tweaking the nuspec. |
I think I got it all working now. I removed the Anyway, there are now two UWP test apps; C# and JavaScript. I've edited the .nuspec as discussed, so I think this is getting ready to merge and publish to stable nuget now. Thoughts? https://www.nuget.org/packages/UnitsNet.WindowsRuntimeComponent Notes: |
I honestly think having to set SpecificVersion isn't right... Especially since you have to manually edit the .jsproj file. I tried adding my own WRC component to a WinJS file and I had a different problem. I got no error or warning symbol, but mine never showed up as a reference! I'm still looking into it. And I see you've got some traction on your issue. That's awesome. Let me know if it goes stale. |
Just an update, according to discussion in NuGet/Home#2406 the JavaScript team is aware of the problem and working on a fix. They also point out a workaround using project.json instead of packages.config, as detailed here. |
Yes, I've been having lots and lots of discussions with them behind the scenes. Seems to be an issue with NuGet and the way WinJS projects use packages.config instead of project.json. I think we're fine to go ahead with publishing our updates. I don't think there's anything we can do on our end to help or work around it. I have this scheduled on my end to take a dependency on your library probably early next week. This week I have a commitment due for a video on the Maker Show and I also received my HoloLens on Friday so I've been busy with creating some communities around that. It looks like the WindowsRuntimeComponent version of UnitsNet is still pre-release on NuGet. When you get that updated to a non-beta release could you let me know? I will set some time aside early next week to get my library updated and depending on it. Thanks! |
Alright, I should be able to merge into master and release a stable nuget this week. A bit hectic with moving to a new house these days, but I don't think this part should take me long. |
@jbienzms #152 now merged, and 3.33.0 stable nuget released. |
Sounds good @anjdreas. I have to be in Austin for HoloLens on Monday and Tuesday, and I have to finish that IoT video up this Friday and next Wednsday. My goal is to have my library live with UnitsNet by end of next week. Sorry for the delay on my end. I'll post back here when it's done. |
No problem, thanks for the update. |
Just following up on this, did you get a chance to test the nuget in your app? |
I did not get to it last week but I did get to it today. :) I have not pushed an update to the NuGet package out yet but I have pushed an update to source control. You can check it out here: https://github.com/jbienzms/iot-devices The reason I haven't pushed a NuGet package yet is because I just published this video about the library on Channel 9 today as part of The Maker Show: Breaking out of the Loop If you get a chance, let me know what you think of the video. Since I don't have my Raspberry Pi with me I don't have the ability to test my changes and I don't want to push a package update without doing that testing. Especially with that video going live. I should have a chance to do some tests on Wednesday and should hopefully be comfortable pushing a package update then. I have a related question about a new unit of measure. I'll post that in another thread though to not confuse this one. |
Cool video, I actually learned a couple of new things such as DispatcherTimer and got a bit more familiar with the IoT library, which I haven't explored yet, so thanks! If you want some constructive criticism, here are a couple of first impressions:
We can continue this conversation elsewhere if you want. As for testing Units.NET, no rush. I'm just checking in and interested to see how it works out. |
@jbienzms Just curious if you got around to test the nuget in the app? |
Yes, I did get around to testing the NuGet. And as you report, it works very well for C# projects but is still having problems in JavaScript apps. Really there's nothing we can do about that. The VS team is aware of the issue and has a bug open for it. I have incorporated UnitsNet into my project and that is how it is committed to source control. Sadly, I still have not gotten around to releasing an updated NuGet package. It's on the TODO list. From your end, you can certainly consider this issue closed. I don't think there's anything else you can do on your end to make the process easier. Thank you again for your hard work and for supporting WinMD with your library! |
Very cool, thanks for the update! |
Just wanted to follow up with you @anjdreas and let you know that I FINALLY published an update to the NuGet package that includes the dependency. As of NuGet package 1.0.8, UnitsNet is referenced by the Microsoft.IoT.Devices library. I'm so very sorry that it took so long to get pushed up to NuGet. Other responsibilities had me pulled in different directions. Having said that, I'm super excited to see the Windows Runtime version has been downloaded over 900 times! Glad to see my library wasn't the only code to benefit from this work. Thank you again for your support, and keep up the great work on an excellent library! |
Thanks for letting us know. I'm glad to hear this piece of code is useful to you and that you are referencing it in the IoT nuget! Would you mind adding a short snippet for our README section? https://github.com/anjdreas/UnitsNet#who-are-using-this |
Sure @anjdreas. Here you go: Microsoft.IoT.Devices extends Windows IoT Core and makes it easier to support devices like sensors and displays. It provides event-driven access for many digital and analog devices and even provides specialized wrappers for devices like joystick, rotary encoder and graphics display. http://aka.ms/iotdevices (home page including docs) |
Thanks, can I link to your github username or would you prefer not to? |
Feel free to link to: http://aka.ms/iotdevices As for the Microsoft logo, I'm not sure about that. It was built on MS time but it was a personal project of passion, not something I was "assigned" to do. I wish I had a logo for the project. I'd gladly let you use that. |
That makes sense, thanks. I'll add your text and your links. |
I would like to use UnitsNet in a large Windows IoT Core Devices project I manage called Microsoft.IoT.Devices. However, when I attempt to use the types defined in UnitsNet as return values in my library I get the following error:
This is due to the type restrictions on WinRT components.
Obviously UnitsNet was written as a Class Library and not a WinRT component. And obviously refactoring the library into a WinRT component may require that some features be left off. However, I still feel this would be incredibly valuable to have, especially in the IoT space.
If you would like to discuss this more please feel free to reach out to me. com dot microsoft at jbienz, or via github.
The text was updated successfully, but these errors were encountered: