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 examples for release 0.7.0 #104

Merged
merged 5 commits into from
May 1, 2024
Merged

Fix examples for release 0.7.0 #104

merged 5 commits into from
May 1, 2024

Conversation

cmnrd
Copy link
Contributor

@cmnrd cmnrd commented Mar 11, 2024

This PR updates the examples to be ready for the 0.7.0 release. This release introduces a couple of breaking changes and the examples need to be adjusted accordingly.

Fixes #103

@cmnrd cmnrd force-pushed the fix-release-0.7.0 branch from 53bd7f4 to 0db31bb Compare March 12, 2024 10:16
@cmnrd cmnrd force-pushed the fix-release-0.7.0 branch from 0db31bb to 71a79d0 Compare March 12, 2024 12:14
@cmnrd
Copy link
Contributor Author

cmnrd commented Mar 12, 2024

@edwardalee I fixed the cmake issues, but now the linker complains in the ROS and MQTT examples. It is missing a reference to SET_NEW_ARRAY. Is this part of the old C API that got removed recently? If so, could you update the examples accordingly?

@edwardalee
Copy link
Contributor

Just pushed updates to replace LF_SET_ARRAY.
I can't test the ROS code, and I'm not sure it's got the memory management quite right.
I've added a note to the code with a question of whether we should be calling release_rcl_serialized_message.

@cmnrd cmnrd marked this pull request as ready for review March 18, 2024 14:24
@cmnrd
Copy link
Contributor Author

cmnrd commented Mar 18, 2024

The memory management indeed looks suspicious. However, I don't understand the current implementation of tokens (and in particular its assumptions) well enough to comment on it.

Should we go ahead and merge this or wait for the 0.7.0 release? Some of the changes are incompatible with 0.6.0.

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge.

@lhstrh
Copy link
Member

lhstrh commented Mar 18, 2024

I think this is ready to merge.

It won't be ready to merge until we release v0.7.0...

@cmnrd
Copy link
Contributor Author

cmnrd commented Mar 19, 2024

We are in an inconsistent state anyway... Currently, the playground neither works with 0.6.0 nor with the current master. If we merge this, it will at least work with master.

@lhstrh lhstrh merged commit 59df69b into main May 1, 2024
5 checks passed
@lhstrh lhstrh deleted the fix-release-0.7.0 branch May 1, 2024 18:37
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.

CMake problem with MQTT examples
3 participants