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

Workaround for ament_lint issues. Run CI every night. #302

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

esteve
Copy link
Collaborator

@esteve esteve commented Feb 26, 2023

No description provided.

@esteve esteve marked this pull request as ready for review February 26, 2023 18:30
@nnmm
Copy link
Contributor

nnmm commented Feb 27, 2023

Looks like the same error happens also on humble.

@esteve
Copy link
Collaborator Author

esteve commented Feb 27, 2023

@nnmm @jhdcs It seems to be caused by pydocstyle >= 6.2.0 ament/ament_lint#428, which gets installed by ros-tooling/setup-ros in https://github.com/ros-tooling/setup-ros/blob/master/src/package_manager/pip.ts#L51

Options I can think of:

3 seems easiest, 2 would be the most "correct" one (but every CI run will take longer) and I'd try to avoid 1

@nnmm
Copy link
Contributor

nnmm commented Feb 27, 2023

@esteve Have you seen ament/ament_lint#428? I think it has already been fixed, we just need to wait for a Rolling sync: ros-tooling/action-ros-ci#795 (comment)

@nnmm
Copy link
Contributor

nnmm commented Feb 27, 2023

I did try using the prerelease/testing APT repo as suggested in the latter link, but it didn't work.

@jhdcs
Copy link
Collaborator

jhdcs commented Feb 27, 2023

My personal vote would be going for the most correct option - however, how much time would this be adding to CI?

@esteve
Copy link
Collaborator Author

esteve commented Feb 27, 2023

My personal vote would be going for the most correct option - however, how much time would this be adding to CI?

This would mean rebuilding ament_lint, which won't take long (it's just a bunch of CMake scripts), and I'm assuming that installing all its development dependencies won't take long either, I don't know for sure, but it's worth a try.

@esteve
Copy link
Collaborator Author

esteve commented Feb 27, 2023

I did try using the prerelease/testing APT repo as suggested in the latter link, but it didn't work.

Yeah, that's weird, there are in fact .debs with newer releases of ament_pep257 for both humble and rolling in the testing repo:

http://packages.ros.org/ros2-testing/ubuntu/pool/main/r/ros-rolling-ament-cmake-pep257/
http://packages.ros.org/ros2-testing/ubuntu/pool/main/r/ros-humble-ament-cmake-pep257/

I don't know why they don't get installed. I've updated setup-ros to v0.6 in case that's why the action was not picking up the testing repository.

@esteve esteve changed the title Allow rolling job to fail Update setup-ros to v0.6 and enable testing repository temporarily Feb 27, 2023
@esteve
Copy link
Collaborator Author

esteve commented Feb 27, 2023

setup-ros@v0.6 dropped support for Galactic as it's no longer supported by Open Robotics (support ended in November https://www.ros.org/reps/rep-2000.html#galactic-geochelone-may-2021-november-2022)

@esteve
Copy link
Collaborator Author

esteve commented Mar 6, 2023

Unfortunately, until ament_lint gets tagged for foxy and the latest release includes ament/ament_lint#431, the foxy job will continue failing. I filed ament/ament_lint#434, so hopefully we can use the ros-testing repo with foxy at some point.

@nnmm
Copy link
Contributor

nnmm commented Mar 7, 2023

@esteve There is also another error in the rolling build:

Starting >>> rclrs_example_msgs
  --- output: rclrs
  error[E0308]: mismatched types
     --> src/subscription/message_info.rs:120:19
      |
  120 |             data: rmw_message_info.publisher_gid.data,
      |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 24 elements, found one with 16 elements

@esteve
Copy link
Collaborator Author

esteve commented Mar 8, 2023

@nnmm yeah, saw that, but I'm more interested now in getting humble and foxy passing. Looks like the publisher_gid field got updated to use 16 bytes, instead of 24, in rolling (see ros2/rmw#345)

@esteve esteve force-pushed the continue-on-error-rolling branch from aa60f85 to ef88829 Compare March 9, 2023 10:55
@esteve esteve changed the title Update setup-ros to v0.6 and enable testing repository temporarily Workaround for ament_lint issues. Run CI every night. Mar 9, 2023
@esteve
Copy link
Collaborator Author

esteve commented Mar 9, 2023

@nnmm I've redone this PR to just install a non-problematic version of pydocstyle until foxy gets a new version of ament_lint / there's a new sync. I've also added a schedule for running the CI job every night so that we can catch errors in rolling earlier, we didn't see the issue with the publisher GID until there was a PR, so it'd be better to be able to catch these issues earlier. We can add use-ros2-testing: ${{ matrix.ros_distribution == 'rolling' }}, but I'd rather do it once #306 gets merged and we upgrade setup-ros to v0.6

@maspe36 once this PR is merged, can you rebase #306 on top of main? Or rebase #306 on top of this PR now? I minimized the changes so it shouldn't have any conflicts with yours. It should fix the CI failures you see in your PR (which are not caused by your changes, it's still the pydocstyle problem). Thanks.

Copy link
Contributor

@nnmm nnmm left a comment

Choose a reason for hiding this comment

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

LGTM!

@nnmm
Copy link
Contributor

nnmm commented Mar 11, 2023

I hope it's fine if I merge your PR this time @esteve, to unblock other PRs.

@nnmm nnmm merged commit 37de108 into main Mar 11, 2023
@delete-merged-branch delete-merged-branch bot deleted the continue-on-error-rolling branch March 11, 2023 15:20
@esteve
Copy link
Collaborator Author

esteve commented Mar 11, 2023

@nnmm sure! Thanks for merging this PR 🙂

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