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

Cache workspaces [ros2] #50

Open
wants to merge 10 commits into
base: ros2
Choose a base branch
from
Open

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Mar 22, 2021

This is an attempt to use the caching features of GitHub Actions to further speed up builds by caching entire workspaces.

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #50 (7ee9447) into ros2 (0af3986) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             ros2      #50   +/-   ##
=======================================
  Coverage   71.91%   71.91%           
=======================================
  Files           5        5           
  Lines         242      242           
=======================================
  Hits          174      174           
  Misses         68       68           

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 0af3986...7ee9447. Read the comment docs.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I'm not yet sure about the workspace caching. Does it work as expected? As far as I can see:

  • First you restore the upstream_ws cache (if available). I hope this also restore file timestamps.
  • Next, industrial_ci updates from UPSTREAM_WORKSPACE. Hopefully, this doesn't touch files if they didn't change. Otherwise the cache would be useless.

README.md Outdated Show resolved Hide resolved
.github/workflows/industrial_ci_action.yml Outdated Show resolved Hide resolved
.github/workflows/industrial_ci_action.yml Outdated Show resolved Hide resolved
.github/workflows/industrial_ci_action.yml Outdated Show resolved Hide resolved
.github/workflows/industrial_ci_action.yml Outdated Show resolved Hide resolved
@tylerjw
Copy link
Member Author

tylerjw commented Mar 22, 2021

I'm not yet sure about the workspace caching. Does it work as expected? As far as I can see:

* First you restore the `upstream_ws` cache (if available). I hope this also restore file timestamps.

* Next, `industrial_ci` updates from `UPSTREAM_WORKSPACE`. Hopefully, this doesn't touch files if they didn't change. Otherwise the cache would be useless.

I believe it does work. It seems to reduce the time to test this package by about half.

@tylerjw
Copy link
Member Author

tylerjw commented Mar 22, 2021

For context, before this caching a CI run of this on ros2 takes about 11min, with this it takes about 4.5min.

@tylerjw
Copy link
Member Author

tylerjw commented Mar 22, 2021

For what it's worth, I started this as a way to quickly iterate on this idea as a way to speed up builds for moveit/moveit2. I'll clean this up when I'm satisfied with it and make it a PR here, although I'm not sure if the time difference here really matters.

@rhaschke
Copy link
Contributor

For what it's worth, I started this as a way to quickly iterate on this idea as a way to speed up builds for moveit/moveit2. I'll clean this up when I'm satisfied with it and make it a PR here, although I'm not sure if the time difference here really matters.

Sure. I was assuming that you take this repo as a quick-turnaround sandbox for your CI experiments.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Looks good from my side. I'm not sure you are done with experimenting though.
Please merge yourself if so.

@tylerjw
Copy link
Member Author

tylerjw commented Mar 23, 2021

I created this: moveit/moveit#2559 to show my current progress on moveit/moveit2. The newest issue I'm encountering is with running out of disk space in codecov test runs. Unfortunately that's not something I can do with this small repo. I'll clean this up and merge it. The clang-tidy script solution can nicely be tested locally so that one should be easy to do without having to use CI runs in the loop.

@tylerjw tylerjw force-pushed the cache-workspaces branch 4 times, most recently from d9c081f to 3468305 Compare March 23, 2021 19:41
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Some minor remarks, see below.

Comment on lines 13 to 19
- {
ROS_DISTRO: foxy, ROS_REPO: main, CCOV_UPLOAD: true,
CMAKE_ARGS: "-DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS='--coverage' -DCMAKE_CXX_FLAGS='--coverage'",
AFTER_RUN_TARGET_TEST: './.ci.prepare_codecov',
ADDITIONAL_DEBS: 'curl lcov grep'
IMAGE: 'foxy-ci',
CCOV: true,
AFTER_RUN_TARGET_TEST: './.ci.prepare_codecov'
}
- {
IMAGE: 'foxy-ci-testing',
}
- {ROS_DISTRO: foxy, ROS_REPO: testing, CCOV_UPLOAD: false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't mix in json syntax here (even though yaml is a superset of json). This reads much better (and is consistent) with the formatting of the remaining file:

env:
  - ROS_DISTRO: ...
    ROS_REPO: ...
  - IMAGE: ...

uses: pat-s/[email protected]
with:
path: ${{ env.BASEDIR }}/target_ws
key: target_ws-${{ env.CACHE_PREFIX }}-${{ hashFiles('**/CMakeLists.txt') }}-${{ hashFiles('**/package.xml') }}-${{ github.run_id }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can hash multiple file patterns at once: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#example-with-multiple-patterns
${{ hashFiles('**/CMakeLists.txt', '**/package.xml') }}

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.

2 participants