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

Remove -x in python distutils shell script #755

Merged
merged 2 commits into from
Sep 23, 2015
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Sep 21, 2015

This prevents extraneous output in the installation step. According to the man page:

http://heirloom.sourceforge.net/sh/sh.1.html

-x Print commands and their arguments as they are executed.

This is the same as activating BASH_XTRACEFD (http://linux.die.net/man/1/sh). This is the type of output it prevents:

+ '[' -n '' ']'
+ cd /Users/william/jade/src/catkin
+ /usr/bin/env PYTHONPATH=/Users/william/jade/install/lib/python2.7/site-packages:/Users/william/jade/build/catkin/lib/python2.7/site-packages: CATKIN_BINARY_DIR=/Users/william/jade/build/catkin /usr/bin/python /Users/william/jade/src/catkin/setup.py build --build-base /Users/william/jade/build/catkin install --prefix=/Users/william/jade/install --install-scripts=/Users/william/jade/install/bin

The reason I noticed and tracked it down was because this is also output on stderr, which the catkin_tools command picked up on and reported as a warning (perhaps incorrectly).

This prevents extraneous output in the installation step.
@tfoote
Copy link
Member

tfoote commented Sep 21, 2015

I'm not sure I'd call the outputs extraneous. That information is there so that you can see what exactly the build is running. There's no other way to get visibility into the internal state of this script unless it prints it out. This allows someone debugging the build to be able to reproduce it quite easily. Otherwise you'd only see the output of setup.py and never know what arguments or paths were passed to it when it was run.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 22, 2015

@tfoote then we should use echo's as not to output on stderr for normal behavior, or run set -x only when a trace or debug environment variable is set, something like that.

Also, at least the first line or output is not very useful and honestly more confusing than helpful if you don't know where it is coming from: + '[' -n '' ']'

@dirk-thomas
Copy link
Member

Using echo to print the major command instead of -x seems to be a good idea.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 22, 2015

Ok, I added 8e2a948, but I'm still testing it in a bigger workspace now, and we should test it on Linux too.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 23, 2015

Seems to work for me on Linux and OS X.

@tfoote
Copy link
Member

tfoote commented Sep 23, 2015

+1

@dirk-thomas
Copy link
Member

+1 I can confirm that the new messages go to stdout and stderr stay clean.

Since catkin has just been released into Indigo and Jade it will take a while until this patch appears in a patch release though.

dirk-thomas added a commit that referenced this pull request Sep 23, 2015
Remove -x in python distutils shell script
@dirk-thomas dirk-thomas merged commit 3b8a3b9 into indigo-devel Sep 23, 2015
@dirk-thomas dirk-thomas deleted the wjwwood-patch-1 branch September 23, 2015 23:30
dirk-thomas added a commit that referenced this pull request Jan 25, 2016
cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
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