mushorg / conpot

ICS/SCADA honeypot
GNU General Public License v2.0
1.22k stars 413 forks source link

Inconsistencies regarding Python version #486

Open srenfo opened 4 years ago

srenfo commented 4 years ago

Conpot is currently all over the place with regards to its Python version:

  1. Travis and tox were recently bumped to Python 3.7.
  2. setup.py specifies Python 3.6 in two places.
  3. The Dockerfile installs Python 3.6 (albeit not explicitly).
    1. The Documentation "assumes you have Python 3.6 installed and running on your computer", explicity installs python3.6-dev and instructs the user to run virtualenv --python=python3.6 conpot.
  4. The PyPI page says " Requires: Python >=3.5".
  5. Due to the above, the badge on the GitHub repo says Python 3.5.

There may be more.

Suggestion:

  1. Agree on a minimum version. Python 3.6 would require the least changes, though it's already two releases behind as of this writing.
  2. Configure Travis to test all versions from the minimum onwards.
  3. Remove the specific version from tox.ini. Tox will then use the interpreter it was invoked with. (Remove basepython and fix or remove envdir.)
  4. Figure out a way to line up the various places a bit better because this situation inevitably leads to more inconsistencies. For example, as of this writing Travis uses a Debian image, Dockerfile uses an Alpine image and the docs assume something Debian-ish but the prerequisite packages in the docs don't actually line up with what's installed in .travis.yml. (I suppose this is a larger than just this issue.)
  5. Fixing PyPI and the badge is probably out since that requires a new release as far as I can tell.
srenfo commented 4 years ago

Addendum to clarify why this is all interconnected: Right now Travis only tests Python 3.7. If a Python 3.6-incompatible change were to be committed, Travis would not catch it. The Docker-based deployments would break but that would go unnoticed until someone complains. The Docker stuff is not tested as far as I can see.

TBH, the more technologies are involved the more complex this is to get right. And this is merely an issue about the Python version! Given that Python has a fairly robust deployment ecosystem (pip, virtualenv, pipx even, etc.) I would just do away with the Docker complexities and keep the docs for Debian (and derivatives). You might disagree of course.

I might also move the install instructions to README.md such that it's less likely they're forgotten about.

xandfury commented 4 years ago

Hi @srenfo As you might've guessed we currently only support version 3.6. Although we are aiming to transition to py3.7 soon. This is the reason why we are running tests with py3.7. Although not officially supported - conpot should run fine on 3.5 as well.

Although you are right about the python badge - it should update when we do a release.

srenfo commented 4 years ago

Part of my point was that you can test all versions simultaneously if you set up tox that way. Hence PR #488.

But fair enough regarding a transition. Do you want to close this issue or do you want it as a reminder for when the time comes?

glaslos commented 3 years ago

@xandfury how about dropping all other versions than 3.8?

glaslos commented 3 years ago

@srenfo do you have a version preference or is consolidation on any version sufficient?

xandfury commented 3 years ago

Yep. Agreed. We can drop versions below 3.8

srenfo commented 3 years ago

@srenfo do you have a version preference or is consolidation on any version sufficient?

My original issue was with the inconsistent handling of Python versions in the repo. IIRC it had caused some local issue at the time, but I don't recall what it was.

I think we're making this issue more difficult than it is. There is only one fundamental question and two (or two and a half) pieces of additional configuration and documentation that derive from the answer:

  1. What is the minimum version of Python that Conpot actually requires to run? The answer needs to be set in python_requires: https://github.com/mushorg/conpot/blob/d08e2ed14578541e234ff1ad28eb94ef7514a020/setup.py#L7-L9 This can be bumped as needed, e. g. if you use a feature from a more recent standard library. In my opinion you (we) can be aggressive about this, but if everything else is done correctly there should be no need to artificially bump the version.

  2. Then, the minimum version should be tested in the CI pipeline. Otherwise you won't catch incompatible changes. This is the second place the version has to be explicitly configured. This strategy assumes that Python is forwards compatible, which it mostly has been. Optionally, newer versions can be tested as well. Tox makes this easy, but this is likely no longer feasible for the Travis CI pipeline given their recent changes. GH Actions to the rescue? Or use tox in conjunction with a multi-python Docker image.

  3. Also update the documentation to reflect any change in the minimum version. This is the third place the minimum version has to be set. Alternatively, document just python3-dev as a requirement and let setuptools handle incompatibilities (which it can thanks to the python_requires metadata above). That way, you may never (or rarely) need to update the docs and you're down to two places.

  4. Maybe use the same minimum version in the Dockerfile. This is a weak requirement since the Dockerfile is not being tested. Though it may add some confidence in the minimum version setting if that version is also being used in "production".

What I would not do is force the exact version in tox.ini. This would be different if the CI pipeline for the minimum version didn't exist. But as is it only hinders local development. setuptools will still throw an error if python_requires is violated on your development machine, but since we assume forwards compatibility it should be fine to let people test with any compliant version that happens to be installed locally. This was the idea behind #488, but I may have to revisit that PR and/or adapt #529 since I also changed the way Conpot is installed by tox.

If you don't want to assume forwards compatibility then the optional testing of (all) newer versions is no longer optional.

Bottom line: My dev machine has 3.8, so despite all the above I'd be fine with that change. With the above strategy I'd also be fine with leaving it at >=3.6 though.