Remove python_selector.py in favor of run_python.sh #10729

sbc100 · 2020-03-19T16:20:33Z

Ubuntu 20.04 dropped the /usr/bin/python symlink so it is no longer
possible use /usr/bin/env python in launcher scripts and then
rely on python_selector.py to select a different python version.

Instead we go back to age-old method of using shell wrapper script.

Here I create a single wrapper script called run_python.sh which
runs a commmand under the best version of python it can find. Each
unsiffixed command is then a symbol link to this shell script. Since
windows users use the .bat files it shouldn't matter whether or not
these symlinks work on windows.

Fixes #10726

kripken

To confirm, while we assume most windows users use the bat files, they should still be able to do python on the python files?

kripken · 2020-03-19T22:13:59Z

tests/test_browser.py

@@ -2464,12 +2464,12 @@ def test_mmap_file(self):
  def test_emrun_info(self):
    if not has_browser():
      self.skipTest('need a browser')
-    result = run_process([PYTHON, path_from_root('emrun'), '--system_info', '--browser_info'], stdout=PIPE).stdout
+    result = run_process([PYTHON, path_from_root('emrun.py'), '--system_info', '--browser_info'], stdout=PIPE).stdout


is this change necessary? if it is that seems like a breaking change. if it isn't then why?

Yes this change is a slightly breaking change. You can no longer run python emcc (because emcc is now itself a shell script that runs python). You can still run ./emcc.py directly or python emcc.py if you like.

I think this is the best approach. The python_selector method where python re-invokes itself is kind of nasty. If I run python2 emcc I kind of expect it to run under python2 not invoke a python3 subprocess to run itself.

sgtm. is there some way we can show a clear error on python emcc / emranlib / etc., like keeping the files around but only containing a print of an error? or if that's not possible, maybe a comment?

I'm not deleting those files. They are now symbolic links to run_python.sh. So they exist by they are shell scripts.

I'm not sure we can have python <some_shell_script> do anything useful... but let me see...

kripken · 2020-03-19T22:17:31Z

em++.py

@@ -1,19 +1,17 @@
-#!/usr/bin/env python


why is em++.py special among the tools? seems like it's getting different treatment in this PR than the others and it's not obvious why.

Because em++ was using the python_selector in a slightly different way. It was using it to redirect to emcc.py as well as to choose the python version.

With this change, once python is running we assume its the correct version, we never try to re-invoke python. So this change means that em++ now runs emcc directly, without re-invoking python.

sbc100 · 2020-03-19T22:29:19Z

To confirm, while we assume most windows users use the bat files, they should still be able to do python on the python files?

Yes. One can always run python on the .py files. This change just means you can't run python on the non-py files (just like you can't run python on the .bat files).

sbc100 · 2020-03-19T22:35:37Z

The essential problem here is that there is no #! line that we can use to correctly run any version of python that works on all systems. So keeping these files in python format is not an option (as far as I can tell).

kripken · 2020-03-20T16:03:04Z

tests/test_browser.py

@@ -2464,12 +2464,12 @@ def test_mmap_file(self):
  def test_emrun_info(self):
    if not has_browser():
      self.skipTest('need a browser')
-    result = run_process([PYTHON, path_from_root('emrun'), '--system_info', '--browser_info'], stdout=PIPE).stdout
+    result = run_process([PYTHON, path_from_root('emrun.py'), '--system_info', '--browser_info'], stdout=PIPE).stdout


sgtm. is there some way we can show a clear error on python emcc / emranlib / etc., like keeping the files around but only containing a print of an error? or if that's not possible, maybe a comment?

Ubuntu 20.04 dropped the /usr/bin/python symlink so it is no longer possible use `/usr/bin/env python` in launcher scripts and then rely on python_selector.py to select a different python version. Instead we go back to age-old method of using shell wrapper script. Here I create a single wrapper script called run_python.sh which runs a commmand under the best version of python it can find. Each unsiffixed command is then a symbol link to this shell script. Since windows users use the .bat files it shouldn't matter whether or not these symlinks work on windows. Fixes #10726

The non-py version of these scripts were converted to shell scripts in #10729. The .bat files run python explicitly so can directly reference .py versions of these scripts. Also, move em-config to em-config.py to match all the others.

diegocr · 2020-04-12T11:35:33Z

Have just upgraded to 1.39.12 and found this PR broke running the compiler on my Windows/MinGW environment :-(

$ emcc --version
/c/emsdk/upstream/emscripten/emcc: line 1: tools/run_python.sh: No such file or directory

diegocr · 2020-04-12T12:51:00Z

Just realized emcc etc should be symbolic links now.. so is there some post-process that is failing here? :-/

Anyway, i made this script to workaround it...

#!/bin/sh

base_dir=$(dirname "$0")

find $base_dir -type f -name "*.orig" -o -name "*.rej" | xargs -l rm

emsdk update
emsdk install latest

for p in `ls $base_dir/*.diff`; do cd $EMSCRIPTEN && patch -p1 < $p; done

files=("em++" "emar" "emcc" "emcmake" "emconfigure" "emmake" "emranlib" "emrun" "emscons")
for f in "${files[@]}"
do
    rm -f "$EMSCRIPTEN/$f"
    ln -s "$EMSCRIPTEN/tools/run_python.sh" "$EMSCRIPTEN/$f"
done

sed -i -e 's-which-& 2>/dev/null-g' "$EMSCRIPTEN/tools/run_python.sh"

Later on, the first time i tried to compile something i got this error:

$ ./build.sh
patching file dcraw.tmp.c
emcc: error: 'C:/emsdk/upstream/bin\clang++.exe --version' failed: [Error 5] Acceso denegado

On the next attempt though, it did worked fine

$ ./build.sh
patching file dcraw.tmp.c
shared:INFO: (Emscripten: system change: 1.39.12|C:/emsdk/upstream/bin|11.0_wasm vs 1.39.11|C:/emsdk/upstream/bin|11.0_wasm, clearing cache)
shared:INFO: (Emscripten: Running sanity checks)
emcc:WARNING: --llvm-lto ignored when using llvm backend
....[snip]
cache:INFO: generating system asset: is_vanilla.txt... (this will be cached in "C:\msys\1.0\home\dc\.emscripten_cache\is_vanilla.txt" for subsequent builds)
cache:INFO:  - ok
cache:INFO: generating system library: libc.a... (this will be cached in "C:\msys\1.0\home\dc\.emscripten_cache\wasm-lto\libc.a" for subsequent builds)
cache:INFO:  - ok
cache:INFO: generating system library: libcompiler_rt.a... (this will be cached in "C:\msys\1.0\home\dc\.emscripten_cache\wasm-lto\libcompiler_rt.a" for subsequent builds)
cache:INFO:  - ok
...[snip]

If you guys want me to do some further troubleshooting, a PR for my env, etc lmk.

sbc100 · 2020-04-12T23:42:42Z

Sadly we don't really have the time or resources to test in MINGW.

I think the problem here is that you are installing the windows SDK, but then using the compiler not via the .bat file launchers but via the shell script symbolic links.

If you want to fix this we might need your help. I can think of possible options:

Try to fix the windows archive extraction in them emsdk to do the same thing as what ln -s is doing. BTW do you know what you ln -s command is doing on the windows filesystem? Do some windows filesystems have actual symbolic link support now?
Don't include the shell scripts at all in the windows installer. Can you run emcc.bat from the bash shell? Will your bash shell automatically add the .bat if you omit it? I'm tempted to choose this option because I don't really want to mess with symbolic links or bash shell scripts on windows machines. (Conversely we should remove the .bat file launchers from the linux and mac SDKs).

parasti · 2020-04-13T15:35:33Z

Also just found out that emcc now bails with an error on MSYS. Worked fine prior to this.

emcc isn't a symlink for me. It is a small text file that literally contains the string "tools/run_python.sh". This is how Git handles symlinks on Windows by default. https://github.com/git-for-windows/git/wiki/Symbolic-Links

In my opinion it is always a mistake use symlinks in distribution. Just don't do it. A small shell script that finds the run_python.sh script from anywhere will work just fine.

parasti · 2020-04-13T16:04:16Z

Here's an idea with a hypothetical tools/find_python.sh script:

#!/bin/sh
abspath="$(type -p $0)"
emroot=$(dirname "$abspath")
python=$("$emroot"/tools/find_python.sh)
exec $python "$abspath.py" "$@"

sbc100 · 2020-04-13T19:50:38Z

Does simply removing emcc work for you ? (i.e. using emcc.bat instead?)

I think it makes sense to include .bat files in the windows SDK and shell scripts in the linux/mac SDK but including both seems unnecessary/wrong.

I agree symbol links are bad idea on windows and we should not be using them. But on linux/mac there is nothing wrong with symbolic links (clang is symlink to clang-11, for example).

parasti · 2020-04-13T20:23:06Z

No, .bat files do not work in bash.

You're making a mistaken assumption that Windows users want to use cmd.exe. I used Linux for a decade. I want to (and can) use bash. MSYS2 and Git Bash bundled with Git for Windows provides a POSIX compatibility layer over the C runtime and filesystem (similar to Emscripten). I can even configure VS Code to run tasks and launch terminals with bash, because Microsoft supports that out of the box. It's mind blowing the first time you try it, but it's a standard thing these days. Just don't use symlinks.

sbc100 · 2020-04-13T21:08:33Z

I'm curious how other software interacts with MSYS. For example windows python. If you type python in bash does is know to looks for python.exe? Or do you explicitly need to type the `.exe on the end?

Also, if you can't run .bat files doesn't what about software which use .bat files as the entry points? Does that just not work with MSYS?

I'm also curious how @diegocr fix works, given that is does use symlinks (it seems to replace the fake symlinks with real ones) and also seems to solve the problem. What is the ln -s command actually doing in your script @diegocr ?

parasti · 2020-04-13T21:47:18Z

I'm curious how other software interacts with MSYS. For example windows python. If you type python in bash does is know to looks for python.exe? Or do you explicitly need to type the `.exe on the end?

I just type python like I would on Linux.

Also, if you can't run .bat files doesn't what about software which use .bat files as the entry points? Does that just not work with MSYS?

It doesn't. If I come across software like that, which is actually rare, I make a launcher shell script for it. I have no use for .bat files except as reference for the shell script implementation.

I'm also curious how @diegocr fix works, given that is does use symlinks (it seems to replace the fake symlinks with real ones) and also seems to solve the problem.

ln -s on MSYS does a copy by default.

sbc100 · 2020-04-13T22:23:04Z

Ok, sounds like its worth fixing the shell launchers to work under MSYS.

Are you mostly concerned about the emsdk use case? Or do you want git clone of emscripten to work out-of-the-box too?

parasti · 2020-04-14T04:08:23Z

Ths way it's set up is perfect. Symlinks are the only problem, really.

sbc100 · 2020-04-14T22:08:47Z

Would you be OK with the emsdk image containing the fix to goes do you think it needs to go all the way back the git content?

sbc100 · 2020-04-14T23:05:03Z

OK, lets try this: #10921

diegocr · 2020-04-18T10:39:37Z

Hi guys, sorry for the late reply - I've just installed 1.39.13 and all is good now, Thanks!

yeputons · 2020-04-27T08:58:30Z

tools/run_python.sh

+  PYTHON=$(which python3)
+fi
+
+if [ -z $PYTHON ]; then


Note that it's possible for Python to be installed in a path with spaces, which breaks this line: #11005

sbc100 requested a review from kripken March 19, 2020 16:20

kripken reviewed Mar 19, 2020

View reviewed changes

sbc100 force-pushed the sh_entry_points branch from 04fc003 to a85e143 Compare March 19, 2020 22:40

kripken approved these changes Mar 20, 2020

View reviewed changes

sbc100 force-pushed the sh_entry_points branch from a85e143 to 4efb17c Compare March 20, 2020 17:28

sbc100 merged commit d5c77b0 into master Mar 20, 2020

sbc100 deleted the sh_entry_points branch March 20, 2020 18:23

sbc100 mentioned this pull request Mar 21, 2020

Fix windows .bat launchers #10743

Merged

sbc100 mentioned this pull request Apr 16, 2020

emscripten.Building.configure causes error #10939

Closed

yeputons mentioned this pull request Apr 27, 2020

Syntax errors in em++ when Python is installed in a directory with spaces #11005

Closed

yeputons reviewed Apr 27, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove python_selector.py in favor of run_python.sh #10729

Remove python_selector.py in favor of run_python.sh #10729

sbc100 commented Mar 19, 2020

kripken left a comment

kripken Mar 19, 2020

sbc100 Mar 19, 2020

kripken Mar 20, 2020

sbc100 Mar 20, 2020

kripken Mar 19, 2020

sbc100 Mar 19, 2020

sbc100 commented Mar 19, 2020

sbc100 commented Mar 19, 2020

kripken Mar 20, 2020

diegocr commented Apr 12, 2020

diegocr commented Apr 12, 2020

sbc100 commented Apr 12, 2020

parasti commented Apr 13, 2020

parasti commented Apr 13, 2020

sbc100 commented Apr 13, 2020 •

edited

Loading

parasti commented Apr 13, 2020

sbc100 commented Apr 13, 2020

parasti commented Apr 13, 2020 •

edited

Loading

sbc100 commented Apr 13, 2020

parasti commented Apr 14, 2020

sbc100 commented Apr 14, 2020

sbc100 commented Apr 14, 2020

diegocr commented Apr 18, 2020

yeputons Apr 27, 2020

Remove python_selector.py in favor of run_python.sh #10729

Remove python_selector.py in favor of run_python.sh #10729

Conversation

sbc100 commented Mar 19, 2020

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Mar 19, 2020

sbc100 commented Mar 19, 2020

Choose a reason for hiding this comment

diegocr commented Apr 12, 2020

diegocr commented Apr 12, 2020

sbc100 commented Apr 12, 2020

parasti commented Apr 13, 2020

parasti commented Apr 13, 2020

sbc100 commented Apr 13, 2020 • edited Loading

parasti commented Apr 13, 2020

sbc100 commented Apr 13, 2020

parasti commented Apr 13, 2020 • edited Loading

sbc100 commented Apr 13, 2020

parasti commented Apr 14, 2020

sbc100 commented Apr 14, 2020

sbc100 commented Apr 14, 2020

diegocr commented Apr 18, 2020

Choose a reason for hiding this comment

sbc100 commented Apr 13, 2020 •

edited

Loading

parasti commented Apr 13, 2020 •

edited

Loading