Skip to content

Idea to fix issue #5310 - Using site packages of a different python version for actionrunner #5388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Idea to fix issue #5310 - Using site packages of a different python version for actionrunner #5388

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
559bbcc
Update sandboxing.py
wingiti Oct 12, 2021
546f5c2
Update sandboxing.py
wingiti Oct 29, 2021
58877d3
Merge branch 'master' into fix_site-packages_for_custom_python_versio…
wingiti Oct 29, 2021
5ba4afb
Update sandboxing.py
wingiti Oct 29, 2021
4e282ba
Update sandboxing.py
wingiti Oct 29, 2021
3d5591a
Update sandboxing.py
wingiti Nov 24, 2021
043958a
Merge branch 'master' into fix_site-packages_for_custom_python_versio…
wingiti Nov 24, 2021
bab39f8
Merge branch 'master' into fix_site-packages_for_custom_python_versio…
wingiti Nov 25, 2021
3da886c
Merge branch 'StackStorm:master' into fix_site-packages_for_custom_py…
wingiti Dec 17, 2021
a7fb408
Merge branch 'master' into fix_site-packages_for_custom_python_versio…
cognifloyd Apr 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions st2common/st2common/util/sandboxing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,26 @@ def get_sandbox_python_path_for_python_action(

pack_base_path = get_pack_base_path(pack_name=pack)
virtualenv_path = get_sandbox_virtualenv_path(pack=pack)
python_binary = cfg.CONF.actionrunner.python_binary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I see that this is already defined. I'll just leave this note here for the next time I review this so I don't have to look it up again.

st2/st2common/st2common/config.py

Lines 454 to 458 in f5c0c0f

cfg.StrOpt(
"python_binary",
default=default_python_bin_path,
help="Python binary which will be used by Python actions.",
),


# Add the custom python's site-packages directory in front of the Python
# system site-packages if python_binary is not default.
if python_binary is not sys.executable and os.path.isfile(python_binary):
python_version = python_binary.rsplit("/", 1)[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an example of how this is supposed to work?
eg if someone sets cfg.CONF.actionrunner.python_binary to /usr/local/bin/python for some custom build of python (or whatever), then that path is not going to have a "version" in its string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cognifloyd, as mentioned in the title already this is an idea and I am open for better solutions. I just created it based on what I currently see on my system and found on the internet about python and site packages.

If someone just sets /usr/local/bin/python the variable name python_version might be misleading (I just thought about using different python binaries containing version digits, like python3.7, etc. when I chose the name) but should still to the job, if the site-packages are at the same directories as "usual". Maybe binary_name would be better...

What I have seen so far is that site-packages are usually located at:
/usr/lib/<binary_name>/site-packages
/usr/lib/<binary_name>/dist-packages
/usr/local/lib/<binary_name>/site-packages
/usr/local/lib/<binary_name>/dist-packages

That is the reason why I created system_prefix_dirs and system_dir_names. But it might be that there are linux distributions or custom configs out there using different locations. In this case the solution will not work.

The idea behind these lines was to fix the issue without additional config options as a user might think it should work out of the box when he sets the python_binary config option. This is what I thought as well... ;)

If you want to catch all possibilities, it might be better to implement a new config option like python_binary_site-packages. In this case a user needs to set up two config settings and needs to know where his site packages are located, but he has full control.

Third possibility would be to use both. Look for python_binary_site-packages and if not defined try the lines above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes. binary_name works better.

I like this implementation and how it avoids additional config options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hard-coding the list of site-packages dirs, what if you looked for the dir relative to the python_binary? So Path(python_binary).parent.parent / "lib" / "site-packages" and variants for lib* directories and either site- or dist-

system_prefix_dirs = ["/usr/lib", "/usr/local/lib"]
system_dir_names = ["site-packages", "dist-packages"]
for system_prefix_dir in system_prefix_dirs:
for system_dir_name in system_dir_names:
if os.path.isdir(
os.path.join(system_prefix_dir, python_version, system_dir_name)
):
custom_py_site_pack_dir = os.path.join(
system_prefix_dir, python_version, system_dir_name
)
existing_sandbox_py_path = sandbox_python_path
sandbox_python_path = (
f"{custom_py_site_pack_dir}:{existing_sandbox_py_path}"
)

if virtualenv_path and os.path.isdir(virtualenv_path):
pack_virtualenv_lib_path = os.path.join(virtualenv_path, "lib")
Expand Down