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

reduce verbosity #48

Closed
wants to merge 5 commits into from
Closed

reduce verbosity #48

wants to merge 5 commits into from

Conversation

rhaschke
Copy link
Contributor

Travis has a log limit in the web interface (10k lines), which is easily exceeded by MoveIt. This PR disables shell debugging when sourcing ROS' setup.bash and removes --verbose mode from catkin calls.

The first commit 7276921 removes a git diff command that causes the following error:

++++git diff origin/master FETCH_HEAD .ci_config
fatal: ambiguous argument 'origin/master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

What was the purpose of this?

@130s
Copy link
Member

130s commented May 15, 2016

Thanks for the feedback. I'll have to come back later to take a closer look but would #43 work for you in terms of verbosity?

@rhaschke
Copy link
Contributor Author

Thinking more about the issue, I came to the conclusion that it is hard to server the two goals of i) printing the commands from travis.sh but ii) not printing the commands in called functions or sourced scripts.
The output in the web interface is cluttered already in the overview (with travis_time_start, set +x, and <<< >>>. Hence, for MoveIt, I resumed to standard travis.yml files, improving them according to some inspiration gained from your script. I like the result much better - but that's my subjective view of course.

Regarding your work, I have the following suggestions:

  • Consider removing the options -i -v completely. The new catkin tools 0.4 does a pretty good job of hiding boring information (everything works as expected) but showing important stuff (warnings and errors).
  • Consider building to install space directly. I don't see a reason why to build twice, for devel and install space. This only doubles the response time of the test.

@130s 130s mentioned this pull request May 16, 2016
@130s
Copy link
Member

130s commented May 18, 2016

I came to the conclusion that it is hard to server the two goals of i) printing the commands from travis.sh but ii) not printing the commands in called functions or sourced scripts.
The output in the web interface is cluttered already in the overview (with travis_time_start, set +x, and <<< >>>. Hence, for MoveIt, I resumed to standard travis.yml files, improving them according to some inspiration gained from your script. I like the result much better - but that's my subjective view of course.

That's a valuable feedback. I like to integrate those to industrial_ci, which is aiming to free package maintainers from maintaining CI configurations, unless they want to.

@rhaschke
Copy link
Contributor Author

I like to integrate those to |industrial_ci|, which is aiming to free
package maintainers from maintaining CI configurations, unless they
want to.
Indeed, this approach is appealing and I like it. But one needs to find
a better way to integrate with the Travis config. What about including
yml into yml? Is that possible?

@130s
Copy link
Member

130s commented May 27, 2016

Thanks for the PR and also suggestions. I'm following them up in #54 and #55.

Speaking of the number of the lines, I think #43 is already doing a good job and a nice thing about #43 is it gives verbose option so that if you need more output you always can print more (good for debugging).

So I close this for now in favor of #43, but please feel free to reopen if you have further updates.

@130s 130s closed this May 27, 2016
@rhaschke rhaschke deleted the reduce-verbosity branch May 27, 2016 10:17
@davetcoleman
Copy link
Contributor

Consider removing the options -i -v completely. The new catkin tools 0.4 does a pretty good job of hiding boring information (everything works as expected) but showing important stuff (warnings and errors).

+1

@davetcoleman
Copy link
Contributor

I like the changes in this closed PR...

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.

None yet

3 participants