Skip to content

Conversation

@Tobias-Fischer
Copy link
Contributor

Description

This PR supersedes #1600 and introduces cross-platform CI for rviz using the RoboStack. It is a minimal set of changes, in particular it does not include changes from #1532 that should be discussed separately. It would be great to get some feedback from the rviz maintainers to see whether there is interest in getting this merged :)

/cc @wolfv

Checklist

  • If you are addressing rendering issues, please provide:
    • Images of both, broken and fixed renderings.
    • Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.
  • If you are changing GUI, please include screenshots showing how things looked before and after.
  • Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
    Due to the lack of active maintainers, we cannot provide support for older release branches anymore.
  • Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers of RViz. Refer to the RViz Wiki for reviewing guidelines.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Sorry for ignoring this PR for so long. I didn't have time to look into this yet.
Of course, I'm open to increase cross-platform compatibility and I agree to most of the changes here. However, I'm not sure we should establish a separate CI workflow. In MoveIt we experienced many false-positive alarms. So if you insist in this approach, please ensure that you fix all upstream dependencies to not experience false alarms.

@wolfv
Copy link
Contributor

wolfv commented Nov 2, 2021

Ok, that was definitely too early for me :) sorry. I should boot on Windows and see what's going on here.

@wolfv wolfv force-pushed the noetic-devel-cross-ci-new branch from b47ab97 to 9e81c2c Compare November 2, 2021 07:13
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

A few more comments 😉

@Tobias-Fischer
Copy link
Contributor Author

I just merged noetic-devel into the cross-ci branch and have already picked up (and fixed) the first regression: 9b1f9dd

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Finally, some remarks to the workflow definition. Please rename some files:

  • .github/ci_cross_platform_env.yml -> .github/robostack_env.yaml
  • .github/workflows/cross_platform_ci.yml -> .github/workflows/robostack.yaml
  • .github/workflows/prerelease.yml -> .github/workflows/prerelease.yaml

Fix as many versions as possible to reduce the risk of false-positives.

@rhaschke rhaschke merged commit 3924f0e into ros-visualization:noetic-devel Nov 3, 2021
@Tobias-Fischer Tobias-Fischer deleted the noetic-devel-cross-ci-new branch November 3, 2021 20:11
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.

6 participants