Issue 5675132278276096: Noissue - Prepare translations (Closed)
|
Created:
June 10, 2015, 6:24 p.m. by René Jeschke Modified:
July 15, 2015, 1:50 p.m. Reviewers:
Felix Dahlke Visibility:
Public. |
DescriptionNoissue - Prepare translations
Patch Set 1 #Patch Set 2 : Added build instructions. #Patch Set 3 : Strategic line breaks #
Total comments: 32
Patch Set 4 : README nits #
MessagesTotal messages: 7
For our own l10n files: Currently they are organized as: mobile/android/locales/abb/AB_CD/android_strings.dtd But I'm not sure about the naming here, 'abb' might get confused with a three-letter language code :D Also, we do not necessarily need to have every l10n in a separate folder, i.e. we could name the files: adblockbrowser_l10-en-US.dtd for example. So, we need a place for our files and a structure. If you don't come up with a better idea, then I will just rename 'abb' to 'adblockbrowser', and leave the rest as is.
This definitely deserves an issue. Aside from my comments: Which resources does mozharness fetch from Mozilla servers? I think we should file a follow-up issue for dealing with that and ensuring our build process works without Mozilla servers, or if we happen to fall behind upstream (as we currently are :D). https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... File README.md (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:31: ###Preparations Nit: Space after ### as on http://daringfireball.net/projects/markdown/syntax ? https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:33: 1. Create a folder outside of the Adblock Browser repo, I call Nit: s/I call it/e.g./ https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:37: hg clone http://hg.mozilla.org/build/mozharness I presume this is always tied to the most recent Firefox version? Can we offer a clone of that to ensure it's the right version? Maybe something for a follow-up issue. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:43: 4. Copy your `mozconfig` from adblockbrowser to `abb-multi`, e.g.: Nit: adblockbrowser also in backticks? https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:45: cp adblockbrowser/mozconfig ./mozconfig Even though we call it `mozconfig`, the file is actually called `.mozconfig`. Is `mozconfig` even being loaded? https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:49: cp mozharness/configs/multi_locale/standalone_mozilla-central.py abb-multi-cfg.py mozilla-central? Isn't there something for mozilla-release, which we are on? (If we keep up with upstream that is, if we fall behind we're on something that probably doesn't exist in mozharness.) https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:54: `L3` Was wondering for a while what "L3" for a while, but once I got that it's the line number: I don't think it's a good idea to hard code line numbers here, those seem incredibly likely to change. "Look for X and change it to Y" is generally better for instructions IMHO. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:58: * **Remove the repository pulling part** from the config, i.e. `L26` to `L30`. After reading this far, I Think what we should actually do is: 1. Fork mozconfig 2. Make all these changes in our fork (in a branch/bookmark named "adblockbrowser") 3. Adjust the repository pulling part to fetch the "adblockbrowser" branch. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/Makefile.in (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/Makefile.in:18: ABBSTRINGSPATH = $(or $(wildcard $(srcdir)/abb/$(ABr_CD)/android_strings.dtd), \ Does ABr_CD actually exist? Haven't seen it being set or referenced anywhere. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/abb/de/android_strings.dtd (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/abb/de/android_strings.dtd:1: <!-- Adblock Browser --> Why is this under locales/abb/de? Is it supposed to be translated? We might as well do that now to ensure it works, or leave it out. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/locales/maemo-locales (left): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/locales/maemo-locales:1: an What are these changes for?
https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... File README.md (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:31: ###Preparations On 2015/06/22 08:10:51, Felix Dahlke wrote: > Nit: Space after ### as on http://daringfireball.net/projects/markdown/syntax ? Done. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:33: 1. Create a folder outside of the Adblock Browser repo, I call On 2015/06/22 08:10:51, Felix Dahlke wrote: > Nit: s/I call it/e.g./ Done. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:37: hg clone http://hg.mozilla.org/build/mozharness On 2015/06/22 08:10:51, Felix Dahlke wrote: > I presume this is always tied to the most recent Firefox version? Can we offer a > clone of that to ensure it's the right version? Maybe something for a follow-up > issue. We can clone everything, so, yes, we can file a follow up issue for this. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:43: 4. Copy your `mozconfig` from adblockbrowser to `abb-multi`, e.g.: On 2015/06/22 08:10:51, Felix Dahlke wrote: > Nit: adblockbrowser also in backticks? Done. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:45: cp adblockbrowser/mozconfig ./mozconfig On 2015/06/22 08:10:51, Felix Dahlke wrote: > Even though we call it `mozconfig`, the file is actually called `.mozconfig`. Is > `mozconfig` even being loaded? We do not call this 'mozconfig', Mozilla calls this 'mozconfig'. I told you about this twice already, but here we go again: Again: in this README we say: "follow Mozilla's build instructions for Fennec" ... and in their step-by-step guide, this file is called 'mozconfig' (it's called so by Mozilla), without a dot. Yes, it works. Here's the quote from Mozilla: "Next, navigate to your source directory and create a text file called "mozconfig". The mozconfig file is what tells the Mozilla build system what to build and how your build environment is prepared. Here is the minimal mozconfig file for building Fennec; it should be named mozconfig in your source directory, as the build scripts will read from the mozconfig file in your source directory by default" (quoted from https://wiki.mozilla.org/Mobile/Fennec/Android#Preparing_your_build_environment ) The file is called 'mozconfig' everywhere in their build guide, so we should stick to 'mozconfig'. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:49: cp mozharness/configs/multi_locale/standalone_mozilla-central.py abb-multi-cfg.py On 2015/06/22 08:10:52, Felix Dahlke wrote: > mozilla-central? Isn't there something for mozilla-release, which we are on? (If > we keep up with upstream that is, if we fall behind we're on something that > probably doesn't exist in mozharness.) There is only one .py config gile in this folder, but we can change the L10N repo base path to one of the release L10N bases on hg.mozilla.org. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:54: `L3` On 2015/06/22 08:10:51, Felix Dahlke wrote: > Was wondering for a while what "L3" for a while, but once I got that it's the > line number: I don't think it's a good idea to hard code line numbers here, > those seem incredibly likely to change. "Look for X and change it to Y" is > generally better for instructions IMHO. Done. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:58: * **Remove the repository pulling part** from the config, i.e. `L26` to `L30`. On 2015/06/22 08:10:52, Felix Dahlke wrote: > After reading this far, I Think what we should actually do is: > > 1. Fork mozconfig > 2. Make all these changes in our fork (in a branch/bookmark named > "adblockbrowser") > 3. Adjust the repository pulling part to fetch the "adblockbrowser" branch. Why do you want to fork a local build configuration file (i.e. mozconfig)? Also, we should refrain from hard-coding build configurations inside a repository anyway. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/Makefile.in (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/Makefile.in:18: ABBSTRINGSPATH = $(or $(wildcard $(srcdir)/abb/$(ABr_CD)/android_strings.dtd), \ On 2015/06/22 08:10:52, Felix Dahlke wrote: > Does ABr_CD actually exist? Haven't seen it being set or referenced anywhere. Nope, I haven't found such folder either, but I think we should be aware of those, because they are used in the original Makefile (see line #9) https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/abb/de/android_strings.dtd (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/abb/de/android_strings.dtd:1: <!-- Adblock Browser --> On 2015/06/22 08:10:51, Felix Dahlke wrote: > Why is this under locales/abb/de? Is it supposed to be translated? We might as > well do that now to ensure it works, or leave it out. I also told you this on IRC, this is a test file that gets removed, I also sent you a screen-shot on IRC to prove that it works :D. So, yes, this file gets removed. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/locales/maemo-locales (left): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/locales/maemo-locales:1: an On 2015/06/22 08:10:51, Felix Dahlke wrote: > What are these changes for? Ah, told you on IRC, those translations don't exist as a repository any longer, at least in the development repo.
https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... File README.md (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:45: cp adblockbrowser/mozconfig ./mozconfig On 2015/06/22 10:24:48, René Jeschke wrote: > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > Even though we call it `mozconfig`, the file is actually called `.mozconfig`. > Is > > `mozconfig` even being loaded? > > We do not call this 'mozconfig', Mozilla calls this 'mozconfig'. I told you > about this twice already, but here we go again: > > Again: in this README we say: "follow Mozilla's build instructions for Fennec" > ... and in their step-by-step guide, this file is called 'mozconfig' (it's > called so by Mozilla), without a dot. Yes, it works. > > Here's the quote from Mozilla: > > "Next, navigate to your source directory and create a text file called > "mozconfig". The mozconfig file is what tells the Mozilla build system what to > build and how your build environment is prepared. Here is the minimal mozconfig > file for building Fennec; it should be named mozconfig in your source directory, > as the build scripts will read from the mozconfig file in your source directory > by default" > > (quoted from > https://wiki.mozilla.org/Mobile/Fennec/Android#Preparing_your_build_environment > ) > > The file is called 'mozconfig' everywhere in their build guide, so we should > stick to 'mozconfig'. They're actually a bit inconsistent here, but I thought they generally refer to `.mozconfig` when talking about the file on disk, apparently it's pretty messed up. My point, however, is that `.mozconfig` is a file that's being automatically loaded whereas `mozconfig` or any other variation of the name needs to be explicitly picked. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:58: * **Remove the repository pulling part** from the config, i.e. `L26` to `L30`. On 2015/06/22 10:24:48, René Jeschke wrote: > On 2015/06/22 08:10:52, Felix Dahlke wrote: > > After reading this far, I Think what we should actually do is: > > > > 1. Fork mozconfig > > 2. Make all these changes in our fork (in a branch/bookmark named > > "adblockbrowser") > > 3. Adjust the repository pulling part to fetch the "adblockbrowser" branch. > > Why do you want to fork a local build configuration file (i.e. mozconfig)? Also, > we should refrain from hard-coding build configurations inside a repository > anyway. Sorry, I meant mozharness. We're setting up a new script there, we should do that in our own fork of mozharness IMO. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/Makefile.in (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/Makefile.in:18: ABBSTRINGSPATH = $(or $(wildcard $(srcdir)/abb/$(ABr_CD)/android_strings.dtd), \ On 2015/06/22 10:24:48, René Jeschke wrote: > On 2015/06/22 08:10:52, Felix Dahlke wrote: > > Does ABr_CD actually exist? Haven't seen it being set or referenced anywhere. > > Nope, I haven't found such folder either, but I think we should be aware of > those, because they are used in the original Makefile (see line #9) Acknowledged. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/base/locales/abb/de/android_strings.dtd (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/base/locales/abb/de/android_strings.dtd:1: <!-- Adblock Browser --> On 2015/06/22 10:24:47, René Jeschke wrote: > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > Why is this under locales/abb/de? Is it supposed to be translated? We might as > > well do that now to ensure it works, or leave it out. > > I also told you this on IRC, this is a test file that gets removed, I also sent > you a screen-shot on IRC to prove that it works :D. > > So, yes, this file gets removed. Acknowledged. https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/locales/maemo-locales (left): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/locales/maemo-locales:1: an On 2015/06/22 10:24:47, René Jeschke wrote: > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > What are these changes for? > > Ah, told you on IRC, those translations don't exist as a repository any longer, > at least in the development repo. That's weird, have they been removed upstream? Seems to me like we're out of sync with mozharness here, or is it something else?
https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... File README.md (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:45: cp adblockbrowser/mozconfig ./mozconfig On 2015/06/22 12:01:01, Felix Dahlke wrote: > On 2015/06/22 10:24:48, René Jeschke wrote: > > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > > Even though we call it `mozconfig`, the file is actually called > `.mozconfig`. > > Is > > > `mozconfig` even being loaded? > > > > We do not call this 'mozconfig', Mozilla calls this 'mozconfig'. I told you > > about this twice already, but here we go again: > > > > Again: in this README we say: "follow Mozilla's build instructions for Fennec" > > ... and in their step-by-step guide, this file is called 'mozconfig' (it's > > called so by Mozilla), without a dot. Yes, it works. > > > > Here's the quote from Mozilla: > > > > "Next, navigate to your source directory and create a text file called > > "mozconfig". The mozconfig file is what tells the Mozilla build system what to > > build and how your build environment is prepared. Here is the minimal > mozconfig > > file for building Fennec; it should be named mozconfig in your source > directory, > > as the build scripts will read from the mozconfig file in your source > directory > > by default" > > > > (quoted from > > > https://wiki.mozilla.org/Mobile/Fennec/Android#Preparing_your_build_environment > > ) > > > > The file is called 'mozconfig' everywhere in their build guide, so we should > > stick to 'mozconfig'. > > They're actually a bit inconsistent here, but I thought they generally refer to > `.mozconfig` when talking about the file on disk, apparently it's pretty messed > up. > > My point, however, is that `.mozconfig` is a file that's being automatically > loaded whereas `mozconfig` or any other variation of the name needs to be > explicitly picked. Still, I am using 'mozconfig' (from the start), it gets automatically loaded and it's even referenced like this in the mozharness repository. Yes, maybe they call it differently for the desktop builds, but at least for Fennec it is called "mozconfig" without the dot, so I think we should stick with this naming, as this reduces confusion. Or do you have really strong objections against this? https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:58: * **Remove the repository pulling part** from the config, i.e. `L26` to `L30`. On 2015/06/22 12:01:01, Felix Dahlke wrote: > On 2015/06/22 10:24:48, René Jeschke wrote: > > On 2015/06/22 08:10:52, Felix Dahlke wrote: > > > After reading this far, I Think what we should actually do is: > > > > > > 1. Fork mozconfig > > > 2. Make all these changes in our fork (in a branch/bookmark named > > > "adblockbrowser") > > > 3. Adjust the repository pulling part to fetch the "adblockbrowser" branch. > > > > Why do you want to fork a local build configuration file (i.e. mozconfig)? > Also, > > we should refrain from hard-coding build configurations inside a repository > > anyway. > > Sorry, I meant mozharness. We're setting up a new script there, we should do > that in our own fork of mozharness IMO. Yes, maybe we should do this, needs another follow up issue :-) https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... File mobile/android/locales/maemo-locales (left): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/mob... mobile/android/locales/maemo-locales:1: an On 2015/06/22 12:01:01, Felix Dahlke wrote: > On 2015/06/22 10:24:47, René Jeschke wrote: > > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > > What are these changes for? > > > > Ah, told you on IRC, those translations don't exist as a repository any > longer, > > at least in the development repo. > > That's weird, have they been removed upstream? Seems to me like we're out of > sync with mozharness here, or is it something else? They are still in the release/L10N folders, they somehow have only be removed in the development repositories (central). I will add more changes to the README to explain the change from central to release L10N, then I can revert these maemo changes.
https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... File README.md (right): https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:45: cp adblockbrowser/mozconfig ./mozconfig On 2015/06/22 12:09:15, René Jeschke wrote: > On 2015/06/22 12:01:01, Felix Dahlke wrote: > > On 2015/06/22 10:24:48, René Jeschke wrote: > > > On 2015/06/22 08:10:51, Felix Dahlke wrote: > > > > Even though we call it `mozconfig`, the file is actually called > > `.mozconfig`. > > > Is > > > > `mozconfig` even being loaded? > > > > > > We do not call this 'mozconfig', Mozilla calls this 'mozconfig'. I told you > > > about this twice already, but here we go again: > > > > > > Again: in this README we say: "follow Mozilla's build instructions for > Fennec" > > > ... and in their step-by-step guide, this file is called 'mozconfig' (it's > > > called so by Mozilla), without a dot. Yes, it works. > > > > > > Here's the quote from Mozilla: > > > > > > "Next, navigate to your source directory and create a text file called > > > "mozconfig". The mozconfig file is what tells the Mozilla build system what > to > > > build and how your build environment is prepared. Here is the minimal > > mozconfig > > > file for building Fennec; it should be named mozconfig in your source > > directory, > > > as the build scripts will read from the mozconfig file in your source > > directory > > > by default" > > > > > > (quoted from > > > > > > https://wiki.mozilla.org/Mobile/Fennec/Android#Preparing_your_build_environment > > > ) > > > > > > The file is called 'mozconfig' everywhere in their build guide, so we should > > > stick to 'mozconfig'. > > > > They're actually a bit inconsistent here, but I thought they generally refer > to > > `.mozconfig` when talking about the file on disk, apparently it's pretty > messed > > up. > > > > My point, however, is that `.mozconfig` is a file that's being automatically > > loaded whereas `mozconfig` or any other variation of the name needs to be > > explicitly picked. > > Still, I am using 'mozconfig' (from the start), it gets automatically loaded and > it's even referenced like this in the mozharness repository. Yes, maybe they > call it differently for the desktop builds, but at least for Fennec it is called > "mozconfig" without the dot, so I think we should stick with this naming, as > this reduces confusion. Or do you have really strong objections against this? `mozconfig` gets automatically used by the build system for you? That's exactly what I mean - for me it doesn't. I have to set the MOZCONFIG variable to use any other file name. No, I don't have strong objections, just wanted to point out that it doesn't make sense to me :D https://codereview.adblockplus.org/5675132278276096/diff/5685265389584384/REA... README.md:58: * **Remove the repository pulling part** from the config, i.e. `L26` to `L30`. On 2015/06/22 12:09:14, René Jeschke wrote: > On 2015/06/22 12:01:01, Felix Dahlke wrote: > > On 2015/06/22 10:24:48, René Jeschke wrote: > > > On 2015/06/22 08:10:52, Felix Dahlke wrote: > > > > After reading this far, I Think what we should actually do is: > > > > > > > > 1. Fork mozconfig > > > > 2. Make all these changes in our fork (in a branch/bookmark named > > > > "adblockbrowser") > > > > 3. Adjust the repository pulling part to fetch the "adblockbrowser" > branch. > > > > > > Why do you want to fork a local build configuration file (i.e. mozconfig)? > > Also, > > > we should refrain from hard-coding build configurations inside a repository > > > anyway. > > > > Sorry, I meant mozharness. We're setting up a new script there, we should do > > that in our own fork of mozharness IMO. > > Yes, maybe we should do this, needs another follow up issue :-) Seems like something we should figure out now to me - apparently the mozharness you're using wants to work against mozilla-central. How does Mozilla use it in release builds?
Created an issue for this: https://issues.adblockplus.org/ticket/2750 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
