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 pre-commit, move from travis to GitHub actions, fix style #129

Merged
merged 9 commits into from
Apr 8, 2022

Conversation

mintar
Copy link
Contributor

@mintar mintar commented Apr 8, 2022

See separate commits for detailed descriptions.

Now everything passes the clang-check-style.py again.
This fixes the following catkin_lint warning:

    warning: variable CMAKE_CXX_FLAGS is modified
From the CMake documentation:

> The PROGRAMS form is identical to the FILES form except that the
> default permissions for the installed file also include OWNER_EXECUTE,
> GROUP_EXECUTE, and WORLD_EXECUTE. This form is intended to install
> programs that are not targets, such as shell scripts.

Since we are installing libraries, we don't want to set the executable
bit. Also fixes catkin_lint warning "executable file is not installed to
bin destination".
@mintar mintar requested a review from clalancette April 8, 2022 14:55
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Yes please! This looks like a good thing to have; once we merge this, we should probably port over to the ROS 2 branches.

One thing is that I didn't see the workflow actually run in this PR. I thought that was supposed to happen when you initially added a .github/workflows file, but I don't see it. Any thoughts on what is going on there? Regardless, I think this is OK to merge in, as we can always do a follow-up with fixes.

@mintar mintar merged commit 806498e into ros-drivers:noetic Apr 8, 2022
@mintar mintar deleted the feat-pre-commit branch April 8, 2022 16:42
@mintar
Copy link
Contributor Author

mintar commented Apr 8, 2022

I was wondering the same! Anyhow, now it's building for the merge commit (because it's a push): https://github.com/ros-drivers/phidgets_drivers/actions/runs/2116423918

Let's hope it will also build for future pull requests. Let's observe this.

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.

None yet

2 participants