Issue 4373 - Made jshydra compatible with Python 3

Issue 4373 - Made jshydra compatible with Python 3

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

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (left):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#oldc...
abp_rewrite.py:1: #!/usr/bin/env python
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.

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#oldc...
abp_rewrite.py:27: return subprocess.check_output(command,
env=env).replace('\r', '')
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.

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:13: def rewrite_js(args, script=None):
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.

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini#newcode6
tox.ini:6: commands = python autotest.py
Note that in order to test all code paths, we'd have to make sure that there is
no cached mozilla* directory, then run utils.ensureJSShell(), and then make sure
it runs again. That would be easy enough here:

  commands =
    rm -rf mozilla*
    python -c 'import utils; utils.ensureJSShell()
    python autotest.py

But then we'd always have to download jsshell (once per environment!) on every
run. This would make the tests extremely slow, impossible to work offline, and
causing unnecessary load on Mozilla's servers.

Theoretically, we could bundle a version of the jsshell package with the tests
of course. However, the whole point of that code is to avoid bundling jsshell
with our code.

So I guess, I will just leave it as it is.

https://codereview.adblockplus.org/29350236/diff/29350250/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/utils.py#newcode40
utils.py:40: system = platform.system()
sys.platform behaves inconsistently since Python 3.3 and before. The value for
Linux changed from 'linux2' to 'linux'. However, platform.system() isn't
effected and can be used instead, without any compatibility hack.

[Vasily Kuznetsov, 2016-08-29 15:29:53]

Looks good in general. What do you think about the proposal to add environments
to tox.ini to test the downloading in Py2/Py3 (see below)?

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (left):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#oldc...
abp_rewrite.py:1: #!/usr/bin/env python
On 2016/08/28 22:36:13, Sebastian Noack wrote:
> 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.

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.

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini#newcode6
tox.ini:6: commands = python autotest.py
On 2016/08/28 22:36:14, Sebastian Noack wrote:
> Note that in order to test all code paths, we'd have to make sure that there
is
> no cached mozilla* directory, then run utils.ensureJSShell(), and then make
sure
> it runs again. That would be easy enough here:
> 
>   commands =
>     rm -rf mozilla*
>     python -c 'import utils; utils.ensureJSShell()
>     python autotest.py
> 
> But then we'd always have to download jsshell (once per environment!) on every
> run. This would make the tests extremely slow, impossible to work offline, and
> causing unnecessary load on Mozilla's servers.
> 
> Theoretically, we could bundle a version of the jsshell package with the tests
> of course. However, the whole point of that code is to avoid bundling jsshell
> with our code.
> 
> So I guess, I will just leave it as it is.

What you say makes sense, however, downloading uses slightly different code
paths in Python2/3 so perhaps it would be good to add (not included by default)
environments that would do the `rm -rf ...` and a comment about why they are
needed. This way this knowledge will not only be in this review.

https://codereview.adblockplus.org/29350236/diff/29350250/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/utils.py#newcode40
utils.py:40: system = platform.system()
On 2016/08/28 22:36:14, Sebastian Noack wrote:
> sys.platform behaves inconsistently since Python 3.3 and before. The value for
> Linux changed from 'linux2' to 'linux'. However, platform.system() isn't
> effected and can be used instead, without any compatibility hack.

Acknowledged.

[Sebastian Noack, 2016-08-30 11:44:39]

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:5: from __future__ import print_function
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.

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini#newcode6
tox.ini:6: commands = python autotest.py
On 2016/08/29 15:29:52, Vasily Kuznetsov wrote:
> On 2016/08/28 22:36:14, Sebastian Noack wrote:
> > Note that in order to test all code paths, we'd have to make sure that there
> is
> > no cached mozilla* directory, then run utils.ensureJSShell(), and then make
> sure
> > it runs again. That would be easy enough here:
> > 
> >   commands =
> >     rm -rf mozilla*
> >     python -c 'import utils; utils.ensureJSShell()
> >     python autotest.py
> > 
> > But then we'd always have to download jsshell (once per environment!) on
every
> > run. This would make the tests extremely slow, impossible to work offline,
and
> > causing unnecessary load on Mozilla's servers.
> > 
> > Theoretically, we could bundle a version of the jsshell package with the
tests
> > of course. However, the whole point of that code is to avoid bundling
jsshell
> > with our code.
> > 
> > So I guess, I will just leave it as it is.
> 
> What you say makes sense, however, downloading uses slightly different code
> paths in Python2/3 so perhaps it would be good to add (not included by
default)
> environments that would do the `rm -rf ...` and a comment about why they are
> needed. This way this knowledge will not only be in this review.

There you go, however, I decided to check for an optional argument, rather than
using separate environments. The main problem with using a separate environment
(besides being unnecessary) is that we want to test that code paths on all
Python versions. That would be possible with a factor condition, but you would
still have to explicitly specify all Python versions when running tox (i.e. "tox
-e py{27,35}-nocache").

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py#newcode13
utils.py:13: import urllib2
It seems more resonable to fallback to urllib2 (rather than urllib) on Python 2,
since it is closer to the semantics of urllib.request on Python 3, e.g. it
raises when encountering bad HTTP responses.

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

LGTM

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py
File abp_rewrite.py (right):

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:5: from __future__ import print_function
On 2016/08/30 11:44:38, Sebastian Noack wrote:
> 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.

Acknowledged.

https://codereview.adblockplus.org/29350236/diff/29350250/abp_rewrite.py#newc...
abp_rewrite.py:13: def rewrite_js(args, script=None):
On 2016/08/28 22:36:13, Sebastian Noack wrote:
> 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.

Acknowledged.

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29350236/diff/29350250/tox.ini#newcode6
tox.ini:6: commands = python autotest.py
On 2016/08/30 11:44:38, Sebastian Noack wrote:
> On 2016/08/29 15:29:52, Vasily Kuznetsov wrote:
> > On 2016/08/28 22:36:14, Sebastian Noack wrote:
> > > Note that in order to test all code paths, we'd have to make sure that
there
> > is
> > > no cached mozilla* directory, then run utils.ensureJSShell(), and then
make
> > sure
> > > it runs again. That would be easy enough here:
> > > 
> > >   commands =
> > >     rm -rf mozilla*
> > >     python -c 'import utils; utils.ensureJSShell()
> > >     python autotest.py
> > > 
> > > But then we'd always have to download jsshell (once per environment!) on
> every
> > > run. This would make the tests extremely slow, impossible to work offline,
> and
> > > causing unnecessary load on Mozilla's servers.
> > > 
> > > Theoretically, we could bundle a version of the jsshell package with the
> tests
> > > of course. However, the whole point of that code is to avoid bundling
> jsshell
> > > with our code.
> > > 
> > > So I guess, I will just leave it as it is.
> > 
> > What you say makes sense, however, downloading uses slightly different code
> > paths in Python2/3 so perhaps it would be good to add (not included by
> default)
> > environments that would do the `rm -rf ...` and a comment about why they are
> > needed. This way this knowledge will not only be in this review.
> 
> There you go, however, I decided to check for an optional argument, rather
than
> using separate environments. The main problem with using a separate
environment
> (besides being unnecessary) is that we want to test that code paths on all
> Python versions. That would be possible with a factor condition, but you would
> still have to explicitly specify all Python versions when running tox (i.e.
"tox
> -e py{27,35}-nocache").

Great, this is even better.

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350236/diff/29350304/utils.py#newcode13
utils.py:13: import urllib2
On 2016/08/30 11:44:39, Sebastian Noack wrote:
> It seems more resonable to fallback to urllib2 (rather than urllib) on Python
2,
> since it is closer to the semantics of urllib.request on Python 3, e.g. it
> raises when encountering bad HTTP responses.

Acknowledged.