Restructure the wrapper

[tstenner]

In order of commits:

1. build examples for .NET Core 5.0 instead of the outdated .NET Core 2.0
2. put Dispose() logic in a base class extending SafeHandle for more exception safety in multithreaded contexts and a single place for Dispose() instead of in every class wrapping a C-API object
3. apply code style to examples so the next changes aren't cluttered by formatting changes
4. some functions return StreamInfo[] arrays, so I added a function to dispose those in a single line
5. use using and DisposeArray() in examples
6. build all examples as a single target so that build options (version, target framework etc.) can be adjusted in a single place. This changes the usage, e.g. dotnet -p examples\FooBar arg1 becomes dotnet -p examples FooBar arg1. Adding an example is a matter of dropping the .cs file in the examples folder, no need to add a new project.
7. as previously discussed, rename the liblsl class to LSL and move classes from this class into the root namespace
8. adjust the examples to reflect the changes in the previous commit
9. bump the assembly version to 2.0 and (for David) print the wrapper version in the example code

[dmedine]

I don't understand this at all. As far as I can tell, .NET Core only goes up as high as 3.1. .NET (without any suffix) goes to 5.0. It is possible that the dotnet command will revert to the right thing, but when I open the property pages in Visual Studio, the traget framework field is simply empty. In the previous version, it is .NET Core 2.0. I believe it should be .NET Core 3.1.

[dmedine]

Sorry, that wasn't clear. I thought Github would do this by on a commit by commit basis, but it publishes the comments for the entire PR (which is one reason I suggested not stacking commits in PRs once upon a time, now that I think about it). So, to clarify, .NET Core only goes as high as 3.1 (https://dotnet.microsoft.com/download/visual-studio-sdks?utm_source=getdotnetsdk&utm_medium=referral) and in visual studio, the properties for the examples project has an empty field for Target framework. I would guess that the dotnet command reverts to something reasonable behind the scenes. I am surprised there isn't a warning.

[dmedine]

Gee, thanks for including me! It is so nice to know that I get to contribute next to nothing after you shoot down everything I suggest and then scoop my contribution before I even have a chance to put any effort into it!

But to be less snarky, I must say that of all the github repositories I contribute to, this has become the most difficult, and mostly this is due to Tristan's attitude. I realize that I have not done much work here in the last few years and that yours, @tstenner, has been both persistent and excellent, but you need to understand that you do not own this project. This is a community effort irregardless how much work you personally contribute. Maybe I don't need to issue this admonishment, but it certainly feels that I do. I am always happy to work towards a consensus on topics before making decisions and I am more than happy to accept yours and other people's opinions when they differ from mine or when they are better informed, but you must be sympathetic to how frustrating it is when one tries to reach a consensus about a suggested change, is confronted with nothing but negative one-line comments, and then gets excluded from the actual development which I was trying to volunteer my time to do in the first place! This is not the first time I have tried to contribute something, have it met by a quick no from Tristan, then turn around to see him do it his way without consulting anyone about it.

With that out of the way, I have a few comments.

One is about the Framework targets and I have requested a change.

The other has to do with the reshuffling of the examples. This project doesn't build in Visual Studio now. I also think the examples are now less clear. At the very least, it needs to build in VS.

The last is about versioning. This is a bit tricky. With pylsl there is Chad's pattern of piggybacking on the lib version and adding patch numbers specific to the wrapper. I like that idea, but it doesn't work here because the changes to the wrapper break the API (again, I complain that Tristan is allowed to do this, but I am not---these changes will also wreak havoc with unity clients and the style is still bad!). This warrants a new Major bump, so 2.0 is good, but it breaks with the pattern established in pylsl. Personally, I think that having meaningful versions for each wrapper is more important than establishing a pattern across all the wrappers, so I am in support of this.

SafeHandle was a very good idea as it is the proper way to do this with InterOp objects, as opposed to IDisposable.

[dmedine]

The consolidate examples commit doesn't build in Visual Studio. When building the examples project I get an error for each attribute in each of the example program assembly info files is a duplicate:

I also think that this makes the examples a bit harder to understand. From a newbie point of view, when learning an API I find that it's nice to be able to simply run one, drop in a breakpoint and see what happens. Pushing them all under the same umbrella muddies the waters a bit. I would prefer keeping them as is.

[tstenner]

I don't understand this at all. As far as I can tell, .NET Core only goes up as high as 3.1. .NET (without any suffix) goes to 5.0. It is possible that the dotnet command will revert to the right thing, but when I open the property pages in Visual Studio, the traget framework field is simply empty. In the previous version, it is .NET Core 2.0. I believe it should be .NET Core 3.1.

You're right, that should be either .net5.0 or .netcoreapp3.1. Neither Visual Studio (2019 Preview) nor the dotnet CLI tool complained, but the project property page didn't accept it. It's fixed in d8a0661.

I must say that of all the github repositories I contribute to, this has become the most difficult, and mostly this is due to Tristan's attitude. I realize that I have not done much work here in the last few years [..]

I went back to the previous discussion and I have to admit that you're right, sorry. In hindsight, I mixed up some of your concerns about the wrapper version and names and didn't explain enough for me to recognize that the liblsl class had to be renamed anyway.

[..] then turn around to see him do it his way without consulting anyone about it.

To be fair, one of my older PRs I've been told I'd get some feedback I should incorporate is getting close to its second anniversary and for almost everything I let the corresponding PR sit around for a few weeks. Also, if it were up to me, the support for ancient MSVC version would be dropped several years earlier, but I've been working around those quirks anyway.

Some of the PRs (this one included) aren't meant as Pull Requests in the original sense but rather Discussions With Attached Code, but DWAC doesn't sound as good.

I wanted to rush this along as I'd like to merge the changes into LSL4Unity so I can use it for a project I'm currently working on. Still, I scooped some of the work you wanted to do, so before merging we can see how to add you to the commits for attribution.

The other has to do with the reshuffling of the examples. This project doesn't build in Visual Studio now. I also think the examples are now less clear. At the very least, it needs to build in VS.

The consolidate examples commit doesn't build in Visual Studio. When building the examples project I get an error for each attribute in each of the example program assembly info files is a duplicate:

I'm not entirely sure, but I think you have an older AssemblyInfo.cs in the folder. With all the goodness (project files you can write by hand, metadata included in the project setup) the new csproj format brings, the automated globbing of everything, even files not included by git and stuff in old build folders and the inability of dotnet clean to remove old build objects, is a constant source of errors like these. I just ran a clean build on a different computer, but without CI builds I can't be sure that it's a) failing because you had some extra files in the folder or b) working for me because I had some extra files not checked in.

Could you delete everything in the liblsl-Csharp folder and rebuild after running git checkout restructure?

The last is about versioning. This is a bit tricky. With pylsl there is Chad's pattern of piggybacking on the lib version and adding patch numbers specific to the wrapper.

This is tricky indeed and part of the whole DWAC, i.e. just a proposal for discussion and amendment. In general, there can be breaking changes in liblsl as well as the wrappers (like in this case), so the versions should be somewhat independent. 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.

I also think that this makes the examples a bit harder to understand. From a newbie point of view, when learning an API I find that it's nice to be able to simply run one, drop in a breakpoint and see what happens. Pushing them all under the same umbrella muddies the waters a bit. I would prefer keeping them as is.

I agree that it's not ideal. I can see three options:

1. Every example as separate project, with its own csproj file (how it was). Changing the version, target framework etc. has to be done for every subproject separately.
2. As before, but common attributes in a Directory.build.props. Changes in one place, but copying an example folder fails in mysterious ways.
3. Every example in a single file, but all in one project. Running an example takes one more step, but the run configuration has to be changed so the working directory points to a folder with lsl.dll in it:

I changed the name of the main function back to Main in the examples so it's possible to copy the .cs file to a new project and start from it immediately. One other QoL-improvement would be a prompt like "You've passed no example name on the command line. Please enter an example name or example number to run it: " so it's easier to start.

I prefer Nr. 3, but the commits to move from 1 to 3 are separate from all others so it's easier to drop them if there's a consensus to keep it as it was.

[dmedine]

Well I am sorry to have been so crabby about this and I appreciate your being big about it. And, for the record, I don't see much reason for supporting obscure ABIs and ancient compilers either.

Some of the PRs (this one included) aren't meant as Pull Requests in the original sense but rather Discussions With Attached Code, but DWAC doesn't sound as good.

Understood. Although I said I would, I never got around to proposing a standard workflow for these things anyway, so I shouldn't complain. I am justified to complain about github, though, which shouldn't put the 'review' button inside a commit diff page and then post the comment to the PR page. That is just bad UX.

I'm not entirely sure, but I think you have an older AssemblyInfo.cs in the folder....

I'm not entirely sure what happened either. Initially I did a clean pull of this submodule into its own working dir and checkedout this PR. The only thing I did before building was to change the Target framework in the properties (I'm in VS here) from empty to .NET Core 3.1. Error city. I did a clean pull of this PR again, did not change the Target framework and it worked---even before attempting to checkout your restructure branch. However, I don't understand why that would have caused all that assembly attribute collision nonsense. Also, I noticed VS built the examples for .NET 5.0, so I guess it is being cute and assumes that that is what you meant by netcore5.0.

I think your versioning proposal is very good, but I am not sure it makes sense until NuGet packages are implemented. Until then, underlying dlls could be swapped. It is probably pretty low risk to plow ahead with the system, though. At the end of the day, I only care about this for support's sake.

Re the examples, I still vote for option 1. There are not that many of them and we shouldn't have to update the target framework often. So for me, the simplicity of each project is worth the hassle of disunion. As I mentioned earlier, the wrapper itself could be slightly improved by using some newer features of C# that aren't supported in .NET Framework 2.0, but perhaps until that happens I guess it doesn't make sense to change the target.

[tstenner]

Well I am sorry to have been so crabby about this and I appreciate your being big about it.

Better crabby than crappy :-) I hadn't been all that cheery, so thanks for adressing it directly after you were rightfully frustrated.

Also, I noticed VS built the examples for .NET 5.0, so I guess it is being cute and assumes that that is what you meant by netcore5.0.

If I was MS, I wouldn't expect anyone to keep track of .NET names either.

I think your versioning proposal is very good, but I am not sure it makes sense until NuGet packages are implemented.

Even without NuGet packages there will be binaries floating around and the sooner they have a sane versioning schema the better. This should be a separate discussion, any idea in which repository this belongs? https://github.com/labstreaminglayer/labstreaminglayer?

Re the examples, I still vote for option 1. There are not that many of them and we shouldn't have to update the target framework often.

With the speed Microsoft has been releasing and deprecating .NET versions recently it has been too often for my taste and when I had to adjust it, it has in 70% of the cases been in a terminal session on another server that had only the latest .NET SDK installed so I had to edit 7 files in a text editor from the 90s. Not exactly what every end user has to face, but it makes it easier to test changes against another .NET version.

[dmedine]

With the speed Microsoft has been releasing and deprecating .NET versions recently it has been too often for my taste and when I had to adjust it, it has in 70% of the cases been in a terminal session on another server that had only the latest .NET SDK installed so I had to edit 7 files in a text editor from the 90s. Not exactly what every end user has to face, but it makes it easier to test changes against another .NET version.

Fair enough. I hadn't thought about the use case where people don't get to keep their machines up-to-date. However, shouldn't these different versions always be back compatible? I was under the impression that they were and all that InterOp stuff is fundamental Microsoft tech. At the end of the day, though, it is a small detail. I'm not that invested in this.

[tstenner]

However, shouldn't these different versions always be back compatible?

Yes and no. Apart from the minor incompatibilities, the major breaking changes on the source code level were between .Net Framework 3.5, .Net Framework 4.0 onwards and .Net Core. So far so good, but building requires the SDK / .Net Framework targeting pack for the specific version in the project configuration. I'll push this change to the examples in a separate DWAC so we can focus on the remaining changes.

[dmedine]

So we are locked in a legacy support loop yet again. C'est la vie.

[tstenner]

Let me go through the checklist:

• One project per example
• David gets fame and fortune for cleaning up the messy class/namespace situation in d004120 😉
• An additional launcher that compiles all example into one .exe that doesn't get noticed unless somebody opens the examples/LSLExamples.csproj directly
• All example .csproj files are identical so the .net version can be updated by changing one project file and running for f in *; do test -d $f && cp HandleMetaData/HandleMetaData.csproj $f/$f.csproj; done in the examples folder

[dmedine]

It all looks fine to me.

Just a note, I would guess that since most users are using this for Unity projects at the moment that targeting .NET 5.0 will be ok. However, there are still many Windows users out there using .NET <=4.x and (the wonderfully named) .NET 5.0 (which is the successor to .NET Core 3.1) will have to change the target. For cross platform development, the newest version is definitely the way to go. The examples shouldn't really matter so much, but the lib should remain back compatible (which it is).

So is the next step writing async versions of all the methods? An awaitable pull_sample_async would definitely be a useful thing to have. It might be best just leave it to users to write their own wrappers. It is a pretty straight forward pattern:

public async Task pull_sample_async(...)
{
     await Task.Run(()=>pull_sample(...));
}

but it is a pain to have to develop this in over and over.

Another thing on my wishlist would be to get rid of the c-style array handling. I feel that pull_sample/chunk should return an IReadOnlyList instead of void. These two things taken together could be a whole other wrapper.

[tstenner]

It all looks fine to me.

Thanks :-)

That leaves @cboulay to voice any objections or press the merge button.

Just a note, I would guess that since most users are using this for Unity projects at the moment that targeting .NET 5.0 will be ok. However, there are still many Windows users out there using .NET <=4.x and (the wonderfully named) .NET 5.0 (which is the successor to .NET Core 3.1) will have to change the target. For cross platform development, the newest version is definitely the way to go. The examples shouldn't really matter so much, but the lib should remain back compatible (which it is).

With the lib at .NET Standard 2.0 it can be used with anything since .NET Framework 4.6.1.
The examples should be what a fresh Visual Studio install defaults to, which is (afaik) .NET 5.0.

So is the next step writing async versions of all the methods?

Integrating that liblsl asio loop with the .NET loop will be… interesting. Async methods will make it a lot easier for users, but I'm not sure how to implement them efficiently.

Another thing on my wishlist would be to get rid of the c-style array handling. I feel that pull_sample/chunk should return an IReadOnlyList instead of void. These two things taken together could be a whole other wrapper.

It's not very ergonomic, but efficient. I think the best way to handle this would be a plain liblsl wrapper (basically, as it is now) and another wrapper that offers a nice interface.

[dmedine]

I don't think we need to worry too much about people wanting to target .NET framework < 4. If need be, they can switch the target in the LSL project. I don't see anything too contemporary in there.

For the ASIO thing, this is precisely what modern .NET is good at. Nearly all async methods call other async methods so I don't see liblsl using boost/asio underneath as a conflict---rather a compliment---to using async. It really is just as simple as wrapping a function in a Task.Run lambda.

To me it also makes sense to wrap pull_sample (and even more especially resolve_streams) into async methods because it is a very convenient way to kick off a method that handles samples as they come in in the background. When a C# method hits await, it returns control to the caller. Then, anything that happens after await will kick off automagically once the task is complete:

private async Task PullAndProcessAsync()
{
    await Task.Run(()=>pull_sample(...));
   // at this point the caller is back on
   ProcessNewData(...);
   // processing of data is happening outside of, say, GUI thread
}

Waiting on IO is exactly what async methods that return Task are meant for. CPU intensive tasks are meant to return Task. This also goes hand in hand with leaning on C# to handle allocating buffers and collecting garbage instead of the C-style array arguments for the output from pull_sample/chunk.

I am not sure if the best thing to do is to leave this library as it is and simply add a class that wraps everything in async methods, or just to roll a new wrapper. People are already using this one, but it is rather 2009.

[tstenner]

For the ASIO thing, this is precisely what modern .NET is good at.

Well, yes and no. Ideally, it would be integrated somehow so it would at most need one thread to run the asio event loop and return control to the task with available data. The Task approach works, but (as far as I can see, please correct me) still uses one OS thread per inlet.