Issue 2956 - Ensure the correct version of jsshell is present

utils.py

Index: utils.py
diff --git a/utils.py b/utils.py
index ac3014ae675e7cf67fd33965c7f4f5a2eb13195b..34cadb7a91ab42d3751102bd8a7d2b2927c28f12 100644
--- a/utils.py
+++ b/utils.py
@@ -11,28 +11,35 @@ import sys
 import urllib
 import zipfile
 
+JSSHELL_VERSION = "31.5.0"
+JSSHELL_URL = ("https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly"
+               "/2015/02/2015-02-25-00-22-19-mozilla-esr31/jsshell-%s.zip")
+
+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, 'mozilla')
+  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.

+
   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
 
-  supported_platforms = {
-    'win32': 'win32',
-    'linux2': {
-      'i686': 'linux-i686',
-      'x86_64': 'linux-x86_64'
-    },
-    'darwin': 'mac',
-  }
   try:
-    build = supported_platforms[sys.platform]
+    build = JSSHELL_SUPPORTED_PLATFORMS[sys.platform]
     if isinstance(build, dict):
       build = build[platform.machine()]
   except KeyError:
@@ -40,8 +47,7 @@ def ensureJSShell():
       sys.platform, platform.machine()
     ))
 
-  download_url = 'https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/02/2015-02-25-00-22-19-mozilla-esr31/jsshell-%s.zip' % build
-  data = StringIO(urllib.urlopen(download_url).read())
+  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()