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

Add arch-specific library folders (#154) #156

Merged
merged 1 commit into from
Mar 12, 2015
Merged

Conversation

scpeters
Copy link
Contributor

Try to detect arch-specific library folders using
gcc and dpkg-architecture, then add to
LD_LIBRARY_PATH and PKG_CONFIG_PATH.
Closes #154.
Based on ros/catkin#624

@scpeters
Copy link
Contributor Author

Which package do I need to install to run nosetests?

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

sudo apt-get install python-nose on Ubuntu or sudo -H pip install -U nose on OS X.

@scpeters
Copy link
Contributor Author

Ok, I have that installed, but when I do python setup.py nosetests, it says error: invalid command 'nosetests'

@scpeters
Copy link
Contributor Author

That command was recommended in #90

@scpeters
Copy link
Contributor Author

oops indentation...

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

What operating system?

@scpeters
Copy link
Contributor Author

Ubuntu trusty

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

I can run python setup.py nosetests in the root of the catkin_tools directory on my VM of Trusty and on my Mac. I'm not sure why it doesn't work for you. Make sure you've installed nose for the same instance of python that you're using.

@scpeters
Copy link
Contributor Author

It was a venv problem; it's working now.

@scpeters
Copy link
Contributor Author

With python 2.7 on trusty, starting in catkin_tools source repo root:

$ virtualenv $HOME/local
$ . ~/local/bin/activate
$ python setup.py install
$ pip install argparse catkin-pkg distribute PyYAML
$ pip install nose coverage flake8 --upgrade
$ pip install mock
$ python setup.py nosetests -s

That mostly works, but some tests complain about not being able to find catkin. There's a few steps in the .travis.ymlI skipped.

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

Yeah, you'll need to install catkin and source the setup file for it.

@scpeters
Copy link
Contributor Author

python3 failure: any advice?

  File "/home/travis/build/catkin/catkin_tools/catkin_tools/verbs/catkin_build/job.py", line 159, in get_multiarch
    assert(not out.strip() or out.strip().count('-') == 2)
TypeError: Type str doesn't support the buffer API

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

That error happens because the contents of out (I guess returned from calling out to the system) is a "bytes" array which is different from a str in Python3. You can decode out before you start doing normal operations on it if you like. Something like this:

>>> import subprocess
>>> out = subprocess.check_output('ls')
>>> out
b'foo\nbar'
>>> out.count('-')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Type str doesn't support the buffer API
>>> out = out.decode()
>>> out
'foo\nbar'
>>> out.count('-')
1

@scpeters
Copy link
Contributor Author

I don't need to declare an explicit type for decode?

@scpeters
Copy link
Contributor Author

I did that in 3833087 after seeing stack overflow

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

It defaults to utf-8.

@scpeters
Copy link
Contributor Author

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(
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2015

+1 other than comments.

['dpkg-architecture', '-qDEB_HOST_MULTIARCH'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()
except:
return ""
Copy link
Member

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 ""?

@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2015

+1 After the two nitpicks, I'll merge.

['gcc', '-print-multiarch'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE)
out, err = p.communicate()
except:
Copy link
Member

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
@scpeters
Copy link
Contributor Author

I fixed the last three comments and then squashed

@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2015

Lgtm, I want to fixup something else, but I'll do that after the merge.

wjwwood added a commit that referenced this pull request Mar 12, 2015
Add arch-specific library folders (#154)
@wjwwood wjwwood merged commit 454c94c into catkin:master Mar 12, 2015
@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2015

Never mind, I was thinking FileNotFoundError wasn't available before Python 3.3, but it is in Python 2.7 apparently too.

@scpeters scpeters deleted the multiarch branch March 12, 2015 22:05
@scpeters
Copy link
Contributor Author

Thanks for the review. I'll start recommending this to the gazebo developers once it is released.

@scpeters scpeters mentioned this pull request Mar 30, 2015
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.

Catkin tools don't handle arch-specific library folders
2 participants