Comment 1 Pierre Ossman 2012-03-29 16:12:15 CEST

*** Bug 2749 has been marked as a duplicate of this bug. ***

Comment 2 Pierre Ossman 2013-10-28 11:25:28 CET

Maybe we can let python-thinlinc be a script instead that does something creative with $PYTHONPATH? It would avoid having that template in each and every Python program.

Comment 3 Pierre Ossman 2021-06-18 14:15:11 CEST

Our current sudorelaunch breaks this approach because it re-excutes using sys.executable which bypasses the script initially used.

At first glance it seems like this should work anyway, as $PYTHONPATH should be inherited through the chain and still be active. Unfortunately sudo refuses to pass on $PYTHONPATH, even with -E, as it is part of its built in list of variables to always remove.

I don't know why we use sys.executable though. There is no rationale in the code or in the commit. So if we use sys.argv[0] then we should go via the hash-bang again and everything should evaluate properly. This should be more robust for future changes as well.

Comment 4 Pierre Ossman 2021-06-18 14:21:05 CEST

Hmm... Using sys.argv[0] is problematic in the installer as it doesn't have a functional hash bang and we have other hacks to get it to work (and tl-setup as well before bug 7566). So that's likely why sys.executable is used.

Will need to get creative here...

Comment 5 Pierre Ossman 2021-06-18 14:33:16 CEST

Python 3.10 has added a more correct argv:

https://bugs.python.org/issue23427

However that doesn't help us right now as it will be many years before we can requires 3.10.

Comment 6 Pierre Ossman 2021-06-21 09:44:35 CEST

Ah. Apparently Python sets sys.executable based on the original argv[0], rather than the real path to the Python interpreter. That means that as long as we make sure argv[0] points to our wrapper, then re-executions will work just fine.

Comment 7 Pierre Ossman 2021-07-06 14:41:10 CEST

Hmm... inheriting $PYTHONPATH could be an issue if we run other things from our programs. The worst case is tl-run-profile which means the entire session is run with our $PYTHONPATH.

If bug 4245 is resolved then this should not have a major practical effect, but it is still a bit ugly.

Comment 8 Pierre Ossman 2021-07-06 16:50:01 CEST

More problems on the same theme, if the user wants to set a custom $PYTHONPATH in ~/.bash_profile, that will get nuked by tl-run-profile.

Ideally we'd like to run with Python's -I flag to get rid of weird effects from the user's environment, without actually removing them from the environment. However Python is very limited in how we can inject our needed module path.

Comment 9 Pierre Ossman 2021-07-07 09:33:42 CEST

Instead of modifying the environment, we could have a wrapper that sets things up before running the actual script. So something like this:

> exec -a "$0" /usr/bin/python3 -I - "$@" << EOT
> import os
> import sys
> if len(sys.argv) < 2:
> 	sys.exit("%s: no script specified" %
> 	         os.path.basename(sys.executable))
> __fn = os.path.abspath(sys.argv[1])
> try:
> 	__file = open(__fn)
> except OSError as e:
> 	sys.exit("%s: can't open file '%s': [Errno %d] %s" %
> 	         (os.path.basename(sys.executable),
> 	          __fn, e.errno, e.strerror))
> with __file:
> 	sys.path.append("/opt/thinlinc/modules")
> 	sys.argv = sys.argv[1:]
> 	code = compile(__file.read(), __fn, "exec")
> 	exec(code)
> EOT

However this has a couple of problems:

 * The __main__ module will not be the script, but a dummy module where the script is loaded in to, a subtle but possibly important difference

 * The stack will have an extra entry at the top with this wrapper

 * The command line gets a bit ugly and doesn't fully represent what is run (you can't copy the output from ps and run it again)

 * This uses stdin to inject the wrapper, meaning we break stdin for the script we want to run

The command line is not easily fixed as it is very magical how you change it (in memory modification of the original argv), so we're probably stuck with that.

The stdin issue could probably be resolved by having the wrapper as a file on disk. This also makes the command line re-executable. However it also gets a lot uglier as we'll have the complete path to that wrapper as an argument.

Comment 10 Pierre Ossman 2021-07-07 16:34:27 CEST

I'm not finding any decent way of injecting our module path if -I is used, so for now that door seems closed to us. Python only reads system paths at that point, which vary depending on Python version and distribution, so not a good extension point for us.

A different approach is emulating what -I does, but keeping environment variables working enough for us to do what we want. This seems to be mostly possibly, except for the very specific -I behaviour of not including the script's directory in sys.path (more on that later).

The basic idea is for the wrapper to do:

 * Move all $PYTHON... environment variables out of the way so they have no effect

 * Disable user site directory

 * Set $PYTHONPATH the way we need it

Then we need to clean up what we've done to the environment before something gets spawned by our scripts. We could do this in modules/thinlinc/__init__.py as that will almost always be included. However it's just almost always, and we don't know how much Python code has been executed before we get to that point. So something earlier and more reliable would be preferred.

Fortunately there is the extension point "sitecustomize". It's a module that gets opportunistically loaded fairly early during Python startup, and we can make sure that module belongs to us by putting it in our modules/ directory (which is first in the search order, because of $PYTHONPATH).

Besides being a bit magical, the only problem with this approach is that it would then hide any other "sitecustomize" module on the system. But we should be able to chain load that one with some fudging of sys.modules and sys.path. E.g. Debian/Ubuntu uses this mechanism by default, so we need to make sure it continues working.


You'd think we could also use this module to get rid of that extra sys.path entry Python adds without -I, but unfortunately not. It turns out that this entry is added after "sizecustomize" is loaded, so we're running to early to fix that.


This approach also doesn't help us with any future features that -I might have. We would have to emulate those as they show up.


Also note that there is talk about changing this mechanism to make sure multiple hooks can be installed. That would be good news for us if it wasn't for the fact that they are also talking about only looking in system directories. Nothing is decided yet though, so we'll see what happens:

https://www.python.org/dev/peps/pep-0648/

Comment 14 Pierre Ossman 2021-07-08 13:55:09 CEST

The "sitecustomize" approach seems to work properly in practice, even if it is a bit more hacky than I'd ideally like. Hopefully upstream gives us better tools in the future.

Everything seems to work nicely though. Tested on SLES 12 and Ubuntu 20.04.

> * sys.path should include /opt/thinlinc/modules when using our wrapper

Yup. Nothing would work otherwise.

> * ps should show a command line that works

It does.

> * sys.executable + sys.argv should give an arg vector that works

Yup. Needed by sudorelaunch.

> * a user's $PYTHONPATH should be ignored by our scripts

It is.

> * the user's site dir should be ignored by our scripts

It is.

(A lot of other things aren't as well behaved though. E.g. gnome-terminal explodes for me here.)

> * all user $PYTHON* variables should propagate to subprocesses (e.g. via tl-run-profile)

It is. Tested with a custom $PYTHONPATH in ~/.bash_profile.

Comment 16 Niko Lehto 2021-07-09 13:33:15 CEST

Tested on Fedora 33:
🗸 sys.path should include /opt/thinlinc/modules when using our wrapper
🗸 ps should show a command line that works
🗸 sys.executable + sys.argv should give an arg vector that works
🗸 a user's $PYTHONPATH should be ignored by our scripts
🗸 the user's site dir should be ignored by our scripts
🗸 all user $PYTHON* variables should propagate to subprocesses

Looks good.