Skip to content
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

Fix liblo 0.32 header compatibility #1954

Merged
merged 1 commit into from Apr 2, 2024

Conversation

cbix
Copy link

@cbix cbix commented Mar 30, 2024

@@ -150,7 +150,7 @@ int OSCDataHandler(const char *osc_address, const char *types, lo_arg **argv,

if (argc == 1) {
if (type == "b") {
lo_blob blob = argv[0];
lo_blob blob = (lo_blob)argv[0];
Copy link
Author

@cbix cbix Mar 30, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@kripton kripton left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for the fix. I was looking for some VERSION symbol in liblo's headers so we could check which version we are building against. However, only a function is provided.
I can confirm that the fix works fine with liblo-0.31 and liblo-0.32, thus I approve the changes.

@peternewman and @nomis52 : Are any older versions of liblo known that we might need to support and that would not compile with these changes?

@cbix
Copy link
Author

cbix commented Mar 30, 2024

CI debian sid failure seems unrelated:

autopkgtest [20:52:13]: test command3: set -e ; for py in $(py3versions -s 2>/dev/null) ; do echo "Testing with $py:" ; $py $(which rdm_responder_test.py) --help ; done
autopkgtest [20:52:13]: test command3: [-----------------------
Testing with python3.12:
bash: line 1: python3.12: command not found
autopkgtest [20:52:14]: test command3: -----------------------]
autopkgtest [20:52:14]: test command3:  - - - - - - - - - - results - - - - - - - - - -
command3             FAIL non-zero exit status 127

@kripton
Copy link
Member

kripton commented Mar 30, 2024

CI debian sid failure seems unrelated:

autopkgtest [20:52:13]: test command3: set -e ; for py in $(py3versions -s 2>/dev/null) ; do echo "Testing with $py:" ; $py $(which rdm_responder_test.py) --help ; done
autopkgtest [20:52:13]: test command3: [-----------------------
Testing with python3.12:
bash: line 1: python3.12: command not found
autopkgtest [20:52:14]: test command3: -----------------------]
autopkgtest [20:52:14]: test command3:  - - - - - - - - - - results - - - - - - - - - -
command3             FAIL non-zero exit status 127

Yes, indeed, that python3.12-related failure is not new, so nothing you broke :)

@peternewman
Copy link
Member

Hi, and thanks for the fix. I was looking for some VERSION symbol in liblo's headers so we could check which version we are building against. However, only a function is provided.

Yeah sadly nothing easy like that and they haven't bumped in such a way we could detect a breaking ABI change either.

I can confirm that the fix works fine with liblo-0.31 and liblo-0.32, thus I approve the changes.
@peternewman and @nomis52 : Are any older versions of liblo known that we might need to support and that would not compile with these changes?

I've got an old box with liblo-0.28 still kicking about and it built fine on there too, so I think we're probably fine from that perspective.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Thanks @cbix LGTM/still compiles for me on an older version.

It would be good to get this into 0.10 branch too, but one of us can cherry-pick/backport it if you don't want to rebase onto there.

Just to have a note of it, have you (or indeed @kripton ) tested the OSC functionality with this patch or just that it compiles?

@cbix
Copy link
Author

cbix commented Mar 31, 2024

have you tested the OSC functionality

not 100 % sure how to test it, I just started olad and ran a python-osc example against its port 7770, couldn't observe any issues or errors but also nothing happens inside ola_dmxmonitor

@peternewman
Copy link
Member

not 100 % sure how to test it, I just started olad and ran a python-osc example against its port 7770, couldn't observe any issues or errors but also nothing happens inside ola_dmxmonitor

So it depends which direction you're testing @cbix . There's more detail in the plugin's docs (available from olad or online here):
https://docs.openlighting.org/ola/conf/ola-osc.conf.html

So by default it should listen on:
https://github.com/OpenLightingProject/ola/blob/aee74a22b88c12b47a52949e828e670f521385ca/plugins/osc/OSCPlugin.cpp#L47C53-L47C69

From memory the input side accepts any of the formats of the output side.

I think I've used oscdump from liblo-tools to look at data being sent from OLAd in the past.

Remember to patch the OSC port to an OLA universe before checking dmxconsole/dmxmonitor (but I suspect you're aware of that bit already).

@cbix
Copy link
Author

cbix commented Apr 2, 2024

Got it, OSC sending and receiving works! Tested with oscdump for output ports and python-osc with random floats and 512 byte blobs for input ports:

Screencast.from.2024-04-02.02-01-56.webm

@peternewman
Copy link
Member

Perfect thanks @cbix !

Do you want to try and rebase this onto the 0.10 branch, otherwise we can merge what you've got and backport it ourselves?

@cbix cbix changed the base branch from master to 0.10 April 2, 2024 08:25
@cbix
Copy link
Author

cbix commented Apr 2, 2024

@peternewman done :)

@peternewman peternewman added this to the 0.10.10 milestone Apr 2, 2024
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM thanks @cbix 💯 !

@peternewman peternewman merged commit 578b2fb into OpenLightingProject:0.10 Apr 2, 2024
19 of 20 checks passed
@kripton
Copy link
Member

kripton commented Apr 2, 2024

Would it be possible to release an 0.10.10 with that fix?

kripton added a commit to kripton/gentoo that referenced this pull request Apr 11, 2024
Closes: https://bugs.gentoo.org/927000
Upstream-PR: OpenLightingProject/ola#1954
Upstream-Commit: OpenLightingProject/ola@e083653
Signed-off-by: Jannis Achstetter <kripton@kripserver.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants