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

Continuous Integration/ROS Noetic Compatibility #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marip8
Copy link

@marip8 marip8 commented Jan 28, 2021

This PR adds Github Actions Continuous Integration (CI) to ensure the repository builds for ROS Kinetic, Melodic, and Noetic. CI is configured to run for pull requests against the master branch, when changes are pushed to the master branch, and on a weekly basis (to catch breakage from dependency changes). Currently the unit tests are disabled in the CI process because the machine running the CI build is not connected to a Photoneo scanner which is required by the tests.

This PR also compiles the package with C++ 14 for compatibility with PCL 1.10 which is the default version for ROS Noetic.

@marip8
Copy link
Author

marip8 commented Jan 28, 2021

This PR may not start a build until it gets merged into the master branch or Github Actions is enabled for this repository. In either case here is a link to the CI result of the same branch in a dummy PR against my own fork's master branch to show the result.

Copy link
Contributor

@PhotoTeeborChoka PhotoTeeborChoka left a comment

Choose a reason for hiding this comment

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

Overall, this is an excellent improvement, it formalizes the installation process of PhoXiControl for Kinetic, Melodic and Noetic.

There are minor modifications that could be done on top of this PR:

  1. use a single installer script (there are 2 used currently, 1 for Kinetic and 1 for Melodic/Noetic)
  2. merge common code
  3. extend with deeper versioning (not only for 1.2.14 support)

But this can be done as a separate PR as well and we could merge this into the master.

Overall, this is a welcome and beneficial addition to the repository.

Thank you, @marip8

@PhotoTeeborChoka
Copy link
Contributor

Note: please follow the official support website for PhoXiControl, it is advised to only use supported operating systems with PXC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants