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

Add support for nuget package generation #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

glopesdev
Copy link

@glopesdev glopesdev commented Jun 15, 2021

Fixes #2

This PR expands support for nuget package generation including common metadata such as projectURL, package license and package tags. A README.md file has been included in the runtime identifier folder to explain how to correctly build the package.

At the moment, assemblies in the runtimes folder need to be resolved manually by calling SetLibraryPath or using the deps.json file when publishing a .NET Core app. Nevertheless t are resolved automatically by both .NET Core and .NET Framework according to the relevant runtime identifier, as long as the package architecture-specific folders are correctly declared. This package follows the foreseeable standards for platform-specific native dependencies using runtime identifiers going forward from .NET 5.0 as documented in RID Catalog and in the section on architecture-specific folders.

@dmedine
Copy link
Contributor

dmedine commented Jun 16, 2021

Hi. I tried to test this and I failed. I may have done it wrong, though. Here were my steps:

  1. clone and build a fresh lsl.dll (x64/Release)
  2. clone your fork of this repo and checkout the issue-2 branch
  3. copy my fresh lsl.dll into the 'runtimes' folder
  4. build liblsl (the csharp one)
  5. pack liblsl
  6. using the nuget package manager, I made a new package source from the pack target directory <path_to_issue-2>\liblsl-Csharp\bin\Release---this is where my freshly packed lsl_csharp.2.0.0.nupkg is
  7. install lsl_csharp using the nuget package manager
  8. see little upside down yield sign warning icons on the project Dependencies tree
  9. in visual studio, set SendData as the startup project
  10. build and run
  11. read System.DLLNotFoundException message that lsl is not found

It seems like I am missing something (like a call to SetDllPath or some .json nonsense).

I also tried giving lsl.dll the copy always property. This copies it into a folder called runtimes in the bin/netstandard2.0 folder but this does not help.

Then I tried adding lsl.dll to the liblsl project and giving it 'copy always'. Now the nupkg works, but I still get the yellow warning tags on my dependencies, which is bad.

@dmedine
Copy link
Contributor

dmedine commented Jun 16, 2021

Finally, just looking ahead a bit, I would say that once this whole process is fixed up, we need to do some adjustment to the package names and other meta-data. 'lsl_csharp' is probably not the best name for the package. Obviously it is .NET if it is on NuGet. I think just 'LSL' is correct. I also think that due to the contributions of others (especially @tstenner and also yourself) that listing Christian Kothe as the sole author is not appropriate anymore. Then I think it would be good to CI this thing and upload it to nuget.org automatically every time we need to update it.

@glopesdev
Copy link
Author

glopesdev commented Jun 16, 2021

@dmedine thanks for giving it a try! I would be curious to hear more about the warnings you got when installing the package, do you have any error message text you could share?

Regarding native binary resolution, there is indeed not a good unified story yet for how they should be handled.

For the "old" .NET framework, what me and several others have used in the past is to include .props and .targets script files in the build folder to automatically copy binary dependencies to the output directly. Dependencies themselves would be stored in the build\native folder. This worked really well with NuGet and didn't require to change anything on the p/invoke bindings layer.

However, there was no convention for identifying native platforms besides the native folder and it didn't work very well with separating library from application layer (i.e. you want to keep libraries as platform agnostic as possible and push the platform specific resolution to applications). The current official approach to address this is to use reference assemblies in .NET Core / .NET 5.0.

In this new world the ref folder stores platform agnostic assemblies which are used exclusively for build purposes (e.g. libraries) and the runtimes folder keeps the platform specific implementation including native dependencies. At runtime dotnet/ASP.NET figure out the current host plaform and automagically load a matching implementation assembly from the runtimes folder.

The story is complicated outside ASP.NET land however as there is no host that can automatically load up the dependencies. This has led to confusion from library authors which end up hacking different ways to do the runtime resolution. Some of them use LoadLibrary and search for the native dependencies in 20 different possible locations, others expect dependencies to magically be placed in specific subdirectories from the executable folder and include build scripts to copy files there.

I personally quite dislike these options as it makes it really easy to end up with different incompatible strategies for doing native dependency loading (some of them even incompatible with different underlying OSes) which compounds the DLL hell problem further.

My current inclination then is to keep things as simple as possible, using all of the agreed upon standards while avoiding any premature decisions on things which are not standard. The p/invoke level already uses the underlying library resolution mechanism of the host OS, so no need to reinvent that wheel and try to reimplement complicated search strategies. Of course, we need a way to make sure the native dependencies included in the package end up in at least one of the places which gets scanned.

Currently I know how to do this with build scripts for .NET framework but really need to study the story better for .NET Core.

This PR was meant as a way to start this conversation for liblsl which I will need to find a solution for in the short/medium-term. .NET 6.0 is also around the corner with more insights on the p/invoke story.

I will investigate a bit further and report back more details in this thread or the original issue. I totally agree with your thoughts regarding package name, but kept it to avoid changing the existing project conventions right away.

@glopesdev
Copy link
Author

Regarding copyright its a complex story. You are totally correct regarding contributions, but tracking these things can get messy. As a rule of thumb I assume that if the original author picked the MIT license then their intention is to keep the legal struggles to a minimum and simply provide an umbrella for contributions. The other extreme is GNU and hybrid model is Apache or a CLA (contributor license agreement). We can have a contributors file and change the copyright to "and contributors" but maybe I would raise a new specific issue for this?

@dmedine
Copy link
Contributor

dmedine commented Jun 16, 2021

We can have a contributors file and change the copyright to "and contributors" but maybe I would raise a new specific issue for this?

Hmm. I hadn't thought that there would be legal ramifications to extend the authors list. I just wanted to acknowledge some of the more recent contributions. I don't think anyone is going to cry if they don't see their name on the package, though ;-).

In any case, the license should also be rolled into the package... but let's deal with the cart before the horse.

@dmedine
Copy link
Contributor

dmedine commented Jun 16, 2021

@dmedine thanks for giving it a try! I would be curious to hear more about the warnings you got when installing the package, do you have any error message text you could share?

No messages, just the little warning badges
scrnsht

@dmedine
Copy link
Contributor

dmedine commented Jun 16, 2021

Sounds like Microsoft has not dealt with this very well. This has been my experience with other projects that rely on native code in C-sharp as well---even ones published published by Microsoft!

It is possible, of course, to insert xml inside the .csproj file that organizes all the dependencies properly (e.g. https://github.com/microsoft/onnxruntime/blob/master/csharp/sample/Microsoft.ML.OnnxRuntime.InferenceSample/Microsoft.ML.OnnxRuntime.InferenceSample.csproj), although Visual Studio doesn't seem to have any features that let you do this apart from by hand. This doesn't help much, though, since there is no way to inject these kinds of instructions into a client application .csproj file from a NuGet install.

Then again, this stuff is for developers. No one would ever expect a C++ developer not to have to provide include and linker directives when using building against 3rd party library. I want it to be as 'easy' as pip, but maybe it just isn't. That onnxruntime library I pointed out above, for example, is on NuGet, but you still have to find and copy the underlying native dll for it to work in your code.

@glopesdev
Copy link
Author

Actually, you can include MSBuild snippets in a NuGet package which will be included as if they are part of the final build project using the .props and .targets files (.props gets added at the beginning of the csproj file, and the .targets snippets get added at the end).

So in theory you could do an automatic deployment script which is correctly executed at build time. However, this is the old way of deploying a build in .NET framework, which doesn't seem to be the way that the new .NET Core / ASP.NET Core operate, where its the runtime, rather than the build, which is responsible for locating these dependencies (unless you are building an AoT project).

I'll experiment a bit more and update this PR accordingly.

@tstenner
Copy link
Contributor

Thanks for tackling this problem :-)

Due to the current situation I don't have a PC with all the .Net SDKs and Visual Studio at hand, but I'll give my two cents anyway:

CI builds are trivial, I've already added a commit to build liblsl-Csharp and the examples. The hard problem is 1) putting the native library somewhere 2) loading the correct library at runtime, and 3) work across .Net versions.
I'm certainly missing something, but I'm currently leaning towards not including the native library in the NuGet package.

NuGet packages are zip-files with a control file, so we can create them alongside the regular liblsl-builds and users will then have to depend on liblsl_csharp (or whatever better name you find) and e.g. liblsl.win.x86_64.

We can have a contributors file and change the copyright to "and contributors" but maybe I would raise a new specific issue for this?

From my understanding that's perfectly fine, but I'd be thankful if you could open a separate issue.

Regarding copyright its a complex story.

I guess the LGPL license would make it a lot more simple: everyone's free to use liblsl_csharp as long as they provide the source code for liblsl_csharp upon request. Contributions are derivative works, so they are automatically under the LGPL.

With the MIT license, I'd assume that contributors add their name to the license file / authors tag in the project file if they regarded their contributions as substantial or just overlook it otherwise. To be sure, we should ask previous contributors.

Sounds like Microsoft has not dealt with this very well. This has been my experience with other projects that rely on native code in C-sharp as well---even ones published published by Microsoft!

You wouldn't have this problem if you didn't stray from the Right Path, i.e. pure C# or the occasional Windows-only DLL 😉

This PR was meant as a way to start this conversation for liblsl which I will need to find a solution for in the short/medium-term. .NET 6.0 is also around the corner with more insights on the p/invoke story.

It's a good start, and if we were to merge your PR and publish NuGet packages without the native library included it'd be better than the status quo and with new all-batteries-included releases we would only improve the situation.

@glopesdev
Copy link
Author

glopesdev commented Jun 16, 2021

Thanks for your feedback. Diving deeper I am happy to say I can now give concrete answers to all of the above and if you test drive the latest commit following the folder structure outlined in the README file it should work out of the box.

  1. putting the native library somewhere

You have to lay out the native dependencies exactly like so:

\runtimes
    \linux-x64
        \native
            liblsl.so
            liblsl.so.1.14.0
    \osx-x64
        \native
            liblsl.dylib
            liblsl.1.14.0.dylib
    \win-x64
        \native
            lsl.dll
    \win-x86
        \native
            lsl.dll

Documentation is extremely hard to find, but it exists

Basically the thing that kept tripping me is the extra native folder which has to come under the architecture-specific runtime identifier. This NuGet issue on packing platform specific native assemblies was extremely informative.

  1. loading the correct library at runtime

It turns out the compiler will copy the dependencies to the right place and resolve them automatically if the above layout is followed.

On .NET Core the runtime knows how to resolve dependencies straight from the runtimes folder, so everything just works cross-platform (!)

On .NET Framework each build can only support a specific architecture which can be specified using the RuntimeIdentifier element, like so:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net472</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="lsl_csharp" Version="2.0.0" />
  </ItemGroup>

</Project>

This will make the compiler copy specifically the 64-bit native binary to the project. By default, if no RuntimeIdentifier is specified, the compiler will default to win7-x86 which has a fallback for win-x86 so a native binary is still copied.

For completion, specifying a RuntimeIdentifier is also possible in .NET Core, if you don't want a cross-platform build.

  1. work across .Net versions

It works across both .NET Core and .NET Framework without changing any code 🥳

Regarding license, I would say in this case sticking with MIT is much easier, especially with a single-file project where any derived works can simply place the copy of the license in LSL.cs. Also it follows naturally from the license for liblsl itself.

@dmedine
Copy link
Contributor

dmedine commented Jun 17, 2021

I guess the LGPL license would make it a lot more simple: everyone's free to use liblsl_csharp as long as they provide the source code for liblsl_csharp upon request. Contributions are derivative works, so they are automatically under the LGPL.

Regarding license, I would say in this case sticking with MIT is much easier, especially with a single-file project where any derived works can simply place the copy of the license in LSL.cs. Also it follows naturally from the license for liblsl itself.

For the record I am very, very, very opposed to changing the license from MIT to LGPL. I can't think any scenario in which a commercial software producer would be willing to use an open source library if it was licensed under LGPL. I think it is also a potential legal vulnerability to switch the license since so many people are already using this library. Many of them are in research, so it is unlikely to bite us (or, more specifically, Christian), but better safe than sorry.

I am in between computers at the moment and the new one (which is where I am set up to test this right now) is at home. I will play with the new commit tomorrow and get back.

Actually, you can include MSBuild snippets in a NuGet package which will be included as if they are part of the final build project using the .props and .targets files (.props gets added at the beginning of the csproj file, and the .targets snippets get added at the end).

That is very interesting! I was looking for some clues about this yesterday and couldn't find much. Can you point me to some examples/docs about how to do this? I keep finding hints on MSDN, but we all know how spotty those pages are.

@glopesdev
Copy link
Author

glopesdev commented Jun 17, 2021

@dmedine of course, I use this technique all the time! The only requirement is to include the .props and .targets file in the build folder of the NuGet package with relevant subdirectory depending on your target platform (e.g. net472, netstandard2.0, etc).

Relevant documentation is here: https://docs.microsoft.com/en-us/nuget/create-packages/creating-a-package#include-msbuild-props-and-targets-in-a-package

@dmedine
Copy link
Contributor

dmedine commented Jun 17, 2021

@dmedine
Copy link
Contributor

dmedine commented Jun 18, 2021

Nope.

Still doesn't work.

I put the native .dll in the correct folder, I copied the .props and .targets files from obj to the correct target directory for lsl_csharp, and I added the RunTimeIdentifier tag into SendData.csproj. But when I run the build, it still doesn't have access to lsl.dll and throws the System.DllNotFoundException as soon as the code tries to use it. Here is a screenshot of my setup. Maybe you can see something I've missed

fail

@glopesdev
Copy link
Author

Hmm, it looks like in this example you still have a ProjectReference to the original project. Can you try removing that and adding just the package reference?

Also in this case you don't need to add any .props or .targets files, the package should already have everything that is necessary.

If it still doesn't work, I can package a working example in a zip file to see if you can reproduce a successful build.

@dmedine
Copy link
Contributor

dmedine commented Jun 18, 2021

Ah yes, that seems to have done the trick!

I would propose the following

  1. approve and merge this PR

  2. open a new issue to discuss the following:

  3. update the README in the repository to reflect the new 'correct' way of handling the lsl.dll/.so/.dylib dependency

  4. get CI/CD running to handle building all the needed NuGet packages and pushing them to nuget.org

  5. replace the ../../liblsl.csproj references with references to the online nuget packages

  6. also include instructions for building and installing the nuget package locally in the top level README (or some other document)

@tstenner
Copy link
Contributor

Great work 👍

It turns out the compiler will copy the dependencies to the right place and resolve them automatically if the above layout is followed.

The compiler, nuget or MSBuild? As long as it works I mostly don't care, but when it doesn't it'd be good to know which tool to blame.

On .NET Framework each build can only support a specific architecture which can be specified using the RuntimeIdentifier element

.NET Framework is Windows-only (from a practical perspective) and Unity doesn't use nuget packages, so I don't this this is a big deal.

I can't think any scenario in which a commercial software producer would be willing to use an open source library if it was licensed under LGPL

I can think of at least one: they don't link against a static library and have someone who will sent out the source code for this specific library on request. The Qt guys would like everyone to think that commercial usage without source disclosure isn't allowed, but that's one of the major selling (pun intended) points of the LGPL compared to the regular GPL.

Well, back to topic:

Do I understand it correctly that one nuget package has to contain the native libraries for all target platforms? It it is so, I'd rather have the C# wrapper depend on a separate package with the binaries so it's easier to add platforms (e.g. android and all the arm family members) without changing the wrapper package.

@dmedine
Copy link
Contributor

dmedine commented Jun 22, 2021

I can think of at least one: they don't link against a static library and have someone who will sent out the source code for this specific library on request. The Qt guys would like everyone to think that commercial usage without source disclosure isn't allowed, but that's one of the major selling (pun intended) points of the LGPL compared to the regular GPL.

Indeed this is what LGPL is meant for; and, you are right there are circumstances where it makes sense. But, in my experience, vendors are loath to relying on open source producers doing this for them. The nice thing about MIT is that you can simply set it and forget it. Otherwise, vendors will prefer to pay a premium for garaunteed stability and support. There are exceptions. FFTW is notable and there are probably others.

The compiler, nuget or MSBuild? As long as it works I mostly don't care, but when it doesn't it'd be good to know which tool to blame.

I believe the answer to this is MSBuild. It is then our task to make the nuget package glom-on-able enough for MSBuild to be able to get it right. Please correct me if I am wrong.

@dmedine
Copy link
Contributor

dmedine commented Jun 22, 2021

Do I understand it correctly that one nuget package has to contain the native libraries for all target platforms? It it is so, I'd rather have the C# wrapper depend on a separate package with the binaries so it's easier to add platforms (e.g. android and all the arm family members) without changing the wrapper package.

I am not clear on this point either. I am not sure, though, if it is right to have 2 packages or simply add support for new targets piecemeal. 2 packages seems hard for the client, but bumping a versions just for a new target support seems wrong to me. Maybe there is another strategy for this in dotnet land that I don't know about---but I kind of doubt it :/

@tstenner
Copy link
Contributor

But, in my experience, vendors are loath to relying on open source producers doing this for them.

Sony has a page where they put the source code for all the libraries they are using and from what I've understood it'd be enough to push the "fork" button once and link to the forked repository. But you're right that any licensing requirements are hard to sell in corporate environments.

I am not clear on this point either. I am not sure, though, if it is right to have 2 packages or simply add support for new targets piecemeal. 2 packages seems hard for the client, but bumping a versions just for a new target support seems wrong to me.

Other package managers (apt, conda, rpm, …) offer a way to say "Package A depends on foobar" and one of several packages providing "foobar" can be used to resolve the dependency. Other package managers allow dependencies like "Package A depends on PackageB>=1.2 | PackageC>=1.4". As far as I can see, Nuget doesn't allow this.

Still, "liblsl_wrapper x.y" could depend on "liblsl_native x.y" and additions to the native package could then be pushed as "liblsl_native x.y.2". Question for the nuget experts: is it sufficient to refresh the nuget packages so the updates to transient dependencies are pulled?

One thing I'd like to see is a way to tell "I want to use liblsl_wrapper, but I have my own lsl.dll/.so/.dylib". Is there an attribute (e.g. <PackageReference Include="liblsl_csharp" Version="1.2" IgnoreDependencies="absolutely"/>) to ignore dependencies? I guess in this package the reference could be conditional <PackageReference Include="liblsl_native" Condition="'$(liblsl_skip_native)' == 'true'"/>) and the including project could set liblsl_skip_native, but does this work with nuget?

@dmedine
Copy link
Contributor

dmedine commented Jun 23, 2021

One thing I'd like to see is a way to tell "I want to use liblsl_wrapper, but I have my own lsl.dll/.so/.dylib". Is there an attribute (e.g. ) to ignore dependencies? I guess in this package the reference could be conditional ) and the including project could set liblsl_skip_native, but does this work with nuget?

That is actually an excellent point that I hadn't considered. However, I think the answer is that people who want to link against some custom/non-standard lsl.dll/so/dylib are going to have to roll their own. I think that it is reasonable to expect people that want to do such a thing don't need NuGet---especially since we have pretty clear build instructions.

Other package managers allow dependencies like "Package A depends on PackageB>=1.2 | PackageC>=1.4". As far as I can see, Nuget doesn't allow this

That, as they say, is the rub. I fear we are still no closer to resolving this. Actually, I have been using Microsoft's onnxruntime library for some projects and you can get the C# wrapper on NuGet no problem, but your code will not run until you right-click on the project, change the extension to 'all .', etc. just as we suggest with adding lsl.dll to a Visual Studio project. On the other hand, that is a pretty Linux-y development project---even though it is from Microsoft---so what do they know?

One idea occurred to me and that is to issue one just with the standard Windows/Linux/Mac shared libs and then for Android or other more exotic platforms have a LSL-Android/whatever package. I believe there is some precedent for that on NuGet.

@tstenner
Copy link
Contributor

If we were to somehow solve the "add a reference to liblsl-wrapper but ignore the liblsl-native dependency"-problem, the wrapper could depend on a native package with the most common libraries and users on exotic platforms could just depend on liblsl-wrapper-without-native-libs and liblsl-native-android-arm8.

@glopesdev
Copy link
Author

@dmedine @tstenner I believe you can do this already at package import time when you specify which assets to include or exclude from each package (see here).

For example, to exclude the built-in native dependencies, it is enough to do the following:

  <ItemGroup>
    <PackageReference Include="lsl_csharp" Version="2.0.0">
      <ExcludeAssets>native</ExcludeAssets>
    </PackageReference>
  </ItemGroup>

@tstenner
Copy link
Contributor

I believe you can do this already at package import time when you specify which assets to include or exclude

That would solve the problem with exotic targets, but those users would still download several megabytes of irrelevant binaries. I guess we can just release a batteries-included version of the wrapper and then later remove the libraries and depend on a separate liblsl-native package.

Regarding the versioning, I have already suggested something in a different PR:

On the other hand, I want to avoid compatibility lists where wrapper XY 2.1.4-2.1.8 supports liblsl 1.14.1-1.14.5 and so on. As Chad noted, we don't have an 'official' release so we can't break anything officially released, but that's something that needs a solution not just in this case but also for future releases.

My quick and dirty proposal: liblsl is currently at 1.14.0, so we could version the wrapper like X.1.14.Y, where X gets incremented for breaking wrapper changes and Y for non-breaking wrapper changes. 1.14 corresponds to the minimum supported liblsl version, so it conforms to semantic versioning, allows breaking changes in wrapper (like this one) and it's more or less plainly visible which liblsl version is needed, even with something like 6.1.18.3. Again, just a proposal and I'd be happy to get your input on it.

@glopesdev
Copy link
Author

@tstenner I like the suggestion regarding versioning, it is useful to be able to map the wrapper version directly to the base library version

I agree it would not be a problem to have two packages, one with the common native targets, and another one without. I would probably start with the package with batteries-included, since advanced users can still just copy LSL.cs to their project anyway.

Regarding project name, is there a conclusion? To avoid name clashes I like to use the .NET suffix to indicate the wrapper (I prefer it to csharp, since technically you are able to use it with VB or managed C++), here are some suggestions:

  • liblsl.net
  • Lsl.Net
  • LSL.Net

The middle one keeps acronym capitalization following .NET convention. The first and last ones break this, but they have enough of their own justification to be picked too.

@tstenner
Copy link
Contributor

LSL.Net

This one 👍

@myd7349
Copy link
Contributor

myd7349 commented Sep 10, 2024

Hello everyone!

I recently wrote a C# wrapper for liblsl and plan to submit some PRs to liblsl-csharp.

Currently, SharpLSL has implemented packaging for liblsl native libraries and supports .NET Framework 3.5+ and .NET 6.0+. You can check it out here: https://github.com/myd7349/SharpLSL/tree/main/Source/SharpLSL.Natives .

I'm thinking that once this PR is merged, I might be able to further improve the NuGet packaging based on it.

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

Successfully merging this pull request may close these issues.

Nuget Package
4 participants