Issue 5385277551935488: Issue 2516 - Add a release build script (Closed)
|
Created:
May 17, 2015, 2:56 p.m. by Felix Dahlke Modified:
May 21, 2015, 11:59 a.m. Reviewers:
René Jeschke CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 2516 - Add a release build script
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Check for --disable-crashreporter, print usage to stderr #
Total comments: 4
Patch Set 3 : Use check_call for ensure_dependencies.py #Patch Set 4 : Check for MOZILLA_OFFICIAL=1 #MessagesTotal messages: 11
http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... File build_release.py (right): http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... build_release.py:20: def check_mozconfig(path, mode): Should we add a check for '--disable-crashreporter' maybe too?
On 2015/05/18 11:30:16, René Jeschke wrote: > http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... > File build_release.py (right): > > http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... > build_release.py:20: def check_mozconfig(path, mode): > Should we add a check for '--disable-crashreporter' maybe too? Hm, maybe. But frankly I'd rather get https://issues.adblockplus.org/ticket/2490 fixed ASAP :P
On 2015/05/18 12:33:35, Felix H. Dahlke wrote: > On 2015/05/18 11:30:16, René Jeschke wrote: > > > http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... > > File build_release.py (right): > > > > > http://codereview.adblockplus.org/5385277551935488/diff/5178081291534336/buil... > > build_release.py:20: def check_mozconfig(path, mode): > > Should we add a check for '--disable-crashreporter' maybe too? > > Hm, maybe. But frankly I'd rather get https://issues.adblockplus.org/ticket/2490 > fixed ASAP :P :-D ... but, maybe we still can have the check in here (as a temporary solution) and add it's removal to https://issues.adblockplus.org/ticket/2491 in the meantime :-D
New patch set is up. On 2015/05/18 12:35:21, René Jeschke wrote: > :-D ... but, maybe we still can have the check in here (as a temporary solution) > and add it's removal to https://issues.adblockplus.org/ticket/2491 in the > meantime :-D Alright, but I DO believe we should fix this in the near future, messing with .mozconfig more than absolutely necessary is not great.
http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... File build_release.py (right): http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... build_release.py:40: def find_mozconfig(mode): Nit: 'find_mozconfig' does not really 'search' for mozconfig. Instead it just returns a path after some validation, so the name might be misleading. (I must admit that I would have named it likewise :-) ) (So, change it if you like, otherwise just leave it) http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... build_release.py:89: subprocess.call([ENSURE_DEPENDENCIES_PATH]) Shouldn't we use 'check_call' here, too? Or is a 'ensure_dependency' failure tolerated during a release build?
http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... File build_release.py (right): http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... build_release.py:40: def find_mozconfig(mode): On 2015/05/19 09:57:54, René Jeschke wrote: > Nit: 'find_mozconfig' does not really 'search' for mozconfig. Instead it just > returns a path after some validation, so the name might be misleading. > > (I must admit that I would have named it likewise :-) ) > (So, change it if you like, otherwise just leave it) Well, it looks for an appropriate mozconfig and complains if there is none. Not brilliant I guess, but I couldn't really come up with something better. http://codereview.adblockplus.org/5385277551935488/diff/5113880120393728/buil... build_release.py:89: subprocess.call([ENSURE_DEPENDENCIES_PATH]) On 2015/05/19 09:57:54, René Jeschke wrote: > Shouldn't we use 'check_call' here, too? Or is a 'ensure_dependency' failure > tolerated during a release build? Yes you're right. I did this by design, but for a release build we really need to avoid this kind of issue.
LGTM
Realised that we also need to export MOZILLA_OFFICIAL=1 - the only notable effect this has is to set debuggable=false in the manifest.
LGTM
Moving Wladimir to CC - we already used this for the release, so I don't feel great about keeping it locally. I'll still gladly address feedback however. 
|
