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

Add cross-platform CI with conda packages (minimal version) #1636

Merged

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.

CMakeLists.txt Outdated Show resolved Hide resolved
src/rviz/default_plugin/marker_display.cpp Outdated Show resolved Hide resolved
src/rviz/yaml_config_writer.cpp Show resolved Hide resolved
@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 😉

src/rviz/windows_compat.h Outdated Show resolved Hide resolved
src/rviz/windows_compat.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@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.

.github/workflows/cross_platform_ci.yml Outdated Show resolved Hide resolved
.github/workflows/cross_platform_ci.yml Outdated Show resolved Hide resolved
.github/workflows/cross_platform_ci.yml Outdated Show resolved Hide resolved
.github/workflows/cross_platform_ci.yml Outdated Show resolved Hide resolved
@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