Issue 2956 - Ensure the correct version of jsshell is present

utils.py

 # 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/.
 
+from contextlib import closing
 import os
 import platform
 from StringIO import StringIO
 import sys
 import urllib
 import zipfile
 
-JSSHELL_VERSION = "31.5.0"
+JSSHELL_DIR = "mozilla-esr31"
 JSSHELL_URL = ("https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly"
-               "/2015/02/2015-02-25-00-22-19-mozilla-esr31/jsshell-%s.zip")
+               "/2015/02/2015-02-25-00-22-19-%s/jsshell-%%s.zip" % JSSHELL_DIR)
 
 JSSHELL_SUPPORTED_PLATFORMS = {
   "win32": "win32",
   "linux2": {
     "i686": "linux-i686",
     "x86_64": "linux-x86_64"
   },
   "darwin": "mac"
 }
 
 def ensureJSShell():
   baseDir = os.path.dirname(__file__)
-  shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)

[Sebastian Noack, 2015/11/23 12:53:58]

With the new approach, hard-coding the version is redundant. On the other hand
calling "js --help" to automatically get the version would add some more
complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31" part
from the URL as directory name?

[kzar, 2015/11/23 13:20:21]

Hmm I see what you mean, but on the other hand having the directory called
something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty ugly
compared to jsshell-31.5.0/. Mind if I leave this as it is?

[Sebastian Noack, 2015/11/23 13:27:48]

How about following?

JSSHELL_DIR = "mozilla-esr31"
JSSHELL_URL = "..." % JSSHELL_DIR

[kzar, 2015/11/23 13:38:12]

That's better but still kind of sucks, the directory would be mozilla-esr31/
instead of jsshell-31.5.0/ which IMO is less descriptive. I also don't like how
the directory name would then be tied to the URL. If we wanted to upgrade to
jsshell 31.6.0 for example we would be stuck using the same directory name.

[Sebastian Noack, 2015/11/23 13:51:16]

Then include the timestamp if that is your concern as I initially suggested.
However independently/redundantly configuring the version isn't justified just
for a leaner dirname. Also note that the current dirnames is just "mozilla" and
improving the semantics of it isn't matter of this change anyway.

[Sebastian Noack, 2015/11/23 13:51:16]

Then include the timestamp if that is your concern as I initially suggested.
However independently/redundantly configuring the version isn't justified just
for a leaner dirnames. Also note that the current dirnames is just "mozilla" and
improving the semantics of it isn't matter of this patch anyway.

[kzar, 2015/11/23 14:01:31]

Fine, Done.

+  shell_dir = os.path.join(baseDir, JSSHELL_DIR)
 
   if not os.path.exists(shell_dir):
     os.makedirs(shell_dir)
   if sys.platform == 'win32':
     path = os.path.join(shell_dir, 'js.exe')
   else:
     path = os.path.join(shell_dir, 'js')
 
   if os.path.exists(path):
     return path
 
   try:
     build = JSSHELL_SUPPORTED_PLATFORMS[sys.platform]
     if isinstance(build, dict):
       build = build[platform.machine()]
   except KeyError:
     raise Exception('Platform %s (%s) not supported by JS shell' % (
       sys.platform, platform.machine()
     ))
 
-  data = StringIO(urllib.urlopen(JSSHELL_URL % build).read())

[Sebastian Noack, 2015/11/23 12:53:58]

While changing this code anyway, mind closing the file-like object returned by
urlopen()?

[kzar, 2015/11/23 13:20:21]

Done.

-  zip = zipfile.ZipFile(data)
-  zip.extractall(shell_dir)
-  zip.close()
+  with closing(urllib.urlopen(JSSHELL_URL % build)) as response, \
+       zipfile.ZipFile(StringIO(response.read())) as zip:
+    zip.extractall(shell_dir)
 
   if not os.path.exists(path):
     raise Exception('Downloaded package didn\'t contain JS shell executable')
 
   try:
     os.chmod(path, 0700)
   except:
     pass
 
   return path