Skip to content

Cleanup CMake redundancies and use FILE_SET for headers #236

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

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

cboulay
Copy link
Collaborator

@cboulay cboulay commented Jul 8, 2025

This is an attempt to delete unneeded CMake code. Draft for now; I just want to trigger the CI runners.

Installing Headers

I struggled with this for a while. It should be the case that headers are installed automatically with install(TARGETS ... INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}), as long as the headers end up in the interface property of the installed target. And the headers should be in this property of the lsl target when it links in lslobj. However, something wasn't quite working.

An alternative would be to use the PUBLIC_HEADER property. This worked to propagate the headers, but for some reason this flattens all the headers into a single folder.

Instead, I ended up going with a relatively new FILE_SET feature. This works well.

LSLTargets.cmake

It looks like we provide a hard-coded LSLConfig.cmake which is simply a wrapper around LSLTargets.cmake -- AND it forces the includes of LSLCMake.cmake. I'm going to try and remove the LSLConfig.cmake and instead generate that file (LSLTargets -> LSLConfig) with the consequence that we won't be forcing people to load LSLCMake.cmake if they find_target(LSL). I think it's likely that I will add it back, but it needs to be updated or cleaned. e.g., there's a bunch of code in there to help make Apple app bundles but that's a lot less useful now that Apple has increased MacOS security, unless we add in code signing and notarization but that's far less portable.

@cboulay cboulay force-pushed the cboulay/cmake_cleanup branch from 018f690 to 91f5b0c Compare July 12, 2025 17:40
@cboulay
Copy link
Collaborator Author

cboulay commented Jul 12, 2025

Update:

On headers, I was originally fighting against lslobj exposing 3rd party libraries because I wanted lsl to link in lslobj PUBLICly with the purpose of inherit lslobj's header definition(s). However, the 'internal tests' require those 3rd party libraries. The choice was to either (1) make the 3rd party libs PRIVATE in lslobj then make the internal tests include/link those 3rd party libs independently, or (2) leave internal tests as they are, link lslobj into lsl as PRIVATE, then manually add the headers to lsl.
I went with the 2nd option because it was simpler.

On the exported CMake config:

By configuring lslexamples directly in the primary cmake tree and copying some cmake config files into the build destination, the lslexamples were not adequately testing the liblsl install. Whether or not that's what the examples were intended to do originally, that's now how I'm using them. I cleaned all the unnecessary config files out of the build destination, and removed lslexamples from the primary CMake tree. Then I added a separate examples step to the GHA script.

For future liblsl debug development, we should really use the 'external tests'.

I still have some Windows work to do before this is ready...

Base automatically changed from cboulay/cmake_refactor to dev July 13, 2025 05:29
@cboulay cboulay force-pushed the cboulay/cmake_cleanup branch from 5e4731a to 283b81b Compare July 13, 2025 05:31
@cboulay cboulay marked this pull request as ready for review July 13, 2025 05:31
@cboulay cboulay force-pushed the cboulay/cmake_cleanup branch from 8d2a332 to c1d3ebe Compare July 14, 2025 21:37
@cboulay
Copy link
Collaborator Author

cboulay commented Jul 15, 2025

It's a shame but I'm going to have to abandon the FILE_SET approach for the headers because it is incompatible with Apple Frameworks. I'll do that in the new PR.

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.

1 participant