several improvements #33

rhaschke · 2018-04-28T15:38:24Z

This PR is actually a merge of several branches (to resolve conflicts).

Enabling assertions in Travis build - replacement for Use debug build #25
To enable assertions, Use debug build #25 originally suggested a Debug build. However, this unneccessarily slows down the unit tests, thus running out of Travis time.
My suggestion is to just enable assertions, but keep optimizations.
~~Running clang-format directly within Travis - replacement for run clang-format outside docker container #31.~~
The clang-format check doesn't need to be run within a docker container, which is more time-consuming and requires to define variables ROS_DISTRO and ROS_REPO.
General improvements
- Some fixes to the utility functions in util.sh
- Enable folding and timing for the main build job too.
- Only run the full MoveIt! build once. All other test cases can be done with an empty .rosinstall.
- rename my_travis_wait -> travis_run_wait
Use xvfb to allow for X11-based unittests - replacement for Allow (simulated) display to be used with ci builds #1
New attempt to get rid of weird message - replacement for Fix travis_wait command #26 / cleanup my_travis_wait #32
/root/moveit/.moveit_ci/util.sh: line 136: 677 Terminated travis_jigger $! $timeout $cmd
This message originates from bash's job control and can be suppressed as discussed here.
This works for me locally, but not in Travis.

I suggest to manually push-forward the master branch to keep the branch history.

davetcoleman

Overall looks like great improvements!

davetcoleman · 2018-04-30T22:05:33Z

.travis.yml

 script:
-  - source .moveit_ci/travis.sh # this is intended only for the repo moveit_ci, other repos should use the .travis.yml script in the README.md
+  - ./travis.sh # this is intended only for the repo moveit_ci, other repos should use the .travis.yml script in the README.md


changing this will break all the repos using moveit_ci currently - is this necessary? i don't see what the advantage of this is. there could be some that are not under ros-planning

No changes needed for repositories using moveit_ci. However, we can use the same syntax as in external repos here too and I will change this accordingly.

davetcoleman · 2018-04-30T22:06:29Z

README.md

    - ROS_DISTRO=kinetic  ROS_REPO=ros              UPSTREAM_WORKSPACE=https://raw.githubusercontent.com/ros-planning/moveit/kinetic-devel/moveit.rosinstall
    - ROS_DISTRO=kinetic  ROS_REPO=ros-shadow-fixed UPSTREAM_WORKSPACE=https://raw.githubusercontent.com/ros-planning/moveit/kinetic-devel/moveit.rosinstall
-    - TEST=clang-format


does moving this to the top will prevent other tests from running if clang fails? so this saves time?

Just nicer formatting. No functional implications.

davetcoleman · 2018-04-30T22:09:10Z

travis.sh

+            exit 0 # This runs as an independent job, do not run regular travis test
+        ;;
+    esac
+done


by skipping docker you run the risk that the ubuntu version that travis is using could be out of date (it was 12.04 for a very long time) and that the exact command isn't available on certain OSes:

apt-get -qq install -y clang-format-3.8

I'm not convinced loading a docker container for this is that big a cost... they are cached on Travis anyway and pre-built by dockerhub

It's a minute extra or so. Sure, not that big a cost.

Also, you don't need to specify ROS_DISTRO and ROS_REPO environment variables needed to run the docker container.

Nevertheless, I reverted this part.

davetcoleman · 2018-04-30T22:09:19Z

travis.sh

+travis_run apt-get -qq install xvfb mesa-utils
+Xvfb -screen 0 640x480x24 :99 &
+export DISPLAY=:99.0
+travis_run_true glxinfo


davetcoleman · 2018-04-30T22:10:01Z

travis.sh

@@ -145,13 +151,13 @@ travis_run rosdep install -y -q -n --from-paths . --ignore-src --rosdistro $ROS_
 travis_run cd $CATKIN_WS

 # Configure catkin
-travis_run catkin config --extend /opt/ros/$ROS_DISTRO --install --cmake-args -DCMAKE_BUILD_TYPE=Release
+travis_run catkin config --extend /opt/ros/$ROS_DISTRO --install --cmake-args -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"


whats the logic for building in very optimized mode? my understanding is that it is risky

That's the default of cmake, but with -DNDEBUG removed.

davetcoleman · 2018-04-30T22:11:01Z

util.sh


-  let "TRAVIS_FOLD_COUNTER += 1"
+#######################################
+# Same as travis_run except ignores errors and does not break build


update comment

davetcoleman · 2018-04-30T22:14:22Z

util.sh


  return $result
 }

 #######################################
-function my_travis_jigger() {
-  # helper method for travis_wait()
+function travis_jigger() {


i'm nervous there might be a naming conflict with travis' default scripts, but i can't remember if they have a travis_jigger. i found various similar functions though:
https://github.com/matomo-org/travis-scripts/blob/master/travis-helper.sh#L34

something to look out for

It worked fine.

rhaschke · 2018-05-01T06:43:04Z

@davetcoleman I addressed your comments. If you approved, I will move the cleanup commits into their respective branches and perform the final merge.

davetcoleman · 2018-05-01T14:10:17Z

I'm not sure what you mean by "move cleanup commits to respective branches"... merging this single PR seems fine to me...?

This reverts commit 19dcebc.

- use local variables where possible - correctly unset variables TRAVIS_START_TIME, TRAVIS_TIME_ID, TRAVIS_FOLD_NAME - travis_run_true as variant of travis_run

https://stackoverflow.com/questions/11097761/is-there-a-way-to-make-bash-job-control-quiet

rhaschke · 2018-05-01T15:38:02Z

@davetcoleman Thanks for approving. I just re-ordered the commits as I like to the commit history to be grouped semantically and not by time.

This was referenced Apr 28, 2018

Use debug build #25

Closed

Allow (simulated) display to be used with ci builds #1

Closed

rhaschke requested a review from davetcoleman April 28, 2018 15:49

This was referenced Apr 28, 2018

improve travis checks moveit/moveit#871

Merged

run clang-format outside docker container #31

Closed

davetcoleman requested changes Apr 30, 2018

View reviewed changes

davetcoleman approved these changes May 1, 2018

View reviewed changes

rhaschke added 14 commits May 1, 2018 17:28

simplify local .travis.yml

66d0c03

update README

83b3ab7

enable assertions for travis build

1385b32

run clang-format outside docker container

762c7d8

Revert "run clang-format outside docker container"

cf94ebb

This reverts commit 19dcebc.

fix checked file name

0d41af3

simplify my_travis_wait

e7365a2

cleanup utility functions

e94f802

- use local variables where possible - correctly unset variables TRAVIS_START_TIME, TRAVIS_TIME_ID, TRAVIS_FOLD_NAME - travis_run_true as variant of travis_run

enable folding and timing for commands run by my_travis_wait

e0819d2

dummy rosinstall file

a85e2dd

rename my_travis_wait -> travis_run_wait

ac5d592

Suppress bash's job control messages

5590130

https://stackoverflow.com/questions/11097761/is-there-a-way-to-make-bash-job-control-quiet

run xvfb to allow for X11-based unittests

57e9e45

Merge branches 'assertions', 'clang-format', 'my_travis_wait' and 'xvfb'

31cd910

rhaschke force-pushed the master branch from 03cf71f to 31cd910 Compare May 1, 2018 15:32

rhaschke merged commit 31cd910 into moveit:master May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

several improvements #33

several improvements #33

rhaschke commented Apr 28, 2018 •

edited

Loading

davetcoleman left a comment

davetcoleman Apr 30, 2018

rhaschke May 1, 2018

davetcoleman Apr 30, 2018

rhaschke May 1, 2018

davetcoleman Apr 30, 2018

rhaschke May 1, 2018

rhaschke May 1, 2018

rhaschke May 1, 2018

davetcoleman Apr 30, 2018

davetcoleman Apr 30, 2018

rhaschke May 1, 2018

davetcoleman Apr 30, 2018

davetcoleman Apr 30, 2018

rhaschke May 1, 2018

rhaschke commented May 1, 2018

davetcoleman commented May 1, 2018

rhaschke commented May 1, 2018

several improvements #33

several improvements #33

Conversation

rhaschke commented Apr 28, 2018 • edited Loading

davetcoleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaschke commented May 1, 2018

davetcoleman commented May 1, 2018

rhaschke commented May 1, 2018

rhaschke commented Apr 28, 2018 •

edited

Loading