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

Issue 29345508: Issue 4098 - Get rid of special build setup for development builds (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 5 months ago by Wladimir Palant
Modified:
3 years, 5 months ago
Reviewers:
Vasily Kuznetsov
Visibility:
Public.

Description

The patch is complete now. It replaces direct calls to buildtools with subprocess calls where possible and adds setting SPIDERMONKEY_BINARY environment variable. Repository: hg.adblockplus.org/sitescripts

Patch Set 1 #

Total comments: 18

Patch Set 2 : Addressed comments #

Total comments: 3

Patch Set 3 : Added spiderMonkeyPath config setting #

Patch Set 4 : Improved ensure_dependencies call #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -49 lines) Patch
M .sitescripts.example View 1 2 2 chunks +9 lines, -14 lines 0 comments Download
M sitescripts/extensions/bin/createNightlies.py View 1 2 3 6 chunks +45 lines, -34 lines 0 comments Download
M sitescripts/extensions/utils.py View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 7
Wladimir Palant
3 years, 5 months ago (2016-06-01 14:48:03 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29345508/diff/29345509/.sitescripts.example File .sitescripts.example (left): https://codereview.adblockplus.org/29345508/diff/29345509/.sitescripts.example#oldcode96 .sitescripts.example:96: urlfixer_galleryID=url-fixer I removed deobfuscator and urlfixer because listing them ...
3 years, 5 months ago (2016-06-01 14:57:12 UTC) #2
Vasily Kuznetsov
In general, looks good. I have a few minor questions/suggestions below. I haven't tested it, ...
3 years, 5 months ago (2016-06-01 17:14:15 UTC) #3
Wladimir Palant
This patch has been finalized and tested now. All blockers landed so we are good ...
3 years, 5 months ago (2016-06-03 14:14:07 UTC) #4
Vasily Kuznetsov
Looks good. Just one small thing (see below). https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensions/bin/createNightlies.py#newcode85 sitescripts/extensions/bin/createNightlies.py:85: 'hg', ...
3 years, 5 months ago (2016-06-03 16:04:49 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensions/bin/createNightlies.py#newcode129 sitescripts/extensions/bin/createNightlies.py:129: if os.path.isfile(command[0]): On 2016/06/03 16:04:49, Vasily Kuznetsov wrote: > ...
3 years, 5 months ago (2016-06-06 11:47:47 UTC) #6
Vasily Kuznetsov
3 years, 5 months ago (2016-06-06 12:48:16 UTC) #7
LGTM

https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi...
File sitescripts/extensions/bin/createNightlies.py (right):

https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi...
sitescripts/extensions/bin/createNightlies.py:129: if
os.path.isfile(command[0]):
On 2016/06/06 11:47:46, Wladimir Palant wrote:
> On 2016/06/03 16:04:49, Vasily Kuznetsov wrote:
> > Should not this become `command[1]` now that we've inserted python as the
> first
> > item of the list?
> 
> Ouch, you are right. In fact, I had a better idea in order to make this code
> less fragile.

Lovely!
Sign in to reply to this message.

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