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 noetic to GitHub actions jobs #103

Merged
merged 31 commits into from Dec 8, 2020
Merged

Conversation

hslusarek
Copy link
Contributor

@hslusarek hslusarek commented Nov 18, 2020

This PR adds the GitHub action support for ROS noetic.

ToDo

  • uncomment unittest_monitoring_frame_msg and fix usage of fmt
  • Is there a better way to discriminate between different googletest versions than using the ROS_VERSION_MINOR macro? (especially for pure googletests)
  • Update readme: main branch targets melodic & noetic

@martiniil martiniil mentioned this pull request Dec 2, 2020
@@ -0,0 +1,30 @@
name: CI-Melodic
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need two seperate workflow files? What is the advantage?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is just a matter of taste. Correct, @SansoneG ?
I like it, because it is less confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The primary reason was to be able to generate CI-Badges for melodic and noetic separately.
Otherwise it is a matter of preference as @martiniil mentioned.

@@ -1,4 +1,4 @@
name: CI
name: CI-Noetic
Copy link
Contributor

Choose a reason for hiding this comment

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

The diff seems to indicate this was was actually git mv from the original?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. Maybe this file simply has more similarities with the original than ci_action_melodic.yml?

CLANG_FORMAT_CHECK: file
CLANG_FORMAT_VERSION: 3.9
CLANG_FORMAT_VERSION: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason behind this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, why it was 3.9 before. Do you know the reason?

Anyways, now the version corresponds to the default on ubuntu focal.

@martiniil
Copy link
Contributor

The test-build with clang-tidy enabled fails on noetic. I can reproduce this in a docker container. It says that it cannot match a template member function when trying to format a psen_scan_v2::monitoring_frame::Message.

I don't understand that this only happens when clang-tidy is enabled. It seems the following check fails but should be true:

is_constructible<fmt::v6::internal::fallback_formatter<psen_scan_v2::monitoring_frame::Message, char, void>>::value

I attach the full error log.
catkin_Log.txt

@agutenkunst
Copy link
Contributor

Please add the information to the Readme that main targets melodic and noetic.

@martiniil martiniil merged commit 687a1c1 into main Dec 8, 2020
@martiniil martiniil deleted the add/github_actions_for_noetic branch December 8, 2020 15:03
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

5 participants