Issue 2750 - [Support other languages] Apply Firefox's translations

Issue 2750 - [Support other languages] Apply Firefox's translations

[René Jeschke, 2015-07-17 19:08:27]

Re-issued the issue ;-)

[Felix Dahlke, 2015-07-26 06:56:52]

Looks good, just some comments around how things are described in the README.

I still think that describing how to manually create this setup is suboptimal,
how I picture it we could:

1. Create a new adblockbrowser-build repo.
2. Add both adblockbrowser and mozharness as dependencies, pointing to the
desired revision/tag.
3. Keep the modified build configuration file there and update it whenever the
mozharness dependency is updated. People could then modify and/or copy that one
if they need to have changes for their local environments (or we could read
environment variables or whatever, being a Python script). The defaults there
should be suitable for use by a build server.

But for the sake of progress and simplicity, I suggest we first go with this
approach of describing the process.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md
File README.md (right):

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode35
README.md:35: hg clone http://hg.mozilla.org/build/mozharness
What if mozharness and our repository gets out of sync? I see a few options:

1. We refer to the relevant release tag here and update that whenever we merge
from upstream
2. We keep a copy (i.e. no modifications, mainly to not depend more than
necessary on hg.mozilla.org) of mozharness on bitbucket and use that, updating
it whenever we do an upstream merge

I'd actually vote for doing both - that way people can also still follow these
instructions to build old versions.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode45
README.md:45: 5. Create a copy of the build configuration, by:
Language nit: "by" doesn't make sense here IMO, we could either leave it out or
say "by executing".

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode51
README.md:51: * Change `BUILD_DIR = "mozilla-central"` to `BUILD_DIR =
"adblockbrowser"`
Not really obvious to me that abb-multi-cfg.py needs to be modified here - how
about a quick introductory sentence above that says this?

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode59
README.md:59: 26     "repos": [{
Since we're not pulling a specific revision of mozharness, and even if we would
we'd probably be unlikely to update the line numbers, IMHO we should rather not
refer to any line numbers.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode65
README.md:65: not want your objdir to be backupped. Keep in mind that you then
need to
Language nit: s/backupped/backed up/

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode70
README.md:70: 1 import os
I'd also vote for removing line numbers here - they don't add anything,
particularly since they're starting at 1 :D

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode75
README.md:75: 6 REPO_PATH = "mozilla-central"
Shouldn't we actually use mozilla-release here? adblockbrowser is a clone of
mozilla-release, not mozilla-central.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode127
README.md:127: 
Nit: There's already a new line at EOF, so why add a second one?

[René Jeschke, 2015-07-27 10:05:43]

https://codereview.adblockplus.org/29322413/diff/29322414/README.md
File README.md (right):

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode45
README.md:45: 5. Create a copy of the build configuration, by:
On 2015/07/26 06:56:51, Felix Dahlke wrote:
> Language nit: "by" doesn't make sense here IMO, we could either leave it out
or
> say "by executing".

Done.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode51
README.md:51: * Change `BUILD_DIR = "mozilla-central"` to `BUILD_DIR =
"adblockbrowser"`
On 2015/07/26 06:56:52, Felix Dahlke wrote:
> Not really obvious to me that abb-multi-cfg.py needs to be modified here - how
> about a quick introductory sentence above that says this?

Done.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode59
README.md:59: 26     "repos": [{
On 2015/07/26 06:56:52, Felix Dahlke wrote:
> Since we're not pulling a specific revision of mozharness, and even if we
would
> we'd probably be unlikely to update the line numbers, IMHO we should rather
not
> refer to any line numbers.

Na, look at line 57, there it reads "The code to remove looks like this:" ...
this is a code portion that needs to be _removed_ and as you can see, it's not
visible in the example configuration file below ;-)

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode65
README.md:65: not want your objdir to be backupped. Keep in mind that you then
need to
On 2015/07/26 06:56:52, Felix Dahlke wrote:
> Language nit: s/backupped/backed up/

Done.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode70
README.md:70: 1 import os
On 2015/07/26 06:56:52, Felix Dahlke wrote:
> I'd also vote for removing line numbers here - they don't add anything,
> particularly since they're starting at 1 :D

Done.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode75
README.md:75: 6 REPO_PATH = "mozilla-central"
On 2015/07/26 06:56:52, Felix Dahlke wrote:
> Shouldn't we actually use mozilla-release here? adblockbrowser is a clone of
> mozilla-release, not mozilla-central.

This line is irrelevant, as we removed this step from "default_actions". Why
this line is still in there? Because it is also irrelevant to remove it, and my
description includes copying the base python file and modifying this, and after
that, the file looks like this here.

[Felix Dahlke, 2015-07-29 20:10:21]

LGTM, assuming this is not adding a second newline at EOF in README.md.

https://codereview.adblockplus.org/29322413/diff/29322414/README.md
File README.md (right):

https://codereview.adblockplus.org/29322413/diff/29322414/README.md#newcode75
README.md:75: 6 REPO_PATH = "mozilla-central"
On 2015/07/27 10:05:42, René Jeschke wrote:
> On 2015/07/26 06:56:52, Felix Dahlke wrote:
> > Shouldn't we actually use mozilla-release here? adblockbrowser is a clone of
> > mozilla-release, not mozilla-central.
> 
> This line is irrelevant, as we removed this step from "default_actions". Why
> this line is still in there? Because it is also irrelevant to remove it, and
my
> description includes copying the base python file and modifying this, and
after
> that, the file looks like this here.

OK I see. Non-obvious, but I guess it's fine for now.