git-afsantos / haros

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

Editable VCS install of pyflwor? #38

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

More of a question really: the readme instructs users to install pyflwor using the following command:

pip install -e git+https://github.com/timtadh/pyflwor.git#egg=pyflwor

Is the -e really needed?

git-afsantos commented 5 years ago

I am not really sure that it is. I installed it that way because that's what the README of the project says. It might work without it, but I have not tested yet.

gavanderhoorn commented 5 years ago

Apparently there is also pyflwor-ext, which appears to be a packaged version released by an external user (ie: not the author).

Seeing as pyflwor hasn't changed since Oct 2017 and version 1.2.0 is still the most recent one, could be interesting to see whether pyflwor-ext also works.

git-afsantos commented 5 years ago

Nice, that might actually work. I will be giving it a try :+1:

gavanderhoorn commented 5 years ago

@git-afsantos: any luck?

gavanderhoorn commented 5 years ago

I've done an install using the pyflwor-ext and haros doesn't complain, but I'm also not seeing results I would expect from the analyses (#42).

If pyflwor-ext would suffice it would make distributing haros quite a bit easier :)

git-afsantos commented 5 years ago

Ok, I just tested this now, and while I think pyflwor-ext does work, it requires either

For reference:

cd pyflwor
sudo pip install ply
pip install . --user

Using the --user flag takes care of the permissions, and using pip install . avoids installing as an egg.

This is because pyflwor compiles the queries on the fly, and generates a few files in the process. Installing a regular egg raises issues with write permissions.

So, the real answer is, while HAROS depends on pyflwor, I should recommend using it only within a virtualenv.

gavanderhoorn commented 5 years ago

This is because pyflwor compiles the queries on the fly, and generates a few files in the process. Installing a regular egg raises issues with write permissions.

So a packaged version of pyflwor doesn't make much sense then?

Or can the directory where it generates those files be configured somehow?

So, the real answer is, while HAROS depends on pyflwor, I should recommend using it only within a virtualenv.

This I don't really understand, unless you mean to say that because of the fact that pyflwor essentially requires an editable install, it should only be installed in a virtualenv to avoid polluting the user's system.

gavanderhoorn commented 5 years ago

According to 6.12 Miscellaneous Yacc Notes in the PLY docs, there are some options for configuring the directory where generated files are written to (see the first 4 bullets).

I'm not sure how accessible those settings are from pyflwor though.


Edit: yacc.yacc(..) is called here in pyflwor and confuses me, as it has write_tables=False, which would seem to imply that serialising the tables is actually disabled.

@git-afsantos: are you using PLY somewhere directly?

git-afsantos commented 5 years ago

According to 6.12 Miscellaneous Yacc Notes in the PLY docs, there are some options for configuring the directory where generated files are written to (see the first 4 bullets).

I'm not sure how accessible those settings are from pyflwor though.

I was looking into the exact same thing.

I think this line is where we would have to make changes.

https://github.com/timtadh/pyflwor/blob/f3861f73b2bf6106a0363c15d5de040d8b01d50f/pyflwor/parser.py#L39

I could open a pull request to pyflwor and hope the author still cares for the project, or HAROS could ship with a modified version of pyflwor.

gavanderhoorn commented 5 years ago

Some additional context: we / I'm again trying to packages haros in such a way that it could be used as a ROS test_dependable linter/analyser as part of a regular ROS Catkin build cycle.

catkin_virtualenv gets us really close, but the (current) requirement of the editable install of pyflwor is blocking.

Pretty happy when I stumbled upon pyflwor-ext, which seemed to solve all issues, but the writable part in the instructions made me wonder, hence this issue.

git-afsantos commented 5 years ago

This I don't really understand, unless you mean to say that because of the fact that pyflwor essentially requires an editable install, it should only be installed in a virtualenv to avoid polluting the user's system.

That is basically the idea. Changing the whole HAROS installation instructions to put everything within a virtualenv from the start.

gavanderhoorn commented 5 years ago

According to 6.12 Miscellaneous Yacc Notes in the PLY docs, there are some options for configuring the directory where generated files are written to (see the first 4 bullets). I'm not sure how accessible those settings are from pyflwor though.

I was looking into the exact same thing.

I think this line is where we would have to make changes.

https://github.com/timtadh/pyflwor/blob/f3861f73b2bf6106a0363c15d5de040d8b01d50f/pyflwor/parser.py#L39

I found the same line, but even though optimize=True, that ctor has write_tables=False as well, which according to the docs prevents writing out the tables.

There is also pyflwor/lexer.py though, and seeing the error message in timtadh/pyflwor#3 mentioning something like lextab, I'm not sure the parser.py line is actually the cause of this.

git-afsantos commented 5 years ago

Some additional context: we / I'm again trying to packages haros in such a way that it could be used as a ROS test_dependable linter/analyser as part of a regular ROS Catkin build cycle.

Cool :+1: I actually have been writing down some ideas for a major refactoring of HAROS that would solve many of these issues, while making it more extensible. I just do not know when will I get the free time to start working on that.

gavanderhoorn commented 5 years ago

Is pyflwor used in the model extraction part of HAROS?

Could the fact that I've not set it up correctly be the cause of my issues in #42?

git-afsantos commented 5 years ago

There is also pyflwor/lexer.py though, and seeing the error message in timtadh/pyflwor@3 mentioning something like lextab, I'm not sure the parser.py line is actually the cause of this.

This could be another culprit, yes, and the docs suggest there is also a parameter to change the output file.

Is pyflwor used in the model extraction part of HAROS?

It is used to run queries after the extraction itself takes place. The query engine is enabled by default (with no support to disable it currently), which is why the import has to succeed. As I mentioned earlier, if I changed a few dependencies to conditional/just-in-time imports, it might solve the issue for the use cases where it is not needed.

gavanderhoorn commented 5 years ago

As I mentioned earlier, if I changed a few dependencies to conditional/just-in-time imports, it might solve the issue for the use cases where it is not needed.

That could provide a way forward to get to an MVP.

Would that be something you see yourself having time for in the near future?

git-afsantos commented 5 years ago

Sure, at least for pyflwor. I could make it so that the queries are skipped and a warning is issued if pyflwor is not installed. This way the dependency becomes optional, if you do not need that feature.

git-afsantos commented 5 years ago

https://github.com/git-afsantos/haros/pull/43 might be enough for you to get to an MVP.

nlimpert commented 5 years ago

I had success running haros with pyflwor_ext (at least in terms of having some output generated - I didn't take a look at the failure of cccc):

ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ ll /tmp/haros_output/
total 8,0K
drwxrwxr-x  2 ubuntuvm ubuntuvm 4,0K Mär  5 09:17 .
drwxrwxrwt 11 root     root     4,0K Mär  5 09:17 ..
ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ haros -c industrial_core/ analyse -d /tmp/haros_output/
Traceback (most recent call last):
  File "/home/ubuntuvm/.local/bin/haros", line 7, in <module>
    from haros.haros import main
  File "/home/ubuntuvm/.local/lib/python2.7/site-packages/haros/haros.py", line 105, in <module>
    from .analysis_manager import AnalysisManager
  File "/home/ubuntuvm/.local/lib/python2.7/site-packages/haros/analysis_manager.py", line 32, in <module>
    import pyflwor
ImportError: No module named pyflwor
ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ pip install pyflwor_ext --user
Collecting pyflwor_ext
Collecting ply (from pyflwor_ext)
  Using cached https://files.pythonhosted.org/packages/a3/58/35da89ee790598a0700ea49b2a66594140f44dec458c07e8e3d4979137fc/ply-3.11-py2.py3-none-any.whl
Collecting future (from pyflwor_ext)
Installing collected packages: ply, future, pyflwor-ext
Successfully installed future-0.17.1 ply-3.11 pyflwor-ext-1.2
You are using pip version 8.1.1, however version 19.0.3 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ haros -c industrial_core/ analyse -d /tmp/haros_output/
[HAROS] Loading common definitions...
[HAROS] Loading plugins...
  > Loaded cpplint_plugin
  > Loaded mi_calculator
  > Loaded cccc_plugin
  > Loaded cppcheck_plugin
  > Loaded ccd_plugin
  > Loaded lizard_plugin
  > Loaded radon_plugin
  > Loaded pylint_plugin
[HAROS] Reading project and indexing source code...
[HAROS] Running analysis...
ERROR:haros.analysis_manager:Plugin cccc_plugin ran into an error.
[HAROS] Saving analysis results...
ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ ll /tmp/haros_output/
total 60K
drwxrwxr-x  7 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 .
drwxrwxrwt 11 root     root     4,0K Mär  5 09:18 ..
drwxrwxr-x  2 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 css
drwxrwxr-x  3 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 data
-rw-rw-r--  1 ubuntuvm ubuntuvm  29K Mär  5 08:47 index.html
drwxrwxr-x  3 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 js
drwxrwxr-x  5 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 lib
drwxrwxr-x  3 ubuntuvm ubuntuvm 4,0K Mär  5 09:18 projects
ubuntuvm@ubuntuvm-VirtualBox-1604:~/rosin_ws/src$ 

So I'm getting results generated and written. If needed I could see whether this could be done in Docker to fulfill the needs required for the ROS buildfarm.

Note: this also seems to work on an 18.04 machine - just for reference.

gavanderhoorn commented 5 years ago

@nlimpert: it would be interesting to see what the effect is of an install of pyflwor-ext in a non-writable location.

But I would've expected things to error-out already seeing as you don't install pyflwor as editable nor install PLY manually as @git-afsantos wrote in https://github.com/git-afsantos/haros/issues/38#issuecomment-469309391.

Have you ran any of the queries that actually use pyflwor?

Might be good to first check #43.

git-afsantos commented 5 years ago

@nlimpert: it would be interesting to see what the effect is of an install of pyflwor-ext in a non-writable location.

This is what I tested (an install with sudo pip) and got the write permissions error.

Have you ran any of the queries that actually use pyflwor?

I ran them on the proposal in #43 without errors, although for the original issue in this thread it does not really matter, since the import was attempted in any case.

I didn't take a look at the failure of cccc

This should be unrelated. Either you do not have cccc installed, or you have a newer version that breaks the expected interface (but I think the tool has not received development for some time now).

And theres always -b cccc_plugin to blacklist it, if it bothers you.

nlimpert commented 5 years ago

This is what I tested (an install with sudo pip) and got the write permissions error.

Apologies, I should have read the discussion more carefully. Apparently I run into the same issue.


Just FYI: I checked whether the setup with catkin_virtualenv runs with haros_catkin defining the following in the requirements.txt:

haros>=2.1.2
pyflwor-ext==1.2

And that seems to run fine on the haros_catkin_test package with no problems at least in Ubuntu 18.04. (obviously PLY seems to work due to the virtualenv created by catkin_virtualenv in the devel-space).

However, the virtualenv environment in Ubuntu 16.04 results in a weird error with pip:

/home/ubuntuvm/rosin_ws/devel_isolated/haros_catkin/share/haros_catkin/venv/bin/python /home/ubuntuvm/rosin_ws/devel_isolated/haros_catkin/share/haros_catkin/venv/bin/pip install -qq -U pip
Traceback (most recent call last):
  File "/home/ubuntuvm/rosin_ws/build_isolated/haros_catkin/venv/bin/pip", line 7, in <module>
    from pip._internal import main

I think this is related to this issue because I guess that this might work with catkin_virtualenv as long as it works in Ubuntu 16.04.

gavanderhoorn commented 5 years ago

Thanks for the update @nlimpert.

I'm not too worried about the case where haros_catkin is part of a workspace itself. But if we want to be able to release haros_catkin, we cannot rely on an editable install, so we should probably find some other way to fix this.

Perhaps some of the previous comment (https://github.com/git-afsantos/haros/issues/38#issuecomment-469317496 and https://github.com/git-afsantos/haros/issues/38#issuecomment-469319200 fi) could help set a direction.

nlimpert commented 5 years ago

Thanks for the update @nlimpert.

I'm not too worried about the case where haros_catkin is part of a workspace itself. But if we want to be able to release haros_catkin, we cannot rely on an editable install, so we should probably find some other way to fix this.

Perhaps some of the previous comment (#38 (comment) and #38 (comment) fi) could help set a direction.

Thanks for the reply. I should have expressed the update more clear (and sorry if I got it wrong); In my setup (18.04) I did not have anything haros related installed (so neither pyflwor nor PLY). In Ubuntu 18.04 the setup with haros_catkin actually worked just fully in the virtualenv by defining pyflwor-ext as a requirement of haros_catkin. So I also did not have to install pyflwor as an editable installation to make use of it.

So I was thinking that if we manage to get catkin_virtualenv to work (again?) in Ubuntu 16.04 (pip is broken in the virtualenv) we might be done.

gavanderhoorn commented 5 years ago

In my setup (18.04) I did not have anything haros related installed (so neither pyflwor nor PLY). In Ubuntu 18.04 the setup with haros_catkin actually worked just fully in the virtualenv by defining pyflwor-ext as a requirement of haros_catkin. So I also did not have to install pyflwor as an editable installation to make use of it.

Did you run the model extraction and query phase of analyse (ie: -n)?

So I was thinking that if we manage to get catkin_virtualenv to work (again?) in Ubuntu 16.04 (pip is broken in there) we might be done.

I've tested catkin_virtualenv on 16.04 just this weekend and it worked for me. Anything special about your setup?

nlimpert commented 5 years ago

Did you run the model extraction and query phase of analyse (ie: -n)?

No - it was just the setup used in haros_catkin which only executes analyse with -d (see here). But I will check how it looks like with -n in 18.04.

I've tested catkin_virtualenv on 16.04 just this weekend and it worked for me. Anything special about your setup?

Oh good to know - I should check for differences.

I have created a branch and will test this once I get catkin_virtualenv to work in my 16.04 setup.

nlimpert commented 5 years ago

I've tested catkin_virtualenv on 16.04 just this weekend and it worked for me. Anything special about your setup?

I can confirm this works fine with a newly setup 16.04 VM :+1:.

I also ran a check whether the proposed setup of haros_catkin works in 16.04 and it seems to be fine. What I did was the following:

  1. Setup a new Ubuntu 16.04 VM with ros-kinetic-desktop-full, ros-kinetic-catkin-virtualenv, cppcheck, cccc, libclang-3.8-dev.
  2. Create one workspace called catkin_ws that only includes my branch of haros_catkin. Build this one with catkin_make_isolated.
  3. Source catkin_ws/devel_isolated/setup.bash and create a new workspace catkin_test_ws with the package haros_catkin_test.
  4. Run catkin_make_isolated in catkin_test_ws and observe the files generated by haros in catkin_test_ws/build_isolated/test_pkg/test_results/haros_report

Then I also checked whether this works with the industrial_core package - particularly the industrial_robot_client package by adding the following to its CMakeLists.txt's content of if (CATKIN_ENABLE_TESTING):

  find_package(haros_catkin REQUIRED)
  haros_report()

... and haros was executed for industrial_robot_client after catkin_make_isolated was run for the catkin_test_ws workspace.

Note that I locally modified the haros_catkin-extras.cmake.em of haros_catkin to run haros with -n. haros reports the following on the command-line:

[HAROS] Running initial setup operations...
Already up-to-date.
[HAROS] Loading common definitions...
[HAROS] Loading plugins...
  > Loaded mi_calculator
  > Loaded pylint_plugin
  > Loaded lizard_plugin
  > Loaded radon_plugin
  > Loaded cppcheck_plugin
  > Loaded ccd_plugin
  > Loaded cpplint_plugin
  > Loaded cccc_plugin
[HAROS] Reading project and indexing source code...
  > Parsing nodes might take some time.
[HAROS] Running analysis...
[HAROS] Saving analysis results...

Afterwards the models of the industrial_robot_client package looks as follows: image @git-afsantos Is that as it is supposed to be? Any other way to check its functionality?

git-afsantos commented 5 years ago

Good to know that it is working, and thanks for sharing the steps. :+1:

That empty layout issue is likely what @gavanderhoorn mentioned in another issue. See this comment for reference.

git-afsantos commented 5 years ago

I will be merging #43, since it seems to provide temporary solution for the issue.

@gavanderhoorn @nlimpert Should I close this issue? Should a new one be opened to keep track of further work on this (e.g. forcing virtualenv or shipping a modified pyflwor)?

nlimpert commented 5 years ago

Sounds good to me!

git-afsantos commented 5 years ago

47 is now tracking the possible ways to deal with pyflwor in the future.

git-afsantos commented 5 years ago

@gavanderhoorn Not sure if you ever got the notification in this comment, since you were not participating in the issue, but these notes might make sense for what you guys are working on now.

git-afsantos commented 5 years ago

@gavanderhoorn @nlimpert I just merged a monkey patch that makes pyflwor write its files to ~/.haros/pyflwor (#50). It seems to work on my machine, and I expect it to work on your end too.

Just a note: ~/.haros/pyflwor does not exist in previous versions, so, when you update, either run haros init again, or just create the pyflwor directory manually.

nlimpert commented 5 years ago

Sounds great! I will check it here with haros_catkin asap.

nlimpert commented 5 years ago

Sorry for the late response! I can confirm that this works.