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

Replace tools/code_check.sh with 'make codecheck' #26

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

adlarkin
Copy link
Contributor

With the new ign-cmake release coming out (2.6.0 - gazebosim/gz-cmake#131), we should run make codecheck in GitHub actions instead of running tools/code_check.sh so that we can make sure users are adhering to the new code checker introduced in gazebosim/gz-cmake#117.

I haven't tested this locally because I'm not sure how to, since it's a change for GitHub actions... does anyone know how to test this/would anyone be willing to test this?

Also, once this is merged and ign-cmake-2.6.0 is released, we will need to update all affected repositories to make sure that CI isn't broken due to being unable to pass make codecheck in GitHub actions (most repositories were already updated a few weeks ago in anticipation for this ign-cmake release, but I'm sure changes have been made to these repositories since things were updated a few weeks ago).

Signed-off-by: Ashton Larkin [email protected]

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

does anyone know how to test this/would anyone be willing to test this?

You can create a branch on any Ignition library and update actions to use this branch, for example:

https:/ignitionrobotics/ign-plugin/blob/4a87904ac0b70385d8e5bb520e84ab11aa2eaba5/.github/workflows/ci.yml#L14

Actions will be triggered on push, so you can see the results on that commit and don't need to open a PR.

once this is merged and ign-cmake-2.6.0 is released

It's already out 😉 http://packages.osrfoundation.org/gazebo/ubuntu-stable/pool/main/i/ignition-cmake2/

we will need to update all affected repositories

Yup, but you did a good push on this a while back, right? So I think we may be able to just get this in and deal with the fallout? 😏

entrypoint.sh Outdated Show resolved Hide resolved
adlarkin added a commit to gazebosim/gz-math that referenced this pull request Dec 10, 2020
adlarkin added a commit to gazebosim/gz-tools that referenced this pull request Dec 10, 2020
@adlarkin adlarkin marked this pull request as draft December 10, 2020 18:28
adlarkin added a commit to gazebosim/gz-math that referenced this pull request Dec 10, 2020
@adlarkin
Copy link
Contributor Author

I went ahead and ran a few tests in ign-math (which should use make codecheck) and ign-tools (which should use tools/code_check.sh). The following test cases behaved as expected:

  1. Default branch of ign-tools (ign-tools1) should pass GitHub actions using tools/code_check.sh: https:/ignitionrobotics/ign-tools/runs/1532616226?check_suite_focus=true
  2. Default branch of ign-math (ign-math6) should pass GitHub actions using make codecheck: https:/ignitionrobotics/ign-math/runs/1532618612
  3. Default branch of ign-math should fail GitHub actions using make codecheck by adding a really long comment that doesn't adhere to the 80 character width limit: https:/ignitionrobotics/ign-math/runs/1532762475

So, unless anyone notices a test case that is missed, I think this is ready for review.

@adlarkin adlarkin marked this pull request as ready for review December 10, 2020 22:28
entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Let's do this! I say we just pull the trigger and deal with the fallout, in case there's any. Otherwise we'll always be chasing a moving target, because there are PRs being merged after we fix the checker.

@adlarkin adlarkin force-pushed the adlarkin/actions_make_codecheck branch from a997193 to 9dd345d Compare December 15, 2020 18:36
@adlarkin adlarkin force-pushed the adlarkin/actions_make_codecheck branch from 9dd345d to b3c9314 Compare December 15, 2020 18:37
@adlarkin adlarkin merged commit b3c9314 into master Dec 15, 2020
@adlarkin adlarkin deleted the adlarkin/actions_make_codecheck branch December 15, 2020 18:38
@adlarkin adlarkin mentioned this pull request Dec 18, 2020
@scpeters
Copy link
Contributor

I think make codecheck doesn't actually work for ign-cmake itself: gazebosim/gz-cmake#134

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.

3 participants