Noissue - Append build name to jsshell directory

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_DIR = "mozilla-esr31"
 JSSHELL_URL = ("https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly"
                "/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_DIR)
 
+  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()
+    ))
+
+  shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)

[Sebastian Noack, 2015/11/24 10:38:43]

Nit: "%s-%s" % (JSSHELL_DIR, build)

[kzar, 2015/11/24 10:43:20]

I think it looks cleaner as is.

os.path.join(baseDir, JSSHELL_DIR + "-" + build)

vs

os.path.join(baseDir, "%s-%s" % (JSSHELL_DIR, build))

[Sebastian Noack, 2015/11/24 10:52:03]

I would generally disagree, if it's only for consistence with other "new" code,
which always uses % when concatenating more than two strings. This is also
faster though that isn't a concern here. But most of the time it's also more
readable. In the case here I see that it's not much a difference in matter of
readability though. And certainly not worth arguing about. LGTM

[Wladimir Palant, 2015/11/24 11:12:40]

I tend to agree with Sebastian here but indeed not important enough.

   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()
-    ))
-
   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