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

Update xtypes #113

Merged
merged 49 commits into from
Jun 15, 2023
Merged

Update xtypes #113

merged 49 commits into from
Jun 15, 2023

Conversation

MiguelBarro
Copy link
Contributor

@MiguelBarro MiguelBarro self-assigned this Jan 16, 2023
@MiguelBarro MiguelBarro force-pushed the feature/cross_platform branch 29 times, most recently from 9fd6d40 to fb63906 Compare January 19, 2023 10:45
Miguel Barro added 8 commits February 23, 2023 08:43
…orm dependent.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
…bjects. Fixes issue #105

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
@russkel
Copy link
Contributor

russkel commented Mar 13, 2023

@russkel I'm afraid I was not able to reproduce the issue in the ci. Would you be so gentle to provide a reproducer?

I just tried to replicate the issue and was unable to. Now that I am thinking about it, it is possible I had a dirty build environment (where old version of xtypes was being used amongst new versions). I will do some more tests with my code base running new xtypes.

laura-eprosima added 3 commits May 9, 2023 08:43
Signed-off-by: laura-eprosima <laura@eprosima.com>
Signed-off-by: laura-eprosima <laura@eprosima.com>
Signed-off-by: laura-eprosima <laura@eprosima.com>
@MiguelBarro MiguelBarro force-pushed the feature/cross_platform branch 4 times, most recently from b22286e to 8565e03 Compare May 9, 2023 07:01
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: laura-eprosima <laura@eprosima.com>
@joakimono
Copy link

Will this PR be merged into main when approved, or are there more pending work? I'm eager to test the library (from main).

@MiguelBarro
Copy link
Contributor Author

That's the idea ... whenever we can spare a reviewer. Meanwhile, I will introduce some bugfix from time to time because I'm working on dependent repos.

Miguel Barro added 2 commits May 12, 2023 12:19
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
context.include_paths.push_back("/opt/ros/foxy/share/");

// Introduce current ros2 paths
std::string distro(std::getenv("ROS_DISTRO"));

Choose a reason for hiding this comment

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

If the environment variable is undefined, std::getenv returns a null pointer. The std::string constructor throws a std::logic_error instead of the graceful exit message. Suggest const char* distro = std::getenv("ROS_DISTRO");, check for null pointer and construct distro string in else .. + std::string(distro) +...

Is the xtypes_idl_validator only intended for ROS? The current implementation demands ROS_DISTRO to be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got me! I turns out that construct a std::string from nullptr is undefined behavior. On MSVC leads to an empty string and in gcc a std::logic_error is raised. Crossplatform pitfalls 🙄.
We only used xtypes_idl_validator in an Integration-Service ROS2-SH related project. It is interesting for us to fail without a ROS2 overlay. But is a good hint too. Let's turn it into a warning.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
@rsanchez15 rsanchez15 merged commit 16eb3fe into main Jun 15, 2023
@rsanchez15 rsanchez15 deleted the feature/cross_platform branch June 15, 2023 06:55
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.

4 participants