Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3 compatibility: Use print() function #5610

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

saschanaz
Copy link
Collaborator

@saschanaz saschanaz commented Sep 28, 2017

I got some help from 2to3 --f print --f exec to fix the Python 3 syntax compatibility in progressive way.

Thus, this PR includes these changes:

// Python 2 only
print "Hello"
exec "code"
// Python 2 and 3
print("Hello")
exec("code")

The next phases would be error handlings, imports, maps/filters and so on.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall this looks good. I just have the one question in a comment above, that I'm not sure about.

@@ -1,3 +1,4 @@
from __future__ import print_function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure we should update external python files that have an upstream. in this file's case, we never use it, it's just part of bullet and we included it all. so i guess it would harmless to modify it, but it would mean we'd see a diff when comparing to upstream. again, in this case that's probably fine (I doubt we'll update bullet, or if we do, we'd do it as a whole, not incrementally).

so overall, I'm not sure what's best here.

Copy link
Collaborator Author

@saschanaz saschanaz Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems bullet, poppler, and freetype haven't been updated from upstream since 2011 so personally I think it will be fine.

If you want I'll still happily exclude them in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think eventually we will have to fix some of them, but probably only after we add CI settings for Python 3?)

@dschuff
Copy link
Member

dschuff commented Oct 2, 2017

Is the goal ultimately to move to python3 for all of emscripten?

@juj
Copy link
Collaborator

juj commented Oct 2, 2017

Is the goal ultimately to move to python3 for all of emscripten?

macOS does not ship python3 by default even in the latest version, so I don't think we want to migrate to requiring python3. I think we are in a good spot with respect to dual python 2 + python 3 support, by using the dynamic routing scripts to run python 2 if one invokes python 3, so from the shell perspective, invoking Emscripten does seem to work from python 3.

If someone does want to contribute and maintain "real" python 3 support, I don't think I would be opposed, but we do also want to support python 2 for a healthy amount of time before we would be able to abandon ship on that.

@juj
Copy link
Collaborator

juj commented Oct 2, 2017

Let's try to be extremely conservative with these kind of mass changes. Could we first migrate only everything that does not have an upstream, and everything that does not live in tests/ and site/.

Also, do we know what is the minimum imposed python 2 version after this kind of change?

@juj
Copy link
Collaborator

juj commented Oct 2, 2017

For emrun, that lives in upstream in https://github.com/juj/emrun.git, so let's leave that one out in here first (do the change via that repository)

@saschanaz
Copy link
Collaborator Author

saschanaz commented Oct 2, 2017

Could we first migrate only everything that does not have an upstream, and everything that does not live in tests/ and site/

I agree for the upstream thing, but why the whole tests/ and site/? It will definitely make the diff smaller, though.

PS: (I didn't know there is a separate repo for emrun 😮)

@saschanaz saschanaz force-pushed the python3-print branch 2 times, most recently from 8653eeb to 8449dd4 Compare October 11, 2017 07:21
@saschanaz
Copy link
Collaborator Author

Rebased and excluded third_party/, tests/ and site/. (Got a backup though)

@kripken
Copy link
Member

kripken commented Oct 11, 2017

Running tests locally, I see a failure on

test_dylink_zlib (test_core.default) ... (checking sanity from test runner)
INFO:root:(Emscripten: Running sanity checks)
  File "/media/alon/2f9a30d7-6124-42d9-87c5-3c80cb70ec54/home/alon/Dev/emscripten/embuilder.py", line 136
    print 'Building targets: %s' % ' '.join (tasks)
                               ^
SyntaxError: invalid syntax
ERROR:root:/home/alon/.emscripten_cache/asmjs/ports-builds/zlib/libz.a: No such file or directory ("/home/alon/.emscripten_cache/asmjs/ports-builds/zlib/libz.a" was expected to be an input file, based on the commandline arguments provided)
ERROR

@saschanaz
Copy link
Collaborator Author

Fixed it, thanks!

@kripken
Copy link
Member

kripken commented Oct 11, 2017

Thanks! How about merging the just-merged incoming to here? It would include your travis testing support, so it could also verify everything works.

@saschanaz
Copy link
Collaborator Author

Rebased again, the test passes 😃 (at least on my repository).

@saschanaz
Copy link
Collaborator Author

Also, do we know what is the minimum imposed python 2 version after this kind of change?

from __future__ import print_function requires Python 2.6.

@saschanaz saschanaz closed this Oct 12, 2017
@saschanaz saschanaz reopened this Oct 12, 2017
@kripken
Copy link
Member

kripken commented Oct 12, 2017

Looking in the docs, we seem to mention 2.7 already almost everywhere, which is good. But I think we should update site/source/docs/building_from_source/toolchain_what_is_needed.rst and site/source/docs/building_from_source/building_emscripten_from_source_on_windows.rst to replace 2.* with 2.6. Although we already recommend 2.7 in several places, so maybe we can just say 2.7?

@dschuff
Copy link
Member

dschuff commented Oct 12, 2017

Python 2.7 has been out for 7 years, I think it's safe to require that.

@juj
Copy link
Collaborator

juj commented Oct 12, 2017

Yeah, Python 2.6 is something we do not want to support. What I'm curious is if there are some things that would require some specific newer version than Python 2.7.0, and if so, that's alright as well, but good to be diligent and mention those somewhere.

@juj
Copy link
Collaborator

juj commented Oct 12, 2017

I agree for the upstream thing, but why the whole tests/ and site/? It will definitely make the diff smaller, though.

My thinking here was the reason to make the diff smaller, and have those dealt with in separate pull requests. The tests/ directory contains a lot of third party code, so I don't think we want to change any of those, unless the upstream project is unmaintained.

PS: (I didn't know there is a separate repo for emrun 😮)

Thanks for the PR there!

@juj
Copy link
Collaborator

juj commented Oct 12, 2017

This PR looks good to me to merge.

@kripken kripken merged commit f043f19 into emscripten-core:incoming Oct 16, 2017
@kripken
Copy link
Member

kripken commented Oct 16, 2017

As a followup we should update those docs to say python 2.7 consistently.

@saschanaz saschanaz deleted the python3-print branch October 17, 2017 00:39
@DopefishJustin
Copy link
Contributor

This breaks emcc.py for me:

Traceback (most recent call last):
  File "/0/home/dfjustin/emscripten/em++", line 15, in <module>
    import emcc
  File "/0/home/dfjustin/emscripten/emcc.py", line 887
    exec('shared.Settings.' + key + ' = ' + value, globals(), locals())
SyntaxError: unqualified exec is not allowed in function 'run' it contains a nested function with free variables
$ python -V
Python 2.7.3

@saschanaz
Copy link
Collaborator Author

Hmm, it seems it is a old Python bug that has been fixed back in 2014. https://bugs.python.org/issue21591

This effectively makes the minimum compatible Python version to 2.7.9. Hmm.

@dschuff
Copy link
Member

dschuff commented Oct 18, 2017

#5685 fixes that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants