sccn / liblsl

C++ lsl library for multi-modal time-synched data transmission over the local network
Other
108 stars 63 forks source link

Don't build as a shared library if LSL_BUILD_STATIC is set #115

Closed chausner closed 3 years ago

chausner commented 3 years ago

Currently, enabling LSL_BUILD_STATIC allows to build LSL as a static library but the names of the generated libraries are different (-static suffix) and shared builds are still getting created. The latter is an issue for the vcpkg port because vcpkg poses quite strong restrictions on what should be part of the package and it does not allow shared libraries to be part of it when static builds are enabled. Also, simply deleting the shared library files does not work because there are references to it in the generated CMake target. For this reason, the vcpkg port only supports LSL as a shared library at the moment.

Also for Conan, builds are usually either static or shared and there is no real usecase for building them both at the same time.

My suggestion is to change the behavior of LSL_BUILD_STATIC to disable building shared libraries completely (or simply use the pre-defined BUILD_SHARED_LIBS variable in CMake). Then we also don't need the -static suffix and the CMakeList.txt should become simpler overall. It would allow me to support static builds in vcpkg and remove one of the hurdles in creating a Conan package.

tstenner commented 3 years ago

I'm somewhat torn on this one. On the one hand, I'm all for getting liblsl in distributions and vcpkg is pretty big on Windows. Building the shared and static library separately enables some optimization opportunities for static libraries, the CMakeLists could use some simplification and so far building liblsl and liblsl-static in one go hasn't been needed so far.

On the other hand, I see liblsl as mostly a shared lib, even for C++ projects. For some vendor provided apps it's only possible to get liblsl bugfixes incorporated by swapping out the liblsl shared library and after my limited success with questions regarding their LSL implementation with some vendors I'm not very optimistic that they'll link their app against another liblsl version. The only way to force that would be to re-license liblsl as LGPL but I'm told that's not likely. (Still, vendors using vcpkg to get liblsl will be more likely to keep liblsl up to date).

Paging @cboulay and @dmedine for further input.

dmedine commented 3 years ago

I for one am completely against the idea of chasing the tail of every new package manager and workflow solution. There is a fork button and if people really need this, that or the other thing, they can press it. There are always going to be hoops to jump through to build a C/C++ project that links to multiple libraries and I don't see why it is our task to jump through all of them rather than our users.

But, down to practical matters. May I ask what vendors are so opposed to updating shared libraries? After all, people (including tstenner) do it all the time with Qt and other popular libs. I know from personal experience that vendors are notoriously bad about letting people see their source code even if it is trivial and with the best of intentions ('I will fix your code if you show it to me!' always stops a conversation), but I don't see that that precludes updating shared libs for bugfixes.

(As a side rant, the whole idea of closed-source software is ridiculous. The value of software is in the distribution and support behind it. The actual code itself is typically worthless (in terms of being able to generate profits) without a team of experts behind it.)

Also, can you please explain why we would have to change the license? I know LGPL is suitable for shared libraries, but MIT allows people to do basically anything they want so long as they copy the license---including linking from differently licensed software.

I think that in general it is a good idea to separate the static and shared lib builds in CMake as far as possible---especially if it makes the CMakeLists files simpler. It also makes sense from an organizational point of view. 32 and 64-bit builds are separated and I see the difference between shared and static as even greater! I also agree with the OP that the '-static' suffix should be gotten rid of. This would be in keeping with the spirit of simplifying the LSL naming conventions and because static and shared libs have different file extensions anyway.

But, I also don't see much benefit in ever linking to a 3rd party static library unless it is exceedingly simple (which LSL is not) or if it is needed for a special case platform like mobile or MBed or whatever---in which cases much more special-case stuff like memory management and adjustments to async processes would have to happen anyway. I would be interested in knowing how many clients are ever using lsl-static.lib on full blown computers. My guess would be that it is nearly 0.

cboulay commented 3 years ago

If we hope to come to a conclusion then we should keep this conversation limited in scope.

I don't see why we can't have a config that builds the static lib without also building the shared lib. That seems reasonable.

I know in Windows the static output has extension .lib, and the shared has .dll AND .lib, so building both simultaneously requires a distinct naming convention. But if we never build both static and shared in the same config then this is a non-issue.

The only downside I see is that our CI runs will take ~10% longer because we will have to separately build and test static and shared configs but I think this is a fine sacrifice.

All that said, @chausner I don't have time to work on this change and I also don't have a vcpkg setup that will allow me to test if the product meets your criteria. If you can put together a PR that implements your suggestions (including changing the GH actions scripts) to your satisfaction then I think there's a pretty good chance we can merge it.

tstenner commented 3 years ago

I for one am completely against the idea of chasing the tail of every new package manager and workflow solution.

And app developers in academia and the industry are against taking more time and effort to get accustomed to yet another project's build and packaging system. In this case, it's just some good practices that don't cause any additional effort.

The actual code itself is typically worthless (in terms of being able to generate profits) without a team of experts behind it.

I've seen analysis packages that promise heaven and earth, yet in the source code it's a bunch of unadjusted t-Tests. Not giving out the source code is quite effective to stop users from realizing what the software is really doing, and there are other justifiable reasons.

Also, can you please explain why we would have to change the license?

If static linking were widespread and a problem, there'd be nothing we could do. With the LGPL license, vendors would be required to offer the liblsl source code and (compiled) object files that could be linked against a newer liblsl.

But, I also don't see much benefit in ever linking to a 3rd party static library unless it is exceedingly simple (which LSL is not) or if it is needed for a special case platform like mobile or MBed or whatever---in which cases much more special-case stuff like memory management and adjustments to async processes would have to happen anyway. I would be interested in knowing how many clients are ever using lsl-static.lib on full blown computers. My guess would be that it is nearly 0.

Some plugin architectures that produce shared libraries (e.g. Pybind11) would benefit from a static library, and if the native C++ ABI were to be resurrected it should only be in a static library.

The only downside I see is that our CI runs will take ~10% longer because we will have to separately build and test static and shared configs but I think this is a fine sacrifice.

As long as the C++ ABI isn't built there's no reason to build the static lib, so the CI times stay mostly the same. The plain static builds are tested with every version bump in the vcpkg / Conan packages.

I have pushed a PR (#116) that builds either a shared or a static lib. The internal tests still need to access non-exported symbols so the object library lslobj has to stay.

tstenner commented 3 years ago

Done in #116.