Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2)

Issue 29330389: Issue 2956 - Ensure the correct version of jsshell is present (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 3 months ago by kzar
Modified:
4 years, 3 months ago
CC:
Oleksandr, sergei
Visibility:
Public.

Description

Issue 2956 - Ensure the correct version of jsshell is present

Patch Set 1 #

Total comments: 6

Patch Set 2 : Use regexp to match version number and tidy things up #

Total comments: 2

Patch Set 3 : Base directory name on jsshell version, avoid checking which version the binary outputs #

Total comments: 9

Patch Set 4 : Make sure to close the urllib response object #

Total comments: 2

Patch Set 5 : Don't download jsshell twice! #

Patch Set 6 : Remove JSSHELL_VERSION variable #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -17 lines) Patch
M .gitignore View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M .hgignore View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M utils.py View 1 2 3 4 5 3 chunks +21 lines, -15 lines 0 comments Download

Messages

Total messages: 21
kzar
Patch Set 1
4 years, 3 months ago (2015-11-18 14:58:06 UTC) #1
sergei
https://codereview.adblockplus.org/29330389/diff/29330390/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35 utils.py:35: if version == "JavaScript-C31.5.0esrpre": Nit: I don't know the ...
4 years, 3 months ago (2015-11-18 15:30:07 UTC) #2
kzar
https://codereview.adblockplus.org/29330389/diff/29330390/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35 utils.py:35: if version == "JavaScript-C31.5.0esrpre": On 2015/11/18 15:30:06, sergei wrote: ...
4 years, 3 months ago (2015-11-18 15:40:50 UTC) #3
sergei
https://codereview.adblockplus.org/29330389/diff/29330390/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35 utils.py:35: if version == "JavaScript-C31.5.0esrpre": On 2015/11/18 15:40:49, kzar wrote: ...
4 years, 3 months ago (2015-11-18 15:50:18 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330390/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode30 utils.py:30: version = [s for s in subprocess.check_output([path, "--help"]).split("\n") This ...
4 years, 3 months ago (2015-11-18 19:53:37 UTC) #5
kzar
Patch Set 2 : Use regexp to match version number and tidy things up https://codereview.adblockplus.org/29330389/diff/29330390/utils.py ...
4 years, 3 months ago (2015-11-19 10:56:42 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330416/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330416/utils.py#newcode39 utils.py:39: shutil.rmtree(shell_dir) I'm not sure whether this is a good ...
4 years, 3 months ago (2015-11-19 20:21:43 UTC) #7
kzar
https://codereview.adblockplus.org/29330389/diff/29330416/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330416/utils.py#newcode39 utils.py:39: shutil.rmtree(shell_dir) On 2015/11/19 20:21:43, Sebastian Noack wrote: > I'm ...
4 years, 3 months ago (2015-11-20 09:02:00 UTC) #8
kzar
Patch Set 3 : Base directory name on jsshell version, avoid checking which version the ...
4 years, 3 months ago (2015-11-23 12:40:49 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29 utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION) With the new ...
4 years, 3 months ago (2015-11-23 12:53:59 UTC) #10
kzar
Patch Set 4 : Make sure to close the urllib response object https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py ...
4 years, 3 months ago (2015-11-23 13:20:22 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29 utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION) On 2015/11/23 13:20:21, ...
4 years, 3 months ago (2015-11-23 13:27:48 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330697/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330697/utils.py#newcode52 utils.py:52: ZipFile(StringIO(urllib.urlopen(JSSHELL_URL % build).read())) as zip: Ehmm... Guess what's wrong ...
4 years, 3 months ago (2015-11-23 13:31:00 UTC) #13
kzar
https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29 utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION) On 2015/11/23 13:27:48, ...
4 years, 3 months ago (2015-11-23 13:38:12 UTC) #14
kzar
Patch Set 5 : Don't download jsshell twice! https://codereview.adblockplus.org/29330389/diff/29330697/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330697/utils.py#newcode52 utils.py:52: ZipFile(StringIO(urllib.urlopen(JSSHELL_URL ...
4 years, 3 months ago (2015-11-23 13:44:11 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29 utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION) On 2015/11/23 13:38:12, ...
4 years, 3 months ago (2015-11-23 13:51:16 UTC) #16
kzar
Patch Set 6 : Remove JSSHELL_VERSION variable https://codereview.adblockplus.org/29330389/diff/29330679/utils.py File utils.py (right): https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29 utils.py:29: shell_dir = ...
4 years, 3 months ago (2015-11-23 14:01:31 UTC) #17
Sebastian Noack
LGTM
4 years, 3 months ago (2015-11-23 14:19:56 UTC) #18
Wladimir Palant
Actually, if we are using different directory names already it would make sense to have ...
4 years, 3 months ago (2015-11-24 09:25:20 UTC) #19
Sebastian Noack
On 2015/11/24 09:25:20, Wladimir Palant wrote: > Actually, if we are using different directory names ...
4 years, 3 months ago (2015-11-24 09:35:54 UTC) #20
kzar
4 years, 3 months ago (2015-11-24 10:33:24 UTC) #21
Message was sent while issue was closed.
There you go https://codereview.adblockplus.org/29330730/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5