search

thiagoralves / OpenPLC_v3

OpenPLC Runtime version 3
GNU General Public License v3.0
1.1k stars 443 forks source link

Fix Python Dependency versions #22

Closed sysarcher closed 5 years ago

sysarcher commented 5 years ago

@thiagoralves what is your opinion on fixing the Python dependencies through a requirements.txt file?

This way we won't break if for instance the Flask project has some changes in the latest version. We can just issue a pip install -r requirements.txt command for installation of the dependencies.

I can submit a PR if you're interested.

pedromorgan commented 5 years ago

@shrmrf yep..

pedromorgan commented 5 years ago

Also we need to consider python3 (although need to check flask etc)

sysarcher commented 5 years ago

@pedromorgan sure... Python 2 is going out in 2020 so not a bad idea :)

thiagoralves commented 5 years ago

I think the requirements.txt file is a good idea. I saw that you made the modification only on the Docker install. Would you mind modifying it for all the other platforms as well? We might just need to make sure it will work under Cygwin (Windows) as well...

About python3, have you tried just running it plain in python3 to see what happens? I still haven't, but it may be worth trying

sysarcher commented 5 years ago

@thiagoralves I am not sure I can test the Windows version. For other linuxes, I'll submit a PR soon!

thiagoralves commented 5 years ago

In fact, I'm starting to get a bit worried about this. Github notified me after I accepted the requirements.txt file that it is using pycrypto <= 2.6.1 and there are security issues about that. In that sense, having pip install downloading the latest version of packages might be a good idea. Maybe we should keep the requirements.txt file as a backup in case something breaks in the future...

sysarcher commented 5 years ago

@thiagoralves the pycrypto is at the latest version. https://pypi.org/project/pycrypto/#history

The problem is that it has been abandoned since a few years. In the latest PR #24 I've only fixed the three packages that we are using (pyserial Flask and Flask-Login)... Let's hope these projects pick up speed and fix the dependency on pycrypto

sysarcher commented 5 years ago

Another perspective:

In fact, I'm starting to get a bit worried about this. Github notified me after I accepted the requirements.txt file that it is using pycrypto <= 2.6.1 ...

This I would say is an advantage of using requirements.txt.. GitHub can understand and parse this file. Otherwise, we'd never know of a vulnerability even though it was there. (more on this from GitHub's blog

sysarcher commented 5 years ago

FYI: I did a bit of investigation and figured that pycrypto==2.6.1 is installed by just installing python and python-pip i.e. sudo apt install python python-pip

Thus, I think removing it in the PR #24 was the correct idea.

Sorry again for the alarm I caused.