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

thorough rework of the Travis CI scripts #44

Closed
wants to merge 13 commits into from
Closed

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jan 25, 2019

Being started from fixing several basic issues in moveit_ci scripts to make #42 and #43 work, I ended up with a thorough rework of the basic moveit_ci scripts in util.sh based on most decent scripts at travis-build.

Issues resolved:

  • BEFORE_SCRIPTS didn't run correctly for a while. For example here testing should be printed, but it's not. I fixed this, by evaluating commands passed to travis_run_*.
  • Getting clang-tidy running wasn't trivial as well, e.g. putting .clang-tidy into the correct location and convincing Travis to not bail out.
  • Building and running tests wasn't executed with travis_run_wait, i.e. timeout and . generation.

Improvements:

  • The key improvements are in f3ac41d
    • better modularization
      • travis_run_simple: no folding
      • travis_run_true: folding, timing, accept failures
      • travis_run: folding, timing, not accepting failures
      • travis_run_wait: +timeout
    • support hierarchical folds (with custom fold names and titles)
    • travis_wait() to wait for a background process to finish
    • limit travis_run_wait's timeout to estimated remaining Travis' build time
      When we hit Travis' hard timeout, caches are not saved by Travis anymore. Hence we need to bail out before Travis to get our caches saved (and thus have a chance on the next build to meet the timeout deadline). To this end, I estimate the remaining build job time and compare against the configurable variable MOVEIT_CI_TRAVIS_TIMEOUT, which defaults to 45min. Although by default open-source projects at http://travis-ci.org have a timeout of 50 mins, a have put some safety-margin in, because booting Travis' virtual machine, loading caches, etc. already consumes time that I cannot measure.
    • filter() and filter-out() utility functions

This whole PR is coming with a whole new bash-based unittest suite to validate correctness of all bash functions.

All smaller commits contain independent and self-explanatory changes, which is why this PR should be merge-committed as discussed in moveit/moveit_tutorials#280 (comment) ;-)

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 25, 2019

Looks like Travis people have reduced the timeout to 50min. I have two builds timing out after 50min:
https://travis-ci.org/ros-planning/moveit_ci/builds/484079731
https://travis-ci.org/ros-planning/moveit_ci/builds/484101958

The tutorials contribute about 5min to the build time.

@rhaschke rhaschke force-pushed the clang-tidy-check branch 3 times, most recently from 150ae5c to 13e9106 Compare January 25, 2019 07:56
@rhaschke
Copy link
Contributor Author

Closing for now, as there are still Travis issues.

@rhaschke rhaschke closed this Jan 25, 2019
@davetcoleman
Copy link
Member

Probably not related, but one thing to keep in mind:

If no output is received from a build for 10 minutes, it’s assumed to have stalled for unknown reasons and is subsequently killed.

https://docs.travis-ci.com/user/common-build-problems/

Also, in 2017 I contacted Travis and they increased it:

We have increased the maximum timeout for the ros-planning/moveit repository to 70 minutes, please let us know if that would be enough.

I just emailed them asking to restore this, maybe it was decreased again?

@rhaschke
Copy link
Contributor Author

If no output is received from a build for 10 minutes, it’s assumed to have stalled for unknown reasons and is subsequently killed.

I know and I'm taking this into account.

In 2017 Travis people increased the timeout.

That's what I had in my mind as well. However, as Travis people are preparing migration of open source projects to travis-ci.com, they obviously reduced the timeout accidentally again.

I contacted them today already and they already increased the limit to 90 minutes. They are incredibly responsive!

@davetcoleman
Copy link
Member

Awesome! So can this CL be re-opened?

One thing I was thinking about is we could have some of the upper-level packages built separately:

Group 1:

  • All the core functionality, planners, etc

Group 2:

  • Minimum core functionality
  • All the visualization tools

But it would be hard to maintain if dependencies change

@rhaschke
Copy link
Contributor Author

So can this CL be re-opened?

Not yet. I still need to fix clang-tidy.

@rhaschke rhaschke changed the title several fixes to the travis CI scripts thorough rework of the Travis CI scripts Jan 27, 2019
@rhaschke
Copy link
Contributor Author

@davetcoleman I think this is ready for review and merging now. Please note the updated main #44 (comment). I essentially reworked the foundations of moveit_ci - the travis_* functions.
We now have new tests for clang-tidy (#12), warnings, and catkin_lint (#11). It's up to us, to enable them ;-)
I think the testing code became much more readible. It's astonishing what you can achieve with bash programming.

@BryceStevenWilley Please have a look at the test results, particularly the MoveIt build: Question: Do these changes look familiar to you?

@BryceStevenWilley
Copy link

Overall this looks good.

  1. Most of the clang-tidy warnings are outside of moveit_core, which so far is the only part of the codebase that's been tidied, so that's expected. There are a few warnings in moveit_core that I don't remember seeing, such as this and this. Maybe the new clang-tidy version that reports these, and the old one misses them? I can't see any other obvious reason.

  2. On the MoveIt build you linked:

The command "catkin build --no-status --summarize" reached the internal timeout of 45 minutes. Aborting.

Even with Travis's help, catkin isn't cooperating. Maybe running catkin build and clang-tidy separately again would be better?

  1. Any reason you didn't copy over the identifier naming rules from the MoveIt repo?

@davetcoleman
Copy link
Member

I made the review of the new PR. Let's keep move this conversation there.

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