search

bp74 / Zstandard.Net

A Zstandard wrapper for .Net
Other
135 stars 26 forks source link

Linux and macOS Support for NETStandard 2.0 Builds #7

Open simplexidev opened 5 years ago

simplexidev commented 5 years ago

This uses NativeLibraryLoader to load the library based on specified file paths for each OS.

This should close #6, once the generic binaries for Linux and macOS are built (which I'm leaving to @bp74, or others).

bp74 commented 5 years ago

Thanks for the pull request. Does this load the zstd library from linux or do we still need to add native libraries to this nuget package?

simplexidev commented 5 years ago

You still need the native binaries for zstdlib (.so/.dylib). I can't build the macOS library due to machine constraints, and I don't know the build process for zstdlib, so I'm leaving that up to you or another contributor. (Sorry!)

simplexidev commented 5 years ago

Correction, it should look for the library also, if you provide the proper file name (.so vs .so.0)

jakubsuchybio commented 5 years ago

@bp74 Why not merge this? Seems like a correct fix.

ChrisMcKee commented 5 years ago

@jakubsuchybio @bp74 there's an issue with the lib loader https://github.com/mellinoe/nativelibraryloader/issues/2 linked to https://github.com/dotnet/corefx/issues/32015 (which replaced a trove of others)

I fixed up the branch, adding in the 1.3.8 binary for linux (and dropped net4.5 out of irritation) https://github.com/ChrisMcKee/Zstandard.Net/tree/cmpatch but the lib loader fails on debian based (stretch/bionic) images as well as alpine due to the multi-located libdl being in different places between releases.

I've got around that issue with manual fallbacks (could be cleaned to use a custom https://github.com/mellinoe/nativelibraryloader/blob/master/NativeLibraryLoader/PathResolver.cs); copying the lib file to the 'expected location' on debian instances; and installing 1.3.3 on the alpine instance (copying the lib in on alpine caused errors)

The tests pass on my WSL, and in current Debian / Ubuntu and Alpine images as long as the requirements are setup.

One of the first thing NativeLibLoader attempts is loading the library from AppContext.BaseDirectory + name; the AppContext.BaseDirectory returns the correct response when ran from the sln folder on alpine as with the others so it's a tad perplexing 😬

In the case of alpine the library generated throws a System.PlatformNotSupportedException so alternative compilation would be required (or utilising the apk package) The problem herein lies with knowing you're in alpine not just "this is linux".

image

The least 'shitty' way of dealing with this afaik is simply to create work for the user image

Which means all of the envs work out of the box without anything beyond libload being added.

image

simplexidev commented 5 years ago

Thanks @ChrisMcKee for picking up my slack, sorry about that. 🙂 ~Are you going to be submitting your own PR, or applying the changes to this one? I'll close this one if you're submitting a PR from your branch.~

EDIT: I noticed that you added to the commits and renamed the commit messages. Probably easier if you submit a separate pull.

ChrisMcKee commented 5 years ago

Tbh I'm not sure the solutions 'ideal' it just works. I've also ditched all the .net45 stuff; if you're running on a computer that can't run 4.7 you've bigger issues than compression 🤣 So its more opening up the possible options to @bp74 as it's his library after all

simplexidev commented 5 years ago

I've also ditched all the .net45 stuff;

I never really looked to see what your changes did, so I missed that too.

if you're running on a computer that can't run 4.7 you've bigger issues than compression

I 100% agree with that.

And I will work on this a bit more then, since it was never really finished (and i forgot all about this PR 🤦‍♂️).

ChrisMcKee commented 5 years ago

Feel free to pull in my branch https://github.com/bp74/Zstandard.Net/pull/14 Shoved it on a pr to simplify rebasing