search

git-afsantos / haros

H(igh) A(ssurance) ROS - Static analysis of ROS application code.
MIT License
190 stars 37 forks source link

Should rosgraph be added to requirements.txt? #64

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

That package will probably always be installed when users have ROS installed, but if requirements.txt is supposed to capture the Python modules necessary to be able to run haros, then it would seem that rosgraph should be listed.

rospkg is listed:

https://github.com/git-afsantos/haros/blob/2196db1a34c1e862d7640d6d85eb404319f17048/requirements.txt#L2

On systems with ROS installed, pip will probably detect it and decide that the dependency has already been satisfied. On systems without, it will be installed.

gavanderhoorn commented 5 years ago

It's used here:

https://github.com/git-afsantos/haros/blob/c2ecbfa432fafcc7adbde543e205439be00f8d2e/haros/launch_parser.py#L30

git-afsantos commented 5 years ago

Makes sense to me. Thanks for the PR! :+1:

git-afsantos commented 5 years ago

Hm, after trying this, pip complains that it cannot find a suitable version for rosgraph (I have also added rosgraph to setup.py). It is probably under a different name.

gavanderhoorn commented 5 years ago

It may even be that rosgraph is not released on pypi :(

I should've checked better (doing too many things at the same time right now).

git-afsantos commented 5 years ago

I cannot find it. Unless it is bundled within some other distribution, I think it may not be released individually on PyPI.

gavanderhoorn commented 5 years ago

Just checked: it's a ROS package, not a pure Python one.

Submitted #66.

Sorry for the noise and the work.

gavanderhoorn commented 5 years ago

It would still be good to have some way of expressing this dependency though.

Perhaps a try: .. except: .. around the import would be an idea? That way we at least can print a more user-friendly error message instead of an import error.

git-afsantos commented 5 years ago

Perhaps a try: .. except: .. around the import would be an idea? That way we at least can print a more user-friendly error message instead of an import error.

I was thinking about this again. I will make it a lazy import, and provide a warning and a fallback, in case it is missing.

gavanderhoorn commented 5 years ago

The fallback is perhaps not necessary, and my use-case is slightly exotic.

Even just a nicer error would be great.

git-afsantos commented 5 years ago

Addressed and fixed in #67 and #68 @gavanderhoorn. Also released as 3.3.2.