-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add arch-specific library folders (#154) #156
Conversation
Which package do I need to install to run nosetests? |
|
Ok, I have that installed, but when I do |
That command was recommended in #90 |
oops indentation... |
What operating system? |
Ubuntu trusty |
I can run |
It was a venv problem; it's working now. |
With python 2.7 on trusty, starting in catkin_tools source repo root:
That mostly works, but some tests complain about not being able to find catkin. There's a few steps in the |
Yeah, you'll need to install catkin and source the setup file for it. |
python3 failure: any advice?
|
That error happens because the contents of
|
I don't need to declare an explicit type for decode? |
I did that in 3833087 after seeing stack overflow |
It defaults to |
I'll squash after code review unless otherwise directed. |
# this function returns the suffix for lib directories on supported systems or an empty string | ||
# it uses two step approach to look for multiarch: first run gcc -print-multiarch and if | ||
# failed try to run dpkg-architecture | ||
p = subprocess.Popen( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These calls to subprocess.Popen
can raise a OSError
or a FileNotFoundError
if the executable you gave does not exist. You may need to catch that. You can test it by changing gcc
to gcc_does_not_exist
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to either check that the executable exists before calling the subprocess or by catching these types of errors to be more graceful when there is an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that these raise errors. Will fix when I have a chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrapped both Popen calls in try/except blocks. How does it look now?
+1 other than comments. |
['dpkg-architecture', '-qDEB_HOST_MULTIARCH'], | ||
stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate() | ||
except: | ||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can you either make this return ''
or update the above return ''
to also be return ""
?
+1 After the two nitpicks, I'll merge. |
['gcc', '-print-multiarch'], | ||
stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
out, err = p.communicate() | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was gonna let this pass, but it is bothering me...
It's not good practice to catch and silently pass on all exceptions. I know it's more work, but it would be better if you caught specific errors, something like this: except (OSError, FileNotFoundError):
Try to detect arch-specific library folders using gcc and dpkg-architecture, then add to LD_LIBRARY_PATH and PKG_CONFIG_PATH. Closes catkin#154. Based on ros/catkin#624
I fixed the last three comments and then squashed |
Lgtm, I want to fixup something else, but I'll do that after the merge. |
Add arch-specific library folders (#154)
Never mind, I was thinking |
Thanks for the review. I'll start recommending this to the gazebo developers once it is released. |
Try to detect arch-specific library folders using
gcc and dpkg-architecture, then add to
LD_LIBRARY_PATH
andPKG_CONFIG_PATH
.Closes #154.
Based on ros/catkin#624