Issue 4373 - Made jshydra compatible with Python 3

abp_rewrite.py

-#!/usr/bin/env python

[Sebastian Noack, 2016/08/28 22:36:13]

In all environments (except for a Python 3 virtualenv) this would refer to
Python 2. But it is arguably preferable to run code that is compatible with
Python 3 on Python 3. However, if changing the shebang to "python3", the script
would only run successfully in environments that have (the right version of)
Python 3 (even if it could run using Python 2 in the same environment).

In the end it turned out that we don't need any executable scripts for jshydra.
This file is only imported and used as module. The |if __name__ == '__main__'|
part below was essentially dead code, so I removed it as well.

We still need to think of a solution for other scripts (e.g. the build.py in the
downstream repositories) though.

[Vasily Kuznetsov, 2016/08/29 15:29:52]

I'm not sure if it's always better to run Python3-compatible code with Python3.
For example people might have our dependencies installed in Python2 so that's
the only one that will work. I think shebang line with `python` gives people the
choice and is probably the best option unless the script is not compatible with
Python2 (in which case `python3` is justified).

General solution could be tricky, for the above mentioned reasons, however, if
we package our code as python packages with `setup.py` and use `console_scripts`
entry point for declaring the scripts, we don't need to have the shebang line
anywhere, so the problem goes away. This would be my preferred direction unless
there are good reasons to not go there.

-# coding: utf-8
-
 # This Source Code is subject to the terms of the Mozilla Public License
 # version 2.0 (the "License"). You can obtain a copy of the License at
 # http://mozilla.org/MPL/2.0/.
 
-import sys
+from __future__ import print_function

[Sebastian Noack, 2016/08/30 11:44:38]

I just noticed that this future import here isn't neccessary anymore. Since the
|__name__ == '__main__'| block has been removed, this module does not print
anything anymore.

[Vasily Kuznetsov, 2016/08/30 13:16:30]

Acknowledged.

+
 import os
 import subprocess
+
 import utils
 
 
-def doRewrite(files, args):
-    application = utils.ensureJSShell()
+def rewrite_js(args, script=None):

[Sebastian Noack, 2016/08/28 22:36:13]

Yes, this is a breaking change that needs to be adapted for when updating the
dependecy in buildtools. The reason for this change was (besides migrating to a
PEP-8 conform name) was to avoid duplication in autotest.py and actually use
this function there so that it gets tested as well.

[Vasily Kuznetsov, 2016/08/30 13:16:30]

Acknowledged.

+    jsshell = utils.ensureJSShell()
+    env = {'LD_LIBRARY_PATH': os.path.relpath(os.path.dirname(jsshell))}
+    base_dir = os.path.dirname(__file__)
 
-    env = {
-        'LD_LIBRARY_PATH': os.path.relpath(os.path.dirname(application)),
-    }
+    if not script:
+        script = os.path.join(base_dir, 'scripts', 'abprewrite.js')
 
-    baseDir = os.path.dirname(utils.__file__)
-    command = [
-        application, os.path.join(baseDir, 'jshydra.js'),
-        os.path.join(baseDir, 'scripts', 'abprewrite.js'),
-        '--arg', ' '.join(args)
-    ] + files
-    return subprocess.check_output(command, env=env).replace('\r', '')

[Sebastian Noack, 2016/08/28 22:36:14]

For reference, this failed in Python 3, because we are getting a byte string but
are replacing unicode strings. One way to fix that would have been calling
|.replace(b'\r', b'')| instead. However, that is what universal_newlines is for
in the first place.

-
-if __name__ == '__main__':
-    try:
-        scriptArgsStart = sys.argv.index('--arg')
-    except ValueError:
-        scriptArgsStart = len(sys.argv)
-    print doRewrite(sys.argv[1:scriptArgsStart], sys.argv[scriptArgsStart + 1:])
+    command = [jsshell, os.path.join(base_dir, 'jshydra.js'), script] + args
+    return subprocess.check_output(command, env=env, universal_newlines=True)