Install the python module inside a virtual environment (#243)

[arthaud]

The current installation process tries to install the ikos python module as a system library. This is probably not what a typical user want.

Instead, let's install a virtual environment for ikos and install the python module within it. Following homebrew's standard, this is installed under /libexec.

This has been tested on macOS Sonoma 14.1.2 and Ubuntu 20.

I also added two options which we will need when updating the Homebrew formula.

[ivanperez-keera]

@arthaud Let me try this on my side too and I'll merge if I don't find any issues. I'll probably get to it in the next hour or so.

[ivanperez-keera]

I'm reviewing this now.

[ivanperez-keera]

This is great. We've very close. Just a few comments:

1. The way that the venv is being used, system libraries are not available to IKOS. For example, the GA CI build job says (I'm hiding details on purpose):

sudo apt-get install --yes [...] python3 python3-pygments
[...]
    
Successfully installed pip-23.3.1
-- Running /opt/ikos/libexec/bin/python -m pip install -U pygments setuptools
Collecting pygments
  Downloading pygments-2.17.2-py3-none-any.whl.metadata (2.6 kB)
Requirement already satisfied: setuptools in /opt/ikos/libexec/lib/python3.10/site-packages (59.6.0)
Downloading pygments-2.17.2-py3-none-any.whl (1.2 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.2/1.2 MB 12.1 MB/s eta 0:00:00
Installing collected packages: pygments

The benefit of doing it this way is that we don't depend on the user installing dependencies, or keeping them around. The drawback is that users won't be able to take advantage of their own package management system. Since we have a plan to take IKOS to debian, it would also make the installation script incompatible with Debian, which would presumably package dependencies as individual packages.

If we want access to system packages, it's possible with --system-site-packages.

In either case, it'll pay to document it. Our readme states:

Dependencies

To build and run the analyzer, you will need the following dependencies:
[...]
(Optional) Pygments

Most of them can be installed using your package manager.

I'd suggest we change that too.

2. venv only exists in python >= 3.3. For python 2, we'd need to go through virtualenv which I believe wraps venv. I'm also fine dropping support for python 2 if we think it's time; we'd just need to change the README (and possibly other parts) accordingly.

3. Because the path to python is now in the destination folder ${CMAKE_INSTALL_PREFIX}/libexec/bin/, the scripts cannot be executed without running make install first. It's not a huge deal but it does make the develop-test cycle a bit more involved, although running it once should suffice.

4. Unless it's customary in python for some reason, for me the variable name USE_PYTHON_VIRTUALENV suggests that the variable will take a 1 or 0 value. May I suggest something like PYTHON_VIRTUALENV_PREFIX, PYTHON_VIRTUALENV_DIR or PYTHON_VIRTUALENV_PATH, also for consistency with other variables in the same file?

[ivanperez-keera]

Thinking back, I don't know how many users could be affected if we dropped support for python 2. My general inclination is to deprecate things first with a warning, and wait for one or more version to remove them (in other projects I wait 3, but we can enforce more agility in this project).

For info: I'm planning a release for this month and the next for March.

[ivanperez-keera]

Also, very minor but just for consistency with the way that other PRs are being merged, can you change the summary line of the commit message to just mention the issue with (#243)., and we'll add the Close #243. or Fix #243. to the merge commit?

[arthaud]

The way that the venv is being used, system libraries are not available to IKOS. For example, the GA CI build job says (I'm hiding details on purpose):

I'm not sure why someone would want to use system libraries? That seems like a rare use case.

Since we have a plan to take IKOS to debian, it would also make the installation script incompatible with Debian, which would presumably package dependencies as individual packages.

I think package maintainers will want to use -DINSTALL_PYTHON_VIRTUALENV=OFF and -DUSE_PYTHON_VIRTUALENV=/usr/bin/python; and install the python package like any other python package.

In either case, it'll pay to document it. Our readme states:

Yeah I should update the documentation (in the same PR). I will fix that tomorrow.

venv only exists in python >= 3.3. For python 2, we'd need to go through virtualenv which I believe wraps venv. I'm also fine dropping support for python 2 if we think it's time; we'd just need to change the README (and possibly other parts) accordingly.

Python 2 does not receive security patches since 2020, I don't think we should support it anymore.
Python 3.3 is also end of life:
https://devguide.python.org/versions/
For instance, the oldest version of debian that's still maintained (bullseye) has python 3.9.

Because the path to python is now in the destination folder ${CMAKE_INSTALL_PREFIX}/libexec/bin/, the scripts cannot be executed without running make install first. It's not a huge deal but it does make the develop-test cycle a bit more involved, although running it once should suffice.

I agree, that's an inconvenience. but wasn't is true already the case before my PR?

Unless it's customary in python for some reason, for me the variable name USE_PYTHON_VIRTUALENV suggests that the variable will take a 1 or 0 value. May I suggest something like PYTHON_VIRTUALENV_PREFIX, PYTHON_VIRTUALENV_DIR or PYTHON_VIRTUALENV_PATH, also for consistency with other variables in the same file?

Agreed, I will change that.

[ivanperez-keera]

The way that the venv is being used, system libraries are not available to IKOS. For example, the GA CI build job says (I'm hiding details on purpose):

I'm not sure why someone would want to use system libraries? That seems like a rare use case.

Since we have a plan to take IKOS to debian, it would also make the installation script incompatible with Debian, which would presumably package dependencies as individual packages.

I think package maintainers will want to use -DINSTALL_PYTHON_VIRTUALENV=OFF and -DUSE_PYTHON_VIRTUALENV=/usr/bin/python; and install the python package like any other python package.

Good point. That solves it.

In either case, it'll pay to document it. Our readme states:

Yeah I should update the documentation (in the same PR). I will fix that tomorrow.

Great!

venv only exists in python >= 3.3. For python 2, we'd need to go through virtualenv which I believe wraps venv. I'm also fine dropping support for python 2 if we think it's time; we'd just need to change the README (and possibly other parts) accordingly.

Python 2 does not receive security patches since 2020, I don't think we should support it anymore.
Python 3.3 is also end of life:
https://devguide.python.org/versions/
For instance, the oldest version of debian that's still maintained (bullseye) has python 3.9.

Great.

There's logic in some scripts for Python 2 (for example, I remember seeing some imports being handled differently depending on the version).

Because the path to python is now in the destination folder ${CMAKE_INSTALL_PREFIX}/libexec/bin/, the scripts cannot be executed without running make install first. It's not a huge deal but it does make the develop-test cycle a bit more involved, although running it once should suffice.

I agree, that's an inconvenience. but wasn't is true already the case before my PR?

I don't think so but, if you don't think it's a big deal, we can ignore it.

Something we can also simplify is removing unnecessary dependencies in the CI jobs since we are now using pip to install them, like python3-pygments, python3-distutils from:

ikos/.github/workflows/build-linux.yml

Lines 19 to 24 in 21cd2b5

	sudo apt-get install --yes \
	gcc g++ cmake libgmp-dev libboost-dev libboost-filesystem-dev \
	libboost-thread-dev libboost-test-dev \
	libsqlite3-dev libtbb-dev libz-dev libedit-dev \
	python3 python3-pygments python3-distutils python3-pip \
	llvm-14 llvm-14-dev llvm-14-tools clang-14

and removing these lines completely:

ikos/.github/workflows/build-macos.yml

Lines 15 to 17 in 21cd2b5

	- name: Install dependencies
	run: |
	pip3 install setuptools

This PR is becoming bigger. We can split it in separate commits (in case we ever need to review it) or even open separate issues for the removal of old Python 2-related logic and the changes to the CI job. We can even address these details after the release: It doesn't hurt to keep those in, even if they don't serve any purpose once your PR is merged.

[ivanperez-keera]

(Sorry, pressed some key that closed the PR while I was typing my answer.)

[arthaud]

I have done most of the changes.
We still need to remove all python 2 imports across the codebase.
We should also update scripts/bootstrap, that script installs python 2 when it's missing (which should be rare, who doesn't have python?)

[ivanperez-keera]

We still need to remove all python 2 imports across the codebase.

A quick search shows:

ikos/analyzer/python/ikos/http.py

Lines 42 to 52 in 21cd2b5

	try:
	# Python 3
	from http.server import HTTPServer, BaseHTTPRequestHandler
	from urllib.parse import parse_qs, urlencode
	from urllib.request import urlopen
	except ImportError:
	# Python 2
	from BaseHTTPServer import HTTPServer, BaseHTTPRequestHandler
	from urlparse import parse_qs
	from urllib import urlencode
	from urllib2 import urlopen

ikos/analyzer/python/ikos/html.py

Lines 42 to 47 in 21cd2b5

	try:
	# Python 3
	from html import escape
	except ImportError:
	# Python 2
	from cgi import escape

This second module is just a wrapper to hide the logic of importing html in different python versions, which means that we can remove that module altogether and instead modify these imports to import html (or even just escape from html):

ikos/analyzer/python/ikos/view.py

Line 59 in 21cd2b5

from ikos import html

ikos/analyzer/python/ikos/highlight.py

Line 42 in 21cd2b5

from ikos import html

We'd also need to adjust the README to mention only Python 3:

ikos/README.md

Line 174 in 21cd2b5

* Python 2 >= 2.7.3 or Python 3 >= 3.3

I also found this comment which we can update:

ikos/analyzer/python/ikos/output_db.py

Lines 70 to 72 in 21cd2b5

	# Use 'str' as text factory since it's the type of string literals
	# This is bytes in python 2 and unicode in python 3
	self.con.text_factory = str

We should also update scripts/bootstrap

That's script is quite outdated. Is it still used anywhere? A quick search shows that it's mentioned in the tests / docker files only.

If it's still useful, we can adjust python in this PR but I'd rather open a small separate issue to update the versions of other dependencies (e.g., LLVM), just to keep separate matters isolated.

If not, we can remove it (again, I can open a small, separate issue for that).

That's all I could find. I can contribute some of these changes by pushing them to your branch if that's ok with you (I believe I have rights but I'd rather ask first).

[arthaud]

That's script is quite outdated. Is it still used anywhere?

It is mentioned in the documentation (see INSTALL_ROOTLESS.md). I don't know if it still works. It's pretty nice to have but it's a bit annoying to maintain.

That's all I could find. I can contribute some of these changes by pushing them to your branch if that's ok with you (I believe I have rights but I'd rather ask first).

Sure, go ahead.

[ivanperez-keera]

Second attempt.

I made a minor tweak in the commit messages just for consistency, but I did not change anything else from your commits.

Feel free to modify my commit messages if you think they are not clear enough (or if something's wrong).

[ivanperez-keera]

@arthaud Is there anything else we need to change?

If not, I'll go ahead and merge these changes.

[ivanperez-keera]

Btw, I don't know if we want to change it yet but I believe your change should get rid of this problem altogether: https://github.com/NASA-SW-VnV/ikos/blob/master/TROUBLESHOOTING.md#could-not-find-ikos-python-module-while-running-ikos

Perhaps we can just wait a bit (I mean, we merge this as is and we wait to change the TROUBLESHOOTING.md guide until IKOS 3.3. or IKOS 3.4) to see if that every happens again. Your call.

[arthaud]

@arthaud Is there anything else we need to change?

LGTM

Btw, I don't know if we want to change it yet but I believe your change should get rid of this problem altogether: https://github.com/NASA-SW-VnV/ikos/blob/master/TROUBLESHOOTING.md#could-not-find-ikos-python-module-while-running-ikos

I would keep these for now in case people are installing older versions of IKOS. If we remove it then that link will break, leaving people confused.

[ivanperez-keera]

Great!