Issue 2483 - Give Adblock Browser its own app ID

Issue 2483 - Give Adblock Browser its own app ID

Adblock Browser hasn't been announced yet, so we can't land the patches for
issue 2509 and 2510 yet. For now, we have to apply them during the build
process.

[Felix Dahlke, 2015-05-14 15:24:51]

Put René in CC since he'll be on holiday since Monday.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/mobi...
File mobile/android/build.mk (right):

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/mobi...
mobile/android/build.mk:11: $(topsrcdir)/adblockplus/build.py
$(DIST)/bin/extensions/\{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d\}.xpi
Seemed like a good opportunity to make our change here less intrusive and move
most of the logic to build.py.

[Wladimir Palant, 2015-05-15 19:16:16]

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
File adblockplus/build.py (right):

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:14: os.mkdir(xpi_dir)
os.makedirs() please - you don't know whether its parent directory exists.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:21: buildtools.packager.getMetadataPath = get_metadata_path
Funny how you replace getMetadataPath yet use a patch instead of assigning to
buildtools.packagerGecko.KNOWN_APPS["adblockbrowser"]. Should be simple enough
and won't require patching dependencies file as an added bonus.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:31: "adblockplus-patched-%s" % os.getpid())
Please use tempfile.mkdtemp(), even though Windows compatibility certainly isn't
your top concern.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:37: patched_base_dir, BUILD_DIR))
1) Call hg import maybe?
2) Please use subprocess.call() with a list as parameter (meaning shell=False).

[Felix Dahlke, 2015-05-15 22:09:08]

New patch set is up.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
File adblockplus/build.py (right):

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:14: os.mkdir(xpi_dir)
On 2015/05/15 19:16:16, Wladimir Palant wrote:
> os.makedirs() please - you don't know whether its parent directory exists.

Done.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:21: buildtools.packager.getMetadataPath = get_metadata_path
On 2015/05/15 19:16:16, Wladimir Palant wrote:
> Funny how you replace getMetadataPath yet use a patch instead of assigning to
> buildtools.packagerGecko.KNOWN_APPS["adblockbrowser"]. Should be simple enough
> and won't require patching dependencies file as an added bonus.

Sticking to applying the issue 2509 patch file as discussed.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:31: "adblockplus-patched-%s" % os.getpid())
On 2015/05/15 19:16:16, Wladimir Palant wrote:
> Please use tempfile.mkdtemp(), even though Windows compatibility certainly
isn't
> your top concern.

Forgot about tempfile, much nicer indeed. Ended up using
tempfile.NamedTemporaryFile().name since copytree doesn't want the target
directory to exist.

http://codereview.adblockplus.org/6238034718621696/diff/5629499534213120/adbl...
adblockplus/build.py:37: patched_base_dir, BUILD_DIR))
On 2015/05/15 19:16:16, Wladimir Palant wrote:
> 1) Call hg import maybe?
> 2) Please use subprocess.call() with a list as parameter (meaning
shell=False).

Done.

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
File adblockplus/build.py (right):

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
adblockplus/build.py:10: BASE_DIR = os.path.dirname(os.path.abspath(__file__))
Realised the naming here was inconsistent with our other scripts, changed that.

[Wladimir Palant, 2015-05-18 11:01:22]

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
File adblockplus/build.py (right):

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
adblockplus/build.py:28: sys.exit(1)
Exiting without an error message is strange, why did you remove it? Besides,
that message belongs on stderr.

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
adblockplus/build.py:37: cwd=os.path.join(patched_abp_dir, "buildtools"))
I'd prefer -R parameter instead of cwd but doesn't really matter.

Please use subprocess.check_call() - we shouldn't continue if the patch failed
to apply.

[Felix Dahlke, 2015-05-18 19:43:28]

That should be it I hope :)

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
File adblockplus/build.py (right):

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
adblockplus/build.py:28: sys.exit(1)
On 2015/05/18 11:01:22, Wladimir Palant wrote:
> Exiting without an error message is strange, why did you remove it? Besides,
> that message belongs on stderr.

Oops, forgot that one.

http://codereview.adblockplus.org/6238034718621696/diff/5685265389584384/adbl...
adblockplus/build.py:37: cwd=os.path.join(patched_abp_dir, "buildtools"))
On 2015/05/18 11:01:22, Wladimir Palant wrote:
> I'd prefer -R parameter instead of cwd but doesn't really matter.
> 
> Please use subprocess.check_call() - we shouldn't continue if the patch failed
> to apply.

Yeah that's nicer, done.

[Wladimir Palant, 2015-05-18 19:48:31]

LGTM