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 Ubuntu Jammy CI #1418

Merged
merged 8 commits into from
Apr 12, 2022
Merged

Add Ubuntu Jammy CI #1418

merged 8 commits into from
Apr 12, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Mar 30, 2022

🎉 New feature

Summary

I'm debugging Gazebo on Jammy from source and the changes in packages.apt were needed to install the correct dependencies, so I went ahead and opened the PR.

This is ready for review!

Test it

Eventually Jammy CI should come back green.

Jammy CI comes back green sometimes, but it's flaky other times. Just like Bionic and Focal.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added the tests Broken or missing tests / testing infra label Mar 30, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Mar 30, 2022
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #1418 (fca6c16) into ign-gazebo6 (fa2d7cc) will increase coverage by 0.13%.
The diff coverage is 98.27%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1418      +/-   ##
===============================================
+ Coverage        63.36%   63.49%   +0.13%     
===============================================
  Files              306      307       +1     
  Lines            24715    24771      +56     
===============================================
+ Hits             15660    15729      +69     
+ Misses            9055     9042      -13     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/detail/BaseView.hh 100.00% <ø> (ø)
src/BaseView.cc 100.00% <ø> (ø)
src/gui/plugins/spawn/Spawn.cc 10.27% <ø> (+0.46%) ⬆️
...rc/systems/odometry_publisher/OdometryPublisher.hh 100.00% <ø> (ø)
...rc/systems/odometry_publisher/OdometryPublisher.cc 88.37% <95.16%> (+2.31%) ⬆️
...e/ignition/gazebo/detail/EntityComponentManager.hh 93.86% <100.00%> (+0.44%) ⬆️
include/ignition/gazebo/detail/View.hh 100.00% <100.00%> (ø)
src/View.cc 100.00% <100.00%> (ø)
src/systems/imu/Imu.cc 74.25% <100.00%> (+2.51%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbc16c6...fca6c16. Read the comment docs.

@chapulina
Copy link
Contributor Author

Ouch, python 3.10 hits us, @j-rivero . Not sure you've seen this yet

  The following packages have unmet dependencies:
   python3-ignition-math6 : Depends: python3 (< 3.10) but 3.10.1-0ubuntu2 is to be installed
  E: Unable to correct problems, you have held broken packages.

@j-rivero
Copy link
Contributor

Ouch, python 3.10 hits us, @j-rivero . Not sure you've seen this yet

  The following packages have unmet dependencies:
   python3-ignition-math6 : Depends: python3 (< 3.10) but 3.10.1-0ubuntu2 is to be installed
  E: Unable to correct problems, you have held broken packages.

I did not see it. I'll fix it.

@j-rivero
Copy link
Contributor

j-rivero commented Apr 5, 2022

I did not see it. I'll fix it.

I think that we were affected by the python 3.10 transition done in Jammy during the last month. Our ign-math6 package is coming from February and they changed the version. I'm going to bump the revision to just build the same source code again. That should make it.

@j-rivero
Copy link
Contributor

j-rivero commented Apr 5, 2022

I think that we were affected by the python 3.10 transition done in Jammy during the last month. Our ign-math6 package is coming from February and they changed the version. I'm going to bump the revision to just build the same source code again. That should make it.

I've released 6.10.0-100 and that fixed the problem. CI is still failing, now because doxygen segfaults. I'm going to look at it.

@chapulina chapulina marked this pull request as ready for review April 6, 2022 00:53
@j-rivero
Copy link
Contributor

j-rivero commented Apr 6, 2022

Two things on my mind to stop using doxygen unconditionally:

  • Extend the doxygen-enabled input in the github action to avoid the installation of doxygen. You already proceed with that in gazebo-tooling/action-gz-ci@17c5aff. Sounds reasonable to me.
  • Why is ign-gazebo unconditionally running the doc generation in standard: cmake && make? Should it be out of the default target and only being run with make doc?

For the second, a simple patch could be enough:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 13443e62..6ac33f1a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -225,26 +225,26 @@ endif()
 configure_file(${CMAKE_SOURCE_DIR}/api.md.in ${CMAKE_BINARY_DIR}/api.md)
 configure_file(${CMAKE_SOURCE_DIR}/tutorials.md.in ${CMAKE_BINARY_DIR}/tutorials.md)
 
-# disable doxygen on macOS due to issues with doxygen 1.9.0
-# there is an unreleased fix; revert this when 1.9.1 is released
-# https://github.com/ignitionrobotics/ign-gazebo/issues/520
-if (NOT APPLE)
-  ign_create_docs(
-    API_MAINPAGE_MD "${CMAKE_BINARY_DIR}/api.md"
-    TUTORIALS_MAINPAGE_MD "${CMAKE_BINARY_DIR}/tutorials.md"
-    ADDITIONAL_INPUT_DIRS "${CMAKE_SOURCE_DIR}/src/systems ${CMAKE_SOURCE_DIR}/src/gui/plugins"
-    IMAGE_PATH_DIRS "${CMAKE_SOURCE_DIR}/tutorials/files"
-    TAGFILES
-     "${IGNITION-MATH_DOXYGEN_TAGFILE} = ${IGNITION-MATH_API_URL}"
-     "${IGNITION-MSGS_DOXYGEN_TAGFILE} = ${IGNITION-MSGS_API_URL}"
-     "${IGNITION-PHYSICS_DOXYGEN_TAGFILE} = ${IGNITION-PHYSICS_API_URL}"
-     "${IGNITION-PLUGIN_DOXYGEN_TAGFILE} = ${IGNITION-PLUGIN_API_URL}"
-     "${IGNITION-TRANSPORT_DOXYGEN_TAGFILE} = ${IGNITION-TRANSPORT_API_URL}"
-     "${IGNITION-SENSORS_DOXYGEN_TAGFILE} = ${IGNITION-SENSORS_API_URL}"
-     "${IGNITION-COMMON_DOXYGEN_TAGFILE} = ${IGNITION-COMMON_API_URL}"
-  )
-endif()
-
 if(TARGET doc)
+  # disable doxygen on macOS due to issues with doxygen 1.9.0
+  # there is an unreleased fix; revert this when 1.9.1 is released
+  # https://github.com/ignitionrobotics/ign-gazebo/issues/520
+  if (NOT APPLE)
+    ign_create_docs(
+      API_MAINPAGE_MD "${CMAKE_BINARY_DIR}/api.md"
+      TUTORIALS_MAINPAGE_MD "${CMAKE_BINARY_DIR}/tutorials.md"
+      ADDITIONAL_INPUT_DIRS "${CMAKE_SOURCE_DIR}/src/systems ${CMAKE_SOURCE_DIR}/src/gui/plugins"
+      IMAGE_PATH_DIRS "${CMAKE_SOURCE_DIR}/tutorials/files"
+      TAGFILES
+       "${IGNITION-MATH_DOXYGEN_TAGFILE} = ${IGNITION-MATH_API_URL}"
+       "${IGNITION-MSGS_DOXYGEN_TAGFILE} = ${IGNITION-MSGS_API_URL}"
+       "${IGNITION-PHYSICS_DOXYGEN_TAGFILE} = ${IGNITION-PHYSICS_API_URL}"
+       "${IGNITION-PLUGIN_DOXYGEN_TAGFILE} = ${IGNITION-PLUGIN_API_URL}"
+       "${IGNITION-TRANSPORT_DOXYGEN_TAGFILE} = ${IGNITION-TRANSPORT_API_URL}"
+       "${IGNITION-SENSORS_DOXYGEN_TAGFILE} = ${IGNITION-SENSORS_API_URL}"
+       "${IGNITION-COMMON_DOXYGEN_TAGFILE} = ${IGNITION-COMMON_API_URL}"
+    )
+  endif()
+
   file(COPY ${CMAKE_SOURCE_DIR}/tutorials/files/ DESTINATION ${CMAKE_BINARY_DIR}/doxygen/html/files/)
 endif()

@chapulina
Copy link
Contributor Author

Why is ign-gazebo unconditionally running the doc generation in standard

I think it has never come up before. I'm ok with your proposal. It would be interesting to do it from ign-gazebo3, and do it for other libs as well. Maybe this can be done at ign-cmake within the ign_create_docs function?

@chapulina
Copy link
Contributor Author

Good news is that CI for Jammy is as good as Bionic and Focal now 🙂 A bit flaky 🙃

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina requested a review from j-rivero April 6, 2022 16:53
@mjcarroll
Copy link
Contributor

Why is ign-gazebo unconditionally running the doc generation in standard: cmake && make? Should it be out of the default target and only being run with make doc?

I'm pretty sure that all of our packages do, which is why I added this PR previously: gazebosim/gz-cmake#144

@mjcarroll
Copy link
Contributor

@chapulina
Copy link
Contributor Author

Ah I didn't remember that option, @mjcarroll . Maybe we can just use that on the action then. That should be easier than changing ign-cmake

@j-rivero
Copy link
Contributor

j-rivero commented Apr 7, 2022

We add the doc target to ALL here: https://github.com/ignitionrobotics/ign-cmake/blob/ign-cmake2/cmake/IgnCreateDocs.cmake#L179-L186

Ah thanks for the reminder. It could be somehow an intrusive change, maybe we target it on by default on ign-cmake3 but with that feature in place we call it ready to be used with ign-cmake2 like in this case. Added to the roadmap in gazebosim/gz-cmake#228

Ah I didn't remember that option, @mjcarroll . Maybe we can just use that on the action then. That should be easier than changing ign-cmake

+1 .

@chapulina
Copy link
Contributor Author

@j-rivero , isn't 4e36f6e redundant as of gazebo-tooling/action-gz-ci#51?

@j-rivero
Copy link
Contributor

j-rivero commented Apr 8, 2022

@j-rivero , isn't 4e36f6e redundant as of ignition-tooling/action-ignition-ci#51?

It is. Both are correct I would say.

@chapulina chapulina merged commit b8eae6b into ign-gazebo6 Apr 12, 2022
@chapulina chapulina deleted the chapuilna/6/jammy branch April 12, 2022 16:02
chapulina added a commit that referenced this pull request Apr 14, 2022
Signed-off-by: Louise Poubel <[email protected]>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress tests Broken or missing tests / testing infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants