Skip to content

Cache workspaces [ros2]#50

Open
tylerjw wants to merge 10 commits into
moveit:ros2from
tylerjw:cache-workspaces
Open

Cache workspaces [ros2]#50
tylerjw wants to merge 10 commits into
moveit:ros2from
tylerjw:cache-workspaces

Conversation

@tylerjw

@tylerjw tylerjw commented Mar 22, 2021

Copy link
Copy Markdown
Member

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

@codecov

codecov Bot commented Mar 22, 2021

Copy link
Copy Markdown

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.

@rhaschke rhaschke left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread README.md Outdated
Comment thread .github/workflows/industrial_ci_action.yml Outdated
Comment thread .github/workflows/industrial_ci_action.yml
Comment thread .github/workflows/industrial_ci_action.yml Outdated
Comment thread .github/workflows/industrial_ci_action.yml Outdated
@tylerjw

tylerjw commented Mar 22, 2021

Copy link
Copy Markdown
Member Author

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

tylerjw commented Mar 22, 2021

Copy link
Copy Markdown
Member Author

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

@tylerjw

tylerjw commented Mar 22, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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.

@rhaschke rhaschke left a comment

Copy link
Copy Markdown
Contributor

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

tylerjw commented Mar 23, 2021

Copy link
Copy Markdown
Member Author

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

@rhaschke rhaschke left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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/always-upload-cache@v2.1.3
with:
path: ${{ env.BASEDIR }}/target_ws
key: target_ws-${{ env.CACHE_PREFIX }}-${{ hashFiles('**/CMakeLists.txt') }}-${{ hashFiles('**/package.xml') }}-${{ github.run_id }}

Copy link
Copy Markdown
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