Allow Python 3 (second try) #5967

saschanaz · 2017-12-22T07:09:55Z

Replaces #5955

This PR:

introduces EMSCRIPTEN_ALLOW_NEWER_PYTHON to allow Python 3
creates force_python_version.py to conditionally force Python 2
separates colored logger as colored_logger.py
uses emcc.py instead of emcc internally
adds a test
lets CircleCI test on Python 3 while Travis still tests on Python 2

kripken · 2017-12-24T18:43:37Z

em++

-  if __name__ == '__main__':
-    sys.exit(subprocess.call(['python2', os.path.join(os.path.dirname(os.path.realpath(__file__)), 'emcc.py')] + sys.argv[1:]))
+emcc = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'emcc')
+if __name__ == '__main__' and not force_python_version.run(emcc):


This line reads unclearly to me. It seems odd that the check for __main__ is related to a check for the python version, and also it's a little surprising that force_python_version returns a result saying whether it ran the command or not.

Could force_python_version call sys.exit to quit, if it ran the command? Then we could just call it inside an if on __main__ as a command, not a condition, that might be clearer?

(Otherwise I think this PR is good!)

Could force_python_version call sys.exit to quit, if it ran the command?

Actually it already does, and I just updated force_python_version to also automate import emcc; emcc.run().

saschanaz · 2017-12-25T02:57:30Z

test_merge_pair randomly fails and I have no idea why...

saschanaz · 2017-12-25T06:35:09Z

Turns out that apply_funcs_map in asm_module.py assumes self.funcs is ordered but actually not.

https://github.com/kripken/emscripten/blob/c5a455873a5ee84ac07961e06a3a58204e5813b2/tools/asm_module.py#L312

kripken

Nice work here!

One more comment, we should measure that these changes don't affect build times. It seems like it shouldn't - same amount of imports/subprocess invocations - but still. Seeing how fast libc or zlib are build, for example.

kripken · 2018-01-02T18:20:26Z

tests/test_other.py

      if has:
        print('  checking emcc...')
-        check_execute([python, EMCC, '--version'])
+        check_execute([python, EMCC[:-3], '--version'])


shouldn't it work with or without .py? Maybe I don't remember why the test checks the .py version later...

Yes, it works with or without it. I did this because the test specifically wants to test emcc.

Could this be a def remove_py_suffix() function or similar that looks for .py at the end and removes that? This could be a bit brittle otherwise for future refactors in mind, if one unconditionally just removes three last chars here?

kripken · 2018-01-02T18:22:28Z

tools/force_python_version.py

+  import subprocess
+  return subprocess.run(py2 + [os.path.realpath(filename) + '.py'] + sys.argv[1:]).returncode
+
+def allowed_version():


how about is_allowed_version, since this checks it, and doesn't return the allowed version?

kripken · 2018-01-02T18:23:33Z

tools/force_python_version.py

@@ -0,0 +1,43 @@
+'''


how about calling this just python_version, or python_version_handling or something like that, since it also provides utilities to check it etc.?

kripken · 2018-01-02T18:26:03Z

tools/shared.py

-EMCC = path_from_root('emcc')
-EMXX = path_from_root('em++')
-EMAR = path_from_root('emar')
+EMCC = path_from_root('emcc.py')


why, and why these three (and not emranlib just below etc.)?

Calling emcc recursively will cause multiple duplicated Python version warnings

There are many tests that call [PYTHON, EMCC] and expect empty stderr output. A Python version warning will break all of them.

The version warning occurs only on emcc wrapper so this change will remove the problems.

emranlib is just a no-op, em-config already allows Python 3, so no need to change. (Do we want it to require Python 2?)

kripken · 2018-01-02T18:26:31Z

tools/shared.py

        if env.get(dangerous) and env.get(dangerous) == non_native.get(dangerous):
          del env[dangerous] # better to delete it than leave it, as the non-native one is definitely wrong
      return env
-    env['CC'] = quote(EMCC) if not WINDOWS else 'python %s' % quote(EMCC)


Calling emcc internally will cause multiple duplicated Python version warnings

I think they are anyway equivalent, is there a specific need to do this conditionally?

Yeah, hopefully we don't need the conditional stuff. @juj, was it working around a windows issue?

I think it's safe to do this, as we should be calling [PYTHON, EMCC] anyhow everywhere, including windows.

On Windows it's not possible to invoke /path/to/emscripten/emcc when emcc is a python script. (there's no concept of reading a shebang from the file on Windows), but one always needs to do python /path/to/emscripten/emcc to run the Python script.

That's why we have the emcc.bat file on Windows. If one invokes the subprocess with "shell expansion" enabled, then /path/to/emscripten/emcc works, and looks up /path/to/emscripten/emcc.bat. However shell expansion is sometimes considered poor form in subprocess invocations, which is why we'd prefer to disable shell expansion when invoking, and explicitly call the batch file /path/to/emscripten/emcc.bat.

To summarize, on Windows

subprocess.run(['c:/emscripten/emcc`]) # Fails, emcc is a py script, can't execute subprocess.run(['c:/emscripten/emcc.py`]) # Fails, emcc.py is a py script, can't execute subprocess.run(['c:/emscripten/emcc.py`], shell=True) # Fails, emcc.py is a py script, can't execute subprocess.run(['c:/emscripten/emcc`], shell=True) # Works, and invokes emcc.bat subprocess.run(['c:/emscripten/emcc.bat`], shell=True) # Works, and invokes emcc.bat subprocess.run(['c:/emscripten/emcc.bat`]) # Works, and invokes emcc.bat, preferred subprocess.run(['python', 'c:/emscripten/emcc`]) # Works, and invokes emcc python script, slightly more preferred compared to the .bat script to avoid py -> batch -> py roundtripping subprocess.run(['python', 'c:/emscripten/emcc.py`]) # Works, and invokes emcc.py script, slightly more preferred compared to the .bat script to avoid py -> batch -> py roundtripping

We prefer to avoid shell=True whenever possible, since it expands PATHs, %s and possibly $s depending on the calling terminal (PowerShell, MinGW, CygWin, MSYS). Since we're already being explicit with the path where emcc executable resides, it's nicer to explicitly say "emcc.bat is here on this absolute path, call this one", or "run this py script on python".

Thanks for great explanation! Yes, we need to explicitly call python on Windows, by is there a specific need to do this conditionally I meant that we can also do same thing on non-Windows: env['CC'] = 'python %s' % quote(EMCC)

I should have been clearer, sorry.

Oh right, I see now I kind of was explaining a separate issue. Yeah, the original adjustment was about bff4d2b, and I agree that it should also work on non-Windows, so this change is good.

saschanaz · 2018-01-03T01:52:05Z

I tried renaming force_python_version to python_selector as python_version.run() looks unintuitive to me. What do you think?

kripken · 2018-01-04T19:30:33Z

I prefer the new names (python_selector, on_correct_version), but just a matter of opinion I guess. @juj, what do you think?

juj · 2018-01-05T15:47:01Z

I like python_selector as a name much better as well rather than force_python_version, since the name force always reads a bit like a "patched this override after the fact on someone else's code" flow.

juj · 2018-01-10T14:18:26Z

This PR looks good to merge to me now, any remaining concerns? Were there still any random test failures?

saschanaz · 2018-01-11T02:20:39Z

I'm not seeing any random failures now, I think it's safe to merge.

kripken · 2018-01-11T20:46:26Z

@saschanaz , great work here and in all the PRs with Python3 support leading to here :)

cpitclaudel · 2018-03-25T02:57:32Z

Thanks for the great work! But this broke some of my builds :/ (I just updated to 1.37.36).

The problem is this part of get_building_env:

    env['CC'] = 'python %s' % quote(EMCC)
    env['CXX'] = 'python %s' % quote(EMXX)
    env['AR'] = 'python %s' % quote(EMAR)
    env['LD'] = 'python %s' % quote(EMCC)

This used to just point to EMCC, EMAR, etc. The change causes some configure scripts to choke as they detect tools: they call which "python /vagrant/z3-js/emsdk-portable/emscripten/1.37.36/emar.py" to check for the appropriate binary, and promptly fail. (The particular case where I'm seeing this is Z3).

kripken · 2018-03-25T16:24:49Z

Thanks @cpitclaudel for the bug report. Can you please open a new issue for that, with a testcase showing the failure? (would be very helpful if you can extract the relevant part of the Z3 configure script for that, then we can use it in testing)

cpitclaudel · 2018-03-25T22:24:34Z

Sure, with pleasure :) #6378

saschanaz added 5 commits December 24, 2017 17:27

test on python 3

0183a1b

separate colored_logger

9dfea7a

use force_python_version

393a204

use .py internally

b58bcf4

add test

d373f8f

saschanaz force-pushed the py3-3 branch from eca1d9d to d373f8f Compare December 24, 2017 08:27

saschanaz mentioned this pull request Dec 24, 2017

Allow Python 3 #5955

Closed

kripken reviewed Dec 24, 2017

View reviewed changes

support run_by_import

da7f75b

self.funcs should be ordered

20c96c0

subprocess.run is always recommended

84c8781

kripken reviewed Jan 2, 2018

View reviewed changes

rename module

67dac28

trim_py_suffix

a590bf3

kripken merged commit f067c2a into emscripten-core:incoming Jan 11, 2018

saschanaz deleted the py3-3 branch January 12, 2018 00:02

This was referenced Jan 12, 2018

Fix test_iostream_and_determinism test failure #6067

Merged

Initial NODERAWFS implementation #6008

Merged

Allow running on Python 3? #5950

Closed

cpitclaudel mentioned this pull request Mar 25, 2018

Setting CXX to 'python …/emcc.py' confuses some configure scripts (follow-up to #5967) #6378

Closed

sy2002 mentioned this pull request Feb 2, 2019

Emscripten build issue on Mac floooh/sokol-samples#18

Closed

Allow Python 3 (second try) #5967

Allow Python 3 (second try) #5967

Uh oh!

Conversation

saschanaz commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Dec 25, 2017

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Jan 3, 2018

Uh oh!

kripken commented Jan 4, 2018

Uh oh!

juj commented Jan 5, 2018

Uh oh!

juj commented Jan 10, 2018

Uh oh!

saschanaz commented Jan 11, 2018

Uh oh!

kripken commented Jan 11, 2018

Uh oh!

cpitclaudel commented Mar 25, 2018

Uh oh!

kripken commented Mar 25, 2018

Uh oh!

cpitclaudel commented Mar 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

saschanaz commented Dec 22, 2017 •

edited

Loading

saschanaz commented Dec 25, 2017 •

edited

Loading

saschanaz Jan 3, 2018 •

edited

Loading