Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4414 closed defect (fixed)

ScriptWrapper should be multiplatform

Reported by: chiiph Owned by: nickm
Priority: Medium Milestone:
Component: Archived/Thandy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #4460 Points:
Reviewer: Sponsor:

Description

As of right now, ScriptWrapper spawns a new python process with the path to the thp script as a parameter (and the right env), but this won't work in every platform since "python" may not be in the PATH at all.

So I've written a ScriptBundleWrapper, that uses python code.InteractiveInterpreter to run code in its own env. The code for this is in my branch multiplatform_script.

Child Tickets

Change History (15)

comment:1 Changed 8 years ago by chiiph

Status: newneeds_review

comment:2 Changed 8 years ago by nickm

If the goal here is only to handle the case where the python binary is not named "python" and in the path, why not just use sys.executable to find out where it is?

I don't think you should use code.InteractiveInterpreter for this at all. The interpreter's semantics actually give you different syntax from regular python.

Example: the following is legal in a Python script, but not legal in an interactive environment:

def a():
 pass
def b():
  pass

and the following is legal in a python script but not an interactive environment:

def a():

   pass

(An interactive interpreter requires a blank line at the end of a top-level block, and requires that every blank line indicate the end of a top-level block.)

Also, stuff like this should make you terrified:

+            run_code.append("os.environ[\""+var+"\"] = \""+self._env[var]+"\"\n

What if there's something in self._env that contains a quote or a newline?

comment:3 in reply to:  2 ; Changed 8 years ago by chiiph

Replying to nickm:

If the goal here is only to handle the case where the python binary is not named "python" and in the path, why not just use sys.executable to find out where it is?

If we ship Thandy with PyInstaller, we won't have a python executable.

I don't think you should use code.InteractiveInterpreter for this at all. The interpreter's semantics actually give you different syntax from regular python.

Example: the following is legal in a Python script, but not legal in an interactive environment:

def a():
 pass
def b():
  pass

and the following is legal in a python script but not an interactive environment:

def a():

   pass

(An interactive interpreter requires a blank line at the end of a top-level block, and requires that every blank line indicate the end of a top-level block.)

True, but I think it's better than using plain eval, and we can't use the python executable for the reason above.

Also, stuff like this should make you terrified:

+            run_code.append("os.environ[\""+var+"\"] = \""+self._env[var]+"\"\n

What if there's something in self._env that contains a quote or a newline?

Yeah, I thought about using repr() but I'm not sure if that would mess up some part of the rest of the env.

comment:4 in reply to:  3 ; Changed 8 years ago by nickm

Replying to chiiph:

Replying to nickm:

If the goal here is only to handle the case where the python binary is not named "python" and in the path, why not just use sys.executable to find out where it is?

If we ship Thandy with PyInstaller, we won't have a python executable.

Ah; that's a problem.

I don't think you should use code.InteractiveInterpreter for this at all. The interpreter's semantics actually give you different syntax from regular python.

Example: the following is legal in a Python script, but not legal in an interactive environment:

def a():
 pass
def b():
  pass

and the following is legal in a python script but not an interactive environment:

def a():

   pass

(An interactive interpreter requires a blank line at the end of a top-level block, and requires that every blank line indicate the end of a top-level block.)

True, but I think it's better than using plain eval, and we can't use the python executable for the reason above.

How is it better than eval? It changes our language from "Python" to "Not really Python". Taking a quick look at the source for code.py, I see that internally all it's really doing is calling compile() and exec().

Also, stuff like this should make you terrified:

+            run_code.append("os.environ[\""+var+"\"] = \""+self._env[var]+"\"\n

What if there's something in self._env that contains a quote or a newline?

Yeah, I thought about using repr() but I'm not sure if that would mess up some part of the rest of the env.

repr() on a string always gives you a string that evaluates to that string().

comment:5 in reply to:  4 ; Changed 8 years ago by rransom

Replying to nickm:

Replying to chiiph:

Replying to nickm:

If the goal here is only to handle the case where the python binary is not named "python" and in the path, why not just use sys.executable to find out where it is?

If we ship Thandy with PyInstaller, we won't have a python executable.

Ah; that's a problem.

PyInstaller is unacceptable for use in TBB. RTFM: http://www.pyinstaller.org/export/latest/tags/1.5.1/doc/Manual.html?format=raw#how-one-file-mode-works

comment:6 Changed 8 years ago by chiiph

Replying to nickm:

How is it better than eval? It changes our language from "Python" to "Not really Python". Taking a quick look at the source for code.py, I see that internally all it's really doing is calling compile() and exec().

eval will execute the code in the same environment (not just in terms of os.environ) than the rest of Thandy (and the same applies to compile/exec, afaict). With InteractiveInterpreter, it runs as if a new python process was doing it. I'll take a look at code.py and figure out a better way of doing this.

repr() on a string always gives you a string that evaluates to that string().

Yes, I guess that's the way to go :) I'll update the patch.

comment:7 in reply to:  5 Changed 8 years ago by nickm

Replying to rransom:

PyInstaller is unacceptable for use in TBB. RTFM: http://www.pyinstaller.org/export/latest/tags/1.5.1/doc/Manual.html?format=raw#how-one-file-mode-works

To be clear, you're not talking about the setuid issue (this should never setuid!) but rather about the fact that it dumps files in a temporary directory, right? IIUC, the right thing here is to not use --onefile if we're going to try pyinstaller.

comment:8 in reply to:  6 ; Changed 8 years ago by nickm

Replying to chiiph:

Replying to nickm:

How is it better than eval? It changes our language from "Python" to "Not really Python". Taking a quick look at the source for code.py, I see that internally all it's really doing is calling compile() and exec().

eval will execute the code in the same environment (not just in terms of os.environ) than the rest of Thandy (and the same applies to compile/exec, afaict). With InteractiveInterpreter, it runs as if a new python process was doing it. I'll take a look at code.py and figure out a better way of doing this.

It does this by calling exec in order to get a new set of globals and locals. Really, that's all it's doing for isolation. You can do the same with execfile.

comment:9 Changed 8 years ago by chiiph

Well, Thandy also uses temporary directories for thp files, so I don't see the actual problem.

comment:10 in reply to:  8 Changed 8 years ago by chiiph

Replying to nickm:

Replying to chiiph:

Replying to nickm:

How is it better than eval? It changes our language from "Python" to "Not really Python". Taking a quick look at the source for code.py, I see that internally all it's really doing is calling compile() and exec().

eval will execute the code in the same environment (not just in terms of os.environ) than the rest of Thandy (and the same applies to compile/exec, afaict). With InteractiveInterpreter, it runs as if a new python process was doing it. I'll take a look at code.py and figure out a better way of doing this.

It does this by calling exec in order to get a new set of globals and locals. Really, that's all it's doing for isolation. You can do the same with execfile.

Hm, ok, I'll look into it and come up with a new patch.

comment:11 Changed 8 years ago by chiiph

I've updated the patch in the same branch (multiplatform_script).

I didn't use repr() because it didn't work with unicode strings. So I thought I could do a simple replacement, and it seems to work for both the case where there's a " and when there's a new line.

comment:12 Changed 8 years ago by nickm

You're still not handling \ correctly, or nul, or any number of characters not legal for literal use. If I stick \" in the middle of a string, your code will turn it into
", breaking out of the string as before.

I don't think this is even necessary. Because this is happening in the same python process, this should have the same os module. So you can just do

   old_environ = os.environ.copy()
   os.environ.clear()
   os.environ.update(self._env)
   try:
      # do the exec stuff here
   finally:
      os.environ.clear()
      os.environ.update(old_environ)

Also, this code does a bare "except:" ... so if there is an error, there is no way to find out what the error was. For debugging, that seems like a problem.

comment:13 Changed 8 years ago by chiiph

Ok, how about now? (updated the same branch)

comment:14 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks better; merging.

comment:15 Changed 8 years ago by chiiph

Parent ID: #4460
Note: See TracTickets for help on using tickets.