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

Port to .net core #11

Closed
damtur opened this issue Jan 25, 2017 · 15 comments
Closed

Port to .net core #11

damtur opened this issue Jan 25, 2017 · 15 comments

Comments

@damtur
Copy link

damtur commented Jan 25, 2017

Hello,

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

Thanks
Damian

@jeffijoe
Copy link
Owner

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
Copy link
Author

damtur commented Jan 26, 2017

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

You may see dotnet/core#206 for some start

@jeffijoe
Copy link
Owner

jeffijoe commented Jan 26, 2017

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
Copy link
Owner

@ramziyassine hows it going? 😄

@apjones6
Copy link
Contributor

apjones6 commented Apr 23, 2017

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

apjones6 commented Apr 24, 2017

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
Copy link
Owner

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
Copy link
Owner

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

@apjones6
Copy link
Contributor

apjones6 commented Apr 24, 2017

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
Copy link
Owner

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 apjones6 mentioned this issue Apr 25, 2017
@apjones6
Copy link
Contributor

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
Copy link
Owner

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

image

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

No branches or pull requests

3 participants