ScriptWrapper should be multiplatform

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.

added component::archived/thandy owner::nickm parent::4460 priority::medium resolution::fixed status::closed type::defect labels

Trac:
Status: new to needs_review

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?

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.

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().

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

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.

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.

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.

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

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.

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.

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.

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

Looks better; merging.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Trac:
Parent: N/A to #4460 (closed)

closed

mentioned in issue #4418 (closed)

mentioned in issue #4460 (closed)