jeffijoe / messageformat.net

ICU MessageFormat implementation for .NET.
MIT License
161 stars 23 forks source link

Port to .net core #11

Closed damtur closed 7 years ago

damtur commented 7 years ago

Hello,

are you think of porting that library to .net core?

Thanks Damian

jeffijoe commented 7 years ago

It's been a while since I have worked with .NET - is .NET Core not compatible with PCL's?

If you are up for it, you are more than welcome to attempt to port it and submit a PR. 😄

damtur commented 7 years ago

I think @ramziyassine Will look and see how hard will it be to port it.

You may see https://github.com/dotnet/core/issues/206 for some start

jeffijoe commented 7 years ago

Cool - the signing key for the assembly is in the repository as well, so to test it out you would have to pack it with nuget and install it locally to confirm it works. Please make sure it is still installable in plain .NET projects as a PCL as well.

Good luck, keep me updated! :shipit:

jeffijoe commented 7 years ago

@ramziyassine hows it going? 😄

apjones6 commented 7 years ago

I decided to have a quick go at moving this to .NET Standard (which is designed to run in all .NET runtimes). It doesn't seem to require any actual code changes; just project and dependencies stuff.

https://github.com/apjones6/messageformat.net/tree/net-standard

I've been able to create and install the NuGet package locally into a .NET core console app and run the first example code in the README without any issues.

EDIT1: I also created a portable library which was able to install and use my built package. I used a NET 4.5 console app referencing the portable project to run the code. I don't know what scenarios I might not be covering though.

EDIT2: Using http://nugettoolsdev.azurewebsites.net/4.0.0/framework-compatibility I think I've determined that my branch loses support for sl5 and wp8. I should get wp8 back if I can lower the netstandard version to 1.0 (1.1 was necessary for concurrent dictionary). It looks like sl5 isn't supported even for 1.0. I don't know the appropriate action to take for that at the moment.

I'm using Visual Studio 2017 (community edition) and needed to add <GenerateAssemblyInfo>False</GenerateAssemblyInfo> to avoid duplicate assembly attributes with the PropertyInfo.cs files. Alternately this could be moved into the csproj file, but I'm not sure how it affects whether we can use a nuspec file.

I've left the versions on 1.1.0 for now, but I needed to remove the * for the revision number. As this wasn't included in the NuGet package versions I assume this isn't important.

I updated the included NuGet.exe and removed the related files. As I understand they relate to package restore in VS, which doesn't seem to apply to VS2017.

P.S. I should note that while I'm very interested in this strategy of formatting text, I've not yet used either this library or others! Just thought it's worth being clear I'm not (yet) familiar with the code, so there's an extra risk of me missing something.

jeffijoe commented 7 years ago

Thanks for this effort @apjones6!

The * in the version string is for MSBuild to auto-increment it on each build, why did you need to remove it? Did it refuse to build?

Losing SL5 and WP8 isn't that big of a deal, if anything we could do a major version bump so SL5 and WP8 developers (god bless their soul) can still use v1.x. As long as it can be used in Xamarin iOS, Android, Windows 10 and now hopefully cross-platform with Standard, I think that's pretty good coverage.

apjones6 commented 7 years ago

Yes it was giving me a compile error about it not being valid. It's possible there's a better resolution, or something else was a root cause, so I'll have another look this evening after work.

I was working on targetting multiple frameworks after my comment last night but having problems that it didn't seem to use the reference to the portable assembly (it was referenced with a condition for the target framework value). I'll have another go at that tonight, but there doesn't seem to be much documentation about multiple target frameworks in vs2017 yet.

jeffijoe commented 7 years ago

I pulled your fork and verified it runs by referencing the project from a .net-core xUnit test on macOS:

image

However it seems assembly signing does not work, at least not when building on macOS, could you provide me a built NuGet package I can try to add?

apjones6 commented 7 years ago

I'll have to do that when I get home (about 9-10 hours from now), but yes I can.

I haven't worked with signed assemblies before, so it's quite possible I've done something wrong. I simply added the reference to the snk file in the vs2017 project, and built using .\.nuget\NuGet.exe pack MessageFormat.nuspec.

EDIT: I had to zip it to attach (I could have renamed but thought it might be more confusing) MessageFormat.1.1.0.nupkg.zip

jeffijoe commented 7 years ago

No you didnt do anything wrong, the snk is for signing the assembly, it was requested in #3 but it seems the tooling for macOS does not support signing builds, question is if it supports consuming signed assemblies.

jeffijoe commented 7 years ago

Also just verified it works on iOS - fantastic! :shipit:

apjones6 commented 7 years ago

The nupkg as requested: MessageFormat.1.1.0.nupkg.zip

I just had a brief go hoping to fix the multiple targeting with <PackageTargetFallback ... /> but no luck as yet. I might post on stack overflow; I'm probably missing something fairly minor.

EDIT: I think I'm basically coming to the conclusion it isn't possible to target all these frameworks in vs2017. It may be possible in 2015 annoyingly (I've not used it; still on 2012 in work). This page is a hint, and when I create a portable library in vs2017 it doesn't seem to be in 2017 format, so I suspect we can't add netstandard as an output of it.

As far as I understand it, it's just wp8 and sl5 we're losing. If that nupkg is okay then I think we'd be ready to open a pull request?

jeffijoe commented 7 years ago

Perfect, just tested that nupkg and it works!

image

Yeah go ahead and do a PR, I'll do a build when I'm back on my Windows PC and release it.

apjones6 commented 7 years ago

The tests failed on the pull request. I'm assuming this is because it's attempting to build using MSBuild 14.0, and vs2017 would be 15.0.

Is this something I need to resolve in the repository, or is it externally managed?

jeffijoe commented 7 years ago

Released as 2.0.0 - thanks @apjones6 for getting this done!

image