sccn / liblsl

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

Default liblsl search path: build folder vs. install folder #64

Open tstenner opened 4 years ago

tstenner commented 4 years ago

In find_package(LSL, CMake searches in the path set in LSL_INSTALL_ROOT first and then some app-defined paths.

I prefer the liblsl build path, but an alternative is the install path.

The build path has several advantages:

cboulay commented 4 years ago
  1. Then we should rename the variable to LSL_BUILD_ROOT or something similar.
  2. Are there any differences at all between the build output and what gets uploaded to the release page? Are all the RPATHs the same? I'd hate to get into a situation where the locally built liblsl "works for me" but the liblsl that we're asking people to download doesn't work.
tstenner commented 4 years ago
  1. The name INSTALL_ROOT can stay, it just says "look in LSL_INSTALL_ROOT first and if it's empty or doesn't contain the cmake config, try these default build paths"

  2. Excellent point! The most important rpath is the one in the executable linked to liblsl and that has to be set / changed in the install target. liblsl also has an rpath, but if doesn't link to (CMake) local libraries

cboulay commented 4 years ago

IIRC, the .deb files on the liblsl release page will install liblsl to /usr/local. Sorry I can't check right now, but does this install also have LSLCMake.cmake? If so, then shouldn't /usr/local be in the list of default search folders to find liblsl?

Since you are talking about making it convenient for faster iteration while developing, what about including Debug vs Release in the list of default folders?

Are we getting to too many folders in the search list? Maybe Findliblsl.cmake was the correct approach after all?

cboulay commented 4 years ago

I'm sorry but I'm just not convinced that the build output is similar enough to the install output on all platforms and configurations that we can ignore testing the install output and that this will be future proof. Therefore I'd like to make sure we always check our apps against the install output, and that users who have the full labstreaminglayer build tree are doing the same thing.

When you or I are doing work that requires the advantages of using the build path, then we can supply LSL_INSTALL_ROOT directly. It's much easier to ask us to add 1 or 2 steps than it is to debug a user's problem that has to do with a subtle difference between build and install.

tstenner commented 4 years ago

IIRC, the .deb files on the liblsl release page will install liblsl to /usr/local.

The default CMake install folder is /usr/local, but the debian packages install into the system tree:

$ sudo dpkg -i liblsl-1.14.0-Linux64-bionic.deb
$ dpkg -L liblsl
/usr/include/lsl_c.h
/usr/lib/liblsl.so.1.14.0
/usr/share/LSL/LSLConfig.cmake
[...]

Since you are talking about making it convenient for faster iteration while developing, what about including Debug vs Release in the list of default folders?

Even with Labrecorder, I rarely need liblsl debug symbols. For debugging I try to write a unit test that captures the problem and compile liblsl + unit tests with debug symbols.

Are we getting to too many folders in the search list? Maybe Findliblsl.cmake was the correct approach after all?

Findliblsl.cmake was a temporary shot at moving complexity from the main CMakeList into something manageable, but it didn't offer anything beyond what find_package already does.

I think the fact that we're at a point where a preference between the build and install path has become one of the important issue instead of something serious speaks for the find_package approach.

I'm just not convinced that [..] we can ignore testing the install output

Finding liblsl in the install()ed directory is tested every time a CI system builds an app, so if anything finding liblsl in the build dir isn't tested.

I'd like to make sure we always check our apps against the install output, and that users who have the full labstreaminglayer build tree are doing the same thing.

Given that full tree builds are deprecated I think you mean cloning the entire labstreaminglayer repository, building liblsl and then building an app. Everything else being equal finding liblsl in the install directory from an app requires users to build and install liblsl every time the pull in changes to liblsl, otherwise the build succeeds (the old install directory is still there), but doesn't include the changes (and it won't warn about it either, unless an app uses a new interface that's not present in the installed liblsl).