Add catkin_lint feature. #219

130s · 2017-10-17T00:53:59Z

An enhancement mentioned in #98

Some considerations:

By default catkin_lint in this PR is run with --strict option, which makes the job fail even when there's only warning returned by catkin_lint. I'm not sure if we want to allow disabling --strict option on CI (those who just want the result without failing the CI job can run catkin_lint locally IMO).
catkin_lint runs between build-install and tests, which I'm not sure is the best location.

130s · 2017-10-17T02:10:17Z

Some CI jobs for industrial_ci (e.g. for Debian Jessie) fail where python-catkin-lint isn't available (from apt). For Debian it's tracked already Debian packaging fkie/catkin_lint#36 but no progress seems to have been made for half an year.
All CI jobs for the repos used for downstream tests (industrial_core, motoman_experimental, ros_control) failed due to catkin_lint emitting errors/warnings. Allowing failures for those jobs is easy but that could blind us for other potential issues in the future.

I don't know if we want to allow_failure those failing jobs just for catkin_lint.

gavanderhoorn · 2017-10-17T20:18:17Z

@130s wrote:

All CI jobs for the repos used for downstream tests (industrial_core, motoman_experimental, ros_control) failed due to catkin_lint emit errors/warnings.

You almost make me feel bad ;)

In any case: I don't think motoman_experimental is really a good package for testing industrial_ci. It's not required to be stable and may contain pkgs that don't follow conventions / are in a flux, etc.

But +1000 for catkin_lint btw. The failed tests shows me quite some things that need fixing in that repository.

🙇

mathias-luedtke · 2017-10-17T20:30:12Z

Great!

Some CI jobs for industrial_ci (e.g. for Debian Jessie) fail where python-catkin-lint isn't available (from apt).

I would suggest to install catkin_lint only if CATKIN_LINT=true.
In addition sudo pip install catkin_lint should work for all platforms.
(Please make this is separate commit, this enables us to revert it later on)

I don't know if we want to allow_failure those failing jobs just for catkin_lint.

Nope ;)

catkin_lint runs between build-install and tests, which I'm not sure is the best location.

I would place it between rosdep install and catkin build.

All CI jobs for the repos used for downstream tests (industrial_core, motoman_experimental, ros_control) failed due to catkin_lint emit errors/warnings.

We should run a test for the industrial_ci package (if it fails, we should fix it) and a failing test with a mock up package.
This way we do not depend on actual packages, which evenually will fix their problems.

By default catkin_lint is run with --strict option,

The --strict option is a no-op for the default settings.
IMHO it should be possible warn about some problems, but to fail only for errors.
However, the --explain option is a good candidate to be passed for all runs.

mathias-luedtke · 2017-10-17T20:34:09Z

industrial_ci/src/tests/source_tests.sh

@@ -158,6 +158,12 @@ if [ "$BUILDER" == catkin ]; then catkin build $OPT_VI --summarize  --no-status

 ici_time_end  # catkin_build

+if [ "$CATKIN_LINT" == "true" ]; then
+    ici_time_start catkin_lint
+    catkin_lint --strict $CATKIN_LINT_ARGS $CATKIN_WORKSPACE/src && echo "catkin_lint passed." || error "`catkin_lint --strict` failed by either/both errors and/or warnings"


IMHO the echo ouputs are superfluous, the fold will highlight the error anyway.

If you check $CATKIN_WORKSPACE/src, the rosinstall packages will get included.
It might be better to check only "$CATKIN_WORKSPACE/src/$(basename $TARGET_REPO_PATH)"

Or simply "$TARGET_REPO_PATH".

the echo ouputs are superfluous, the fold will highlight the error anyway.

echo is called only upon success, so no problem?

It's is harder to read / maintain and the ouput does not add any information
So I would remove it, but I leave the decision to you.

I found the output of catkin_lint is often not clearly telling me if it passed or not, e.g. with the following that includes a line from my commit (catkin_lint passed.), if you're not that used to the tool then it could be hard to tell without my commit. With folded in green-color, most of the cases you might not even have to unfold the section, but I'd still prefer to being explicit.

https://travis-ci.org/ros-industrial/industrial_ci/jobs/289293486

catkin_lint: checked 1 packages and found 1 problems catkin_lint: 1 notices have been ignored. Use -W2 to see them catkin_lint passed. <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Function catkin_lint returned with code '0' after 0 min 3 sec

It's harder to read / maintain

How so?

I found the output of catkin_lint is often not clearly telling me if it passed or not

The state if encoded in "Function catkin_lint returned with code '0' after 0 min 3 sec" and the color.

It's harder to read / maintain

How so?

catkin_lint --strict $CATKIN_LINT_ARGS $CATKIN_WORKSPACE/src && echo "catkin_lint passed." || error "catkin_lint --strict failed by either/both errors and/or warnings"
(Beware: do not use the backticks in the string, it is the same as $()!)

vs.

catkin_lint --strict $CATKIN_LINT_ARGS $CATKIN_WORKSPACE/src

An enhancement mentioned in ros-industrial#98 Some considerations: - By default `catkin_lint` is run with `--strict` option, which makes the job fail even when there's only warning returned by catkin_lint. I'm not sure if we want to allow disabling --strict option on CI (those who just want the result without failing the CI job can run catkin_lint locally IMO). - `catkin_lint` runs between build-install and tests, which I'm not sure is the best location.

130s · 2017-10-18T00:37:43Z

Thanks @ipa-mdl @gavanderhoorn for the feedback, all of which should be now addressed.

mathias-luedtke · 2017-10-18T08:33:36Z

industrial_ci/src/tests/source_tests.sh

@@ -149,6 +149,13 @@ set +o pipefail

 ici_time_end  # rosdep_install

+if [ "$CATKIN_LINT" == "true" ]; then
+    ici_time_start catkin_lint
+    sudo apt-get install -qq -y python-catkin-lint || error


|| error is not needed, the script will fail on all errors.
Please use the pip version for compatibility with Debian.

mathias-luedtke · 2017-10-18T08:36:07Z

industrial_ci/src/tests/source_tests.sh

+if [ "$CATKIN_LINT" == "true" ]; then
+    ici_time_start catkin_lint
+    sudo apt-get install -qq -y python-catkin-lint || error
+    catkin_lint --explain $CATKIN_LINT_ARGS $TARGET_REPO_PATH && echo "catkin_lint passed." || error "`catkin_lint --strict` failed by either/both errors and/or warnings"


error message still contains --strict ;)

mathias-luedtke · 2017-10-18T08:44:57Z

.travis.yml

@@ -56,7 +56,7 @@ env:
    - ROS_DISTRO=kinetic OS_NAME=debian OS_CODE_NAME=xenial EXPECT_EXIT_CODE=1
    - ROS_DISTRO=kinetic DOCKER_BASE_IMAGE="ros:kinetic-ros-base" NOT_TEST_BUILD='true' DEBUG_BASH='true' VERBOSE_OUTPUT='false'
    - ROS_DISTRO=kinetic APTKEY_STORE_SKS=hkp://ha.pool.sks-keyservers.vet  # Passing wrong SKS URL as a break test. Should still pass.
-    - ROS_DISTRO=lunar ROS_REPO=ros-shadow-fixed USE_MOCKUP='industrial_ci/mockups/industrial_ci_testpkg'
+    - ROS_DISTRO=lunar ROS_REPO=ros-shadow-fixed USE_MOCKUP='industrial_ci/mockups/industrial_ci_testpkg' CATKIN_LINT=true


industrial_ci_testpkg should get patched to issue a warning, so we can add a supposed-to-fail test using the `--strict' flag.

Do you mean we should use something else, not industrial_ci_testpkg for checking catkin_lint feature? If so do you have a suggestion?

Do you mean we should use something else, not industrial_ci_testpkg for checking catkin_lint feature? If so do you have a suggestion?

No, that's fine.
But I want to add another teststhat fails on purpose to see that catkin_lint was actually run for the given situation.

mathias-luedtke · 2017-10-18T16:18:26Z

industrial_ci/src/tests/source_tests.sh

@@ -149,6 +149,14 @@ set +o pipefail

 ici_time_end  # rosdep_install

+if [ "$CATKIN_LINT" == "true" ]; then
+    ici_time_start catkin_lint
+    sudo apt-get install -qq -y python-pip


python-pip should already be installed in our images.
I am undecided if we want to enforce it here again.

mathias-luedtke · 2017-10-18T16:56:48Z

industrial_ci/src/tests/source_tests.sh

-    sudo apt-get install -qq -y python-catkin-lint || error
-    catkin_lint --explain $CATKIN_LINT_ARGS $TARGET_REPO_PATH && echo "catkin_lint passed." || error "`catkin_lint --strict` failed by either/both errors and/or warnings"
+    sudo apt-get install -qq -y python-pip
+    sudo pip install python-catkin-lint


sudo pip install catkin_lint

mathias-luedtke · 2017-10-18T21:48:48Z

What do you think of CATKIN_LINT=pedantic as a shortcut for CATKIN_LINT=true CATKIN_LINT_ARGS="--strict -W2"?

…ness. Small fixes.

130s · 2017-10-19T19:51:50Z

@ipa-mdl All your comments and suggestions should be addressed. Please review again.

mathias-luedtke

Looks great!

I have modified the pedantic test to expect to fail.
However, I did not want to sacrify our only lunar test, so I have duplicated the other.

If the the job passes, just go ahead to squash and/or merge :)

130s · 2017-10-20T13:20:23Z

I have modified the pedantic test to expect to fail.
However, I did not want to sacrify our only lunar test, so I have duplicated the other.

+1
Thanks for adding the nice finish.

130s requested a review from mathias-luedtke October 17, 2017 00:54

130s force-pushed the add/catkin_lint branch 4 times, most recently from a86b458 to d6ef730 Compare October 17, 2017 01:38

130s force-pushed the add/catkin_lint branch 2 times, most recently from c6f197f to fabbcef Compare October 17, 2017 02:55

130s changed the title ~~WIP: Add catkin_lint feature.~~ Add catkin_lint feature. Oct 17, 2017

gavanderhoorn mentioned this pull request Oct 17, 2017

catkin_lint is not happy ros-industrial/motoman_experimental#29

Closed

mathias-luedtke reviewed Oct 17, 2017

View reviewed changes

130s added 2 commits October 17, 2017 17:34

[catkin_lint] Run against industrial_ci only.

6f7c45d

130s force-pushed the add/catkin_lint branch from fabbcef to d170b80 Compare October 18, 2017 00:35

mathias-luedtke mentioned this pull request Oct 18, 2017

CI consolidation roadmap #98

Closed

26 tasks

mathias-luedtke requested changes Oct 18, 2017

View reviewed changes

mathias-luedtke reviewed Oct 18, 2017

View reviewed changes

mathias-luedtke mentioned this pull request Oct 18, 2017

Address catkin_lint errors/warnings ros-industrial/ros_canopen#254

Merged

[catkin_lint] Use pip to install catkin-lint for more cross-platform-…

0d05593

…ness. Small fixes.

130s force-pushed the add/catkin_lint branch from 741ca12 to c845fdf Compare October 19, 2017 18:05

[catkin_lint] Add pedantic option.

adca709

130s force-pushed the add/catkin_lint branch from c845fdf to adca709 Compare October 19, 2017 19:19

fix CATKIN_LINT=pedantic test

33be4ab

mathias-luedtke approved these changes Oct 19, 2017

View reviewed changes

130s merged commit 2308a63 into ros-industrial:master Oct 20, 2017

130s deleted the add/catkin_lint branch October 20, 2017 13:20

130s mentioned this pull request Jul 11, 2018

Run additional tests (non ROS) after build #58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add catkin_lint feature. #219

Add catkin_lint feature. #219

130s commented Oct 17, 2017 •

edited

Loading

130s commented Oct 17, 2017 •

edited

Loading

gavanderhoorn commented Oct 17, 2017

mathias-luedtke commented Oct 17, 2017 •

edited

Loading

mathias-luedtke Oct 17, 2017 •

edited

Loading

mathias-luedtke Oct 17, 2017 •

edited

Loading

mathias-luedtke Oct 17, 2017

130s Oct 18, 2017

mathias-luedtke Oct 18, 2017

130s Oct 18, 2017 •

edited

Loading

mathias-luedtke Oct 18, 2017

130s commented Oct 18, 2017

mathias-luedtke Oct 18, 2017

mathias-luedtke Oct 18, 2017

mathias-luedtke Oct 18, 2017

130s Oct 18, 2017

mathias-luedtke Oct 18, 2017 •

edited

Loading

mathias-luedtke Oct 18, 2017

mathias-luedtke Oct 18, 2017

mathias-luedtke commented Oct 18, 2017

130s commented Oct 19, 2017

mathias-luedtke left a comment •

edited

Loading

130s commented Oct 20, 2017

Add catkin_lint feature. #219

Add catkin_lint feature. #219

Conversation

130s commented Oct 17, 2017 • edited Loading

130s commented Oct 17, 2017 • edited Loading

gavanderhoorn commented Oct 17, 2017

mathias-luedtke commented Oct 17, 2017 • edited Loading

mathias-luedtke Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

mathias-luedtke Oct 17, 2017 • edited Loading

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

130s Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

130s commented Oct 18, 2017

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

mathias-luedtke Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathias-luedtke commented Oct 18, 2017

130s commented Oct 19, 2017

mathias-luedtke left a comment • edited Loading

Choose a reason for hiding this comment

130s commented Oct 20, 2017

130s commented Oct 17, 2017 •

edited

Loading

130s commented Oct 17, 2017 •

edited

Loading

mathias-luedtke commented Oct 17, 2017 •

edited

Loading

mathias-luedtke Oct 17, 2017 •

edited

Loading

mathias-luedtke Oct 17, 2017 •

edited

Loading

130s Oct 18, 2017 •

edited

Loading

mathias-luedtke Oct 18, 2017 •

edited

Loading

mathias-luedtke left a comment •

edited

Loading