Skip to content

[liblsl] Update to 1.14.0#16461

Merged
ras0219-msft merged 12 commits intomicrosoft:masterfrom
chausner:liblsl-1.14.0
Mar 10, 2021
Merged

[liblsl] Update to 1.14.0#16461
ras0219-msft merged 12 commits intomicrosoft:masterfrom
chausner:liblsl-1.14.0

Conversation

@chausner
Copy link
Copy Markdown
Contributor

This updates liblsl to the latest version 1.14.0.

It also fixes an issue that prevented the previous ports from compiling on Linux and probably MacOS. So I removed the corresponding entries from the CI baseline assuming that it now builds fine on both platforms. I successfully tested the port on Windows and Linux but have no way to test on MacOS.

I once again tried to get static builds working but it appears bigger changes need to be made in the original repository so I kept it disabled for now.

@chausner
Copy link
Copy Markdown
Contributor Author

FYI The fix to make it compile also on non-Windows platforms was to simply remove the quotation marks where -Dlslgitrevision= is defined.

@JonLiu1993 JonLiu1993 self-assigned this Mar 1, 2021
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Mar 1, 2021
@JonLiu1993 JonLiu1993 assigned JackBoosY and unassigned JonLiu1993 Mar 1, 2021
@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 1, 2021
@chausner
Copy link
Copy Markdown
Contributor Author

chausner commented Mar 1, 2021

@JackBoosY Thanks a lot for your refactorings, it's much cleaner now. 👍 Just tested again on Linux and Windows, and looks good to me overall.

@chausner
Copy link
Copy Markdown
Contributor Author

chausner commented Mar 1, 2021

One more note: liblsl bundles pugixml but it can be turned off by setting LSL_BUNDLED_PUGIXML=OFF. I already tested with using the pugixml dependency in vcpkg and it compiles fine. However, I decided against using the vcpkg dependency for that since liblsl can only be built as a dynamic library at the moment (so there should not be any problem if the user links against pugixml in their own application again). Also, using the bundled version of pugixml is probably safer than some potentially incompatible version that comes in vcpkg.

I am not sure what the general recommendation is, but if you prefer to use the vcpkg dependency instead of the bundled one, I am happy to change it.

Copy link
Copy Markdown
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, overall it looks great!

Our policy is that we must use the vcpkg copy of pugixml: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-vendored-dependencies

LGTM once this is addressed :)

@chausner
Copy link
Copy Markdown
Contributor Author

chausner commented Mar 1, 2021

Our policy is that we must use the vcpkg copy of pugixml: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-vendored-dependencies

Alright, I made the necessary changes in 8932039.

What about Boost which is currently bundled in parts in liblsl, see https://github.com/sccn/liblsl#boost? In contrast to pugixml, it cannot be disabled easily so if we also want to replace it with the Boost vcpkg package, we need to resort to patching, I'm afraid.

chausner and others added 3 commits March 1, 2021 22:26
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@JackBoosY
Copy link
Copy Markdown
Contributor

@chausner Yep, we should patch its source to use boost components also.

@JackBoosY JackBoosY added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 2, 2021
@chausner
Copy link
Copy Markdown
Contributor Author

chausner commented Mar 2, 2021

@chausner Yep, we should patch its source to use boost components also.

I looked into this a bit but it seems non-trivial to do. They took a copy of Boost and changed all namespaces to "lslboost". So to make it work with the original Boost headers, we would have to patch the liblsl code base to use the original names everywhere. I am not sure if it is worth the effort. There also does not seem to be an easy way to do this in CMake without resorting to platform-dependent commands like sed or iterating through all files and doing a string-replace.

In any case, since the bundling of Boost in liblsl is independent of this package update (Boost has always been bundled with liblsl), I would suggest to merge this PR for now so that we have at least the latest version of the library in vcpkg. The patching to remove the bundled Boost copy could be done separately if anybody feels like it (but it will probably not be me).

@JackBoosY
Copy link
Copy Markdown
Contributor

@chausner Yep, we should patch its source to use boost components also.

I looked into this a bit but it seems non-trivial to do. They took a copy of Boost and changed all namespaces to "lslboost". So to make it work with the original Boost headers, we would have to patch the liblsl code base to use the original names everywhere. I am not sure if it is worth the effort. There also does not seem to be an easy way to do this in CMake without resorting to platform-dependent commands like sed or iterating through all files and doing a string-replace.

In any case, since the bundling of Boost in liblsl is independent of this package update (Boost has always been bundled with liblsl), I would suggest to merge this PR for now so that we have at least the latest version of the library in vcpkg. The patching to remove the bundled Boost copy could be done separately if anybody feels like it (but it will probably not be me).

Accepted.

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Mar 3, 2021
@ras0219-msft ras0219-msft merged commit ce17802 into microsoft:master Mar 10, 2021
@ras0219-msft
Copy link
Copy Markdown
Contributor

This PR LGTM, thanks for detaching pugixml!

@chausner chausner deleted the liblsl-1.14.0 branch September 5, 2021 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants