Issue 6077 - Create script to modify the list of search engines dynamically

mozharness/abb/transform_locales.py

 # This file is part of Adblock Plus
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import json
 import os
 import re
+import shutil
 import sys
 
 _LOCALE_RE = re.compile("^([a-z]{2,3}(?:-[A-Z]{2})?)$")
 _VALUES_LOCALE_RE = re.compile("^values-([a-z]{2,3}(?:-r[A-Z]{2})?)$")
 
 _SEARCH_PROPS_RE = re.compile("^browser\.search\."
                               "(defaultenginename|order\.).*$")
 _SHORTNAME_RE = re.compile("^<ShortName>(.*)</ShortName>$")
 
 _PROPERTY_FORMAT_RE = re.compile("^(([^=]*)=)(.*)$")
 _ENTITY_FORMAT_RE = re.compile("^(\s*<!ENTITY\s*([^\"\s]*)\s*\")(.*)(\">)$")
 _STRING_FORMAT_RE = re.compile(
     "^(\s*<string name=\"([^\"]*)\">)(.*)(</string>)$")
 
 _MOZBUILD_PATH = os.path.join("python", "mozbuild")
 
 _CHROME_PATH = os.path.join("dist", "bin", "chrome")
 _RES_PATH = os.path.join("mobile", "android", "base", "res")
-_I10N_PATH = os.path.join("abb-build", "l10n")

[diegocarloslima, 2017/12/13 10:44:42]

This var should be named L10N_PATH :)

-_LISTJSON_PATH = os.path.join("mobile", "locales", "search")
-_SEARCHPLUGINS_PATH = os.path.join("mobile", "locales", "searchplugins")

[diegocarloslima, 2017/12/13 10:44:41]

Maybe the ("mobile","locales") part could be extracted to another variable to
avoid writing twice the same string constants, e.g. _LOCALES_PATH =
os.path.join("mobile", "locales").

Also, its not really clear to me why do we need the _LISTJSON_PATH + _LIST_JSON.
Couldn't we have just one var (e.g. _LIST_JSON_PATH) that would also include the
`list.json` filename?

+_L10N_PATH = os.path.join("abb-build", "l10n")
+_LOCALES_PATH = os.path.join("mobile", "locales")
+_LISTJSON_PATH = os.path.join(_LOCALES_PATH, "search", "list.json")
+_GENERAL_SEARCHPLUGINS_PATH = os.path.join(_LOCALES_PATH, "searchplugins")
 
 _BROWSER_DIR = "browser"
 _REGION_PROPS_PATH = os.path.join(_BROWSER_DIR, "region.properties")
+_LOCALE_SEARCHPLUGINS_PATH = os.path.join(_BROWSER_DIR, "searchplugins")
 
 _APPSTRINGS_PROPS_PATH = os.path.join(_BROWSER_DIR, "appstrings.properties")
 _STRINGS_XML_PATH = "strings.xml"
-_LIST_JSON = "list.json"
 
 _DEFAULT_LOCALE = "en-US"
-_KEY_VDF = "visibleDefaultEngines"
-_KEY_LOC = "locales"
-_KEY_DEF = "default"

[diegocarloslima, 2017/12/13 10:44:42]

I know that having short variable names helps to be in compliance with pep8, but
if I look into those keys in the code below, they are not clear to me, so I have
to come here to see what they stand for, which is kinda of undesired

+_DEF_ENGINES = "visibleDefaultEngines"
 
 # Add Ecosia as secondary search engine.
 # See https://issues.adblockplus.org/ticket/5518
 _ECOSIA_ID = "ecosia"
+_ECOSIA_PATH = os.path.join(_GENERAL_SEARCHPLUGINS_PATH, "ecosia.xml")
 
 _SEARCH_ENGINE_ORDER_DEFAULT = [
     "duckduckgo",
     "yahoo",
     "google",
     "wikipedia",
     "amazondotcom"]
 
 _SEARCH_ENGINE_ORDER_ECOSIA = [
     "duckduckgo",
     "yahoo",
     "google",
-    "ecosia",
+    _ECOSIA_ID,
     "wikipedia",
     "amazon"]
 
 _SEARCH_ENGINE_ORDER = {
     "de": _SEARCH_ENGINE_ORDER_ECOSIA,
     "en-GB": _SEARCH_ENGINE_ORDER_ECOSIA,
     "en-US": _SEARCH_ENGINE_ORDER_ECOSIA,
     "fr": _SEARCH_ENGINE_ORDER_ECOSIA,
     "nl": _SEARCH_ENGINE_ORDER_ECOSIA,
     "zh-CN": ["baidu",
@@ skipping 70 matching lines @@
     return None
 
 
 def _write_lines(filename, lines):
     """Writes lines into file appending \\n"""
     with open(filename, "w") as fd:
         for l in lines:
             fd.write("%s\n" % l)
 
 
-def _transform_locale(locale, build_dir, locale_path, logger):
+def _transform_locale(data, locale, project_dir, locale_path, logger):
     logger.info("Processing locale '%s'..." % locale)
-    search_list_path = os.path.join(build_dir, _LISTJSON_PATH)
-    _check_path_exists(search_list_path, logger)

[diegocarloslima, 2017/12/13 10:44:41]

I think it makes more sense to check for the existence of the `list.json` file
(_LISTJSON_PATH + _LIST_JSON) instead of the `search` folder. Also, since this
check isn't locale dependent, it should be only made once (before actually
calling _transform_locale)

 
     # Check for region.properties existence
     region_file_path = os.path.join(locale_path, _REGION_PROPS_PATH)
     _check_path_exists(region_file_path, logger)
 
     # Check for appstrings.properties existence
     appstrings_file_path = os.path.join(locale_path, _APPSTRINGS_PROPS_PATH)
     _check_path_exists(appstrings_file_path, logger)
+
+    ecosia_dst = os.path.join(locale_path,
+                              _LOCALE_SEARCHPLUGINS_PATH, "ecosia.xml")
 
     # Get whitelist and build regex
     whitelist = _SEARCH_ENGINE_ORDER.get(locale,
                                          _SEARCH_ENGINE_ORDER_DEFAULT)
     white_re = re.compile("^(%s).*$" % "|".join(whitelist))
 
+    if _ECOSIA_ID in whitelist and not os.path.exists(ecosia_dst):
+        shutil.copyfile(os.path.join(project_dir, _ECOSIA_PATH), ecosia_dst)
+
     all_engine_ids = []
     engine_ids = []
     removed_engine_ids = []
 
-    jsonFile = open(os.path.join(search_list_path, _LIST_JSON), 'r')
-    data = json.load(jsonFile)
-    jsonFile.close()

[diegocarloslima, 2017/12/13 10:44:42]

We could use the `with open` syntax here, to avoid the close statement

-
-    for item in data[_KEY_LOC][locale][_KEY_DEF][_KEY_VDF]:
+    for item in data['locales'][locale]['default'][_DEF_ENGINES]:
         all_engine_ids.append(item)
-        if white_re.match(item):
-            engine_ids.append(item)
-        else:
-            removed_engine_ids.append(item)
+        if len(item) > 0:
+            if white_re.match(item):
+                engine_ids.append(item)
+            else:
+                removed_engine_ids.append(item)
+
+    if _ECOSIA_ID in whitelist and _ECOSIA_ID not in all_engine_ids:
+        all_engine_ids.append(_ECOSIA_ID)
+        engine_ids.append(_ECOSIA_ID)
 
     # Make sure we still have search engines left
     if len(engine_ids) == 0:
         logger.fatal("No search engines left over for '%s'" % locale)
 
+    data['locales'][locale]['default'][_DEF_ENGINES] = all_engine_ids
+
     # 'Parse' XML to get matching 'ShortName' for all engine IDs
     engine_names = {}
-    search_plugins_path = os.path.join(build_dir, _SEARCHPLUGINS_PATH)
+    search_plugins_path = os.path.join(project_dir,
+                                       _GENERAL_SEARCHPLUGINS_PATH)
     for eid in engine_ids[:]:
         xml_file_path = os.path.join(search_plugins_path, "%s.xml" % eid)
         if not os.path.exists(xml_file_path):
             logger.info("Missing xml file for plugin %s. Searched in path %s" %
                         (eid, xml_file_path))
             engine_ids.remove(eid)
             continue
         short_name = None
         with open(xml_file_path, "r") as fd:
             for line in fd:
@@ skipping 26 matching lines @@
     with open(region_file_path, "r") as fd:
         for line in fd:
             line = line.rstrip("\r\n")
             if not _SEARCH_PROPS_RE.match(line.strip()):
                 props.append(line)
 
     # Append default search engine name
     props.append("browser.search.defaultenginename=%s" % engine_order[0])
 
     # Append search engine order
-    for i in range(0, min(5, len(engine_order))):

[diegocarloslima, 2017/12/13 10:44:42]

The condition here should be `for i in range(0, len(engine_order)):`.  We used
to have this limitation of 5 search engines, but we don't want this limitation
anymore, so we would append whatever we have.

Also, since we're not adding Ecosia into this piece of logic (it will be never
present in engine_order), it seems that Ecosia will be added for the correct
locales, but it won't be ordered properly, and it will be placed in last
position

+    for i in range(0, len(engine_order)):
         props.append("browser.search.order.%d=%s" % (i + 1, engine_order[i]))
 
     # Write back region.properties
     _write_lines(region_file_path, props)
 
     # Replaces ocurrences of 'Firefox' by 'Adblock Browser' in
     # 'appstrings.properties'
     lines = []
     replacement_count = 0
 
     with open(appstrings_file_path, "r") as fd:
         for line in fd:
             line = line.rstrip("\r\n")
             replacement = _replace_in_value(_PROPERTY_FORMAT_RE, line,
                                             _FIREFOX_REPLACE_STR,
                                             _ABB_REPLACEMENT_STR)
             if replacement:
                 line = replacement
                 replacement_count += 1
             lines.append(line)
 
     # Apply changes to appstrings.properties
     _write_lines(appstrings_file_path, lines)
     logger.info("Replaced %d ocurrences of %s in 'appstrings.properties'" %
                 (replacement_count, _FIREFOX_REPLACE_STR))
 
 
-def _generate_browser_search(locale, locale_path, res_path, build_dir):
+def _generate_browser_search(locale, locale_path, res_path, project_dir):
     raw_dir = "raw" if locale == _DEFAULT_LOCALE else (
         "raw-%s" % locale.replace("-", "-r"))
 
     browser_path = os.path.join(locale_path, _BROWSER_DIR)
     browsersearch_file_path = os.path.join(res_path, raw_dir,
                                            "browsersearch.json")
 
-    sys.path.append(os.path.join(build_dir, _MOZBUILD_PATH))
+    sys.path.append(os.path.join(project_dir, _MOZBUILD_PATH))
     import mozbuild.action.generate_browsersearch as generate_browsersearch
 
     # Call generate_browsersearch.py script to regenerate
     # res/raw-LOCALE/browsersearch.json with the updated search engines
     generate_browsersearch.main(["--verbose", "--srcdir", browser_path,
                                  browsersearch_file_path])
+
+
+def _generate_search_json(locale, locale_path, project_dir):
+    script_path = os.path.join(project_dir, "python", "mozbuild",
+                               "mozbuild", "action", "generate_searchjson.py")
+    list_json_path = os.path.join(project_dir, _LISTJSON_PATH)
+    searchplugins_path = os.path.join(locale_path, _BROWSER_DIR,
+                                      "searchplugins", "list.json")
+
+    import subprocess as s
+    # Call generate_searchjson.py script
+    s.check_call(['python', script_path, list_json_path,
+                 locale, searchplugins_path])
 
 
 def _transform_values_locale(locale, path, logger):
     logger.info("Processing values-%s..." % locale)
 
     # Check for strings.xml existence
     strings_file_path = os.path.join(path, _STRINGS_XML_PATH)
     _check_path_exists(strings_file_path, logger)
 
     # Replaces ocurrences of 'Firefox' by 'Adblock Browser' in 'strings.xml'
@@ skipping 30 matching lines @@
         print "INFO: %s" % s
 
     def error(self, s):
         print "ERROR: %s" % s
 
     def fatal(self, s):
         print "FATAL: %s" % s
         exit(1)
 
 
-def transform_locales(build_dir, obj_dir, logger=MinimalLogger()):
+def transform_locales(project_dir, obj_dir, logger=MinimalLogger()):
     chrome_path = os.path.join(obj_dir, _CHROME_PATH)
     _check_path_exists(chrome_path, logger)
 
     res_path = os.path.join(obj_dir, _RES_PATH)
     _check_path_exists(res_path, logger)
+
+    list_json_path = os.path.join(project_dir, _LISTJSON_PATH)
+    _check_path_exists(list_json_path, logger)
 
     locales = _get_locales_from_path(chrome_path, _LOCALE_RE)
     values_locales = _get_locales_from_path(res_path, _VALUES_LOCALE_RE)
 
     locales_found_msg = "Found %d locales in %s"
     logger.info(locales_found_msg % (len(locales), chrome_path))
     logger.info(locales_found_msg % (len(values_locales), res_path))
 
-    for locale in locales:
-        locale_path = os.path.join(chrome_path, locale, "locale", locale)
-        if os.path.exists(locale_path):
-            _transform_locale(locale, build_dir, locale_path, logger)
-            _generate_browser_search(locale, locale_path, res_path, build_dir)
-        else:
-            logger.error("Missing folder for locale '%s' in path: %s" %
-                         (locale, locale_path))
-
-    for locale in values_locales:
-        locale_path = os.path.join(res_path, "values-" + locale)
-        _transform_values_locale(locale, locale_path, logger)
-
-
-def transform_search_engines_list(abs_mozilla_dir, obj_dir,

[diegocarloslima, 2017/12/13 10:44:42]

I see that you're using here the `abs_mozilla_dir` nomenclature. On other
funcions we name it `build_dir`, which now I don't think it was a good name
choice :( . Anyways, I think we should  name it consistently, so it can be more
easily perceived that it means the same directory. I would suggest naming it
just mozilla_dir, or abb_dir or project_dir (and also replacing `build_dir`
occurrences). What do you think?

-                                  logger=MinimalLogger()):
     # open the Mozilla list of search engines, put it into a buffer and
     # close the JSON file after reading
-    search_list_path = os.path.join(abs_mozilla_dir, _LISTJSON_PATH)
-    jsonFile = open(os.path.join(search_list_path, _LIST_JSON), 'r')
-    data = json.load(jsonFile)
-    jsonFile.close()

[diegocarloslima, 2017/12/13 10:44:41]

We could use the `with open` syntax here, to avoid the close statement. Also,
this block is pretty much the same that we have in _transform_locale. It might
be better to have it as an utility function to avoid duplication of code

-
-    chrome_path = os.path.join(abs_mozilla_dir, _I10N_PATH)

[diegocarloslima, 2017/12/13 10:44:42]

Didn't really get why are you using a different chrome path here (in comparison
to chrome_path = os.path.join(obj_dir, _CHROME_PATH) )

-    _check_path_exists(chrome_path, logger)
-    # get all locales we want to transform
-    locales = _get_locales_from_path(chrome_path, _LOCALE_RE)
-
-    # set default search engine order
-    data[_KEY_DEF][_KEY_VDF] = _SEARCH_ENGINE_ORDER_DEFAULT
-    for loc in locales:

[diegocarloslima, 2017/12/13 10:44:42]

If we moved all the logic that is currently being added also in
_transform_locale to here, we could avoid lots of code duplications (e.g.
creation of whitelist , whitelist_re, all_engine_ids, engine_ids,
removed_engine_ids, and so on)

[diegocarloslima, 2017/12/13 10:55:29]

Also, I think that this should be an internal function, in order to avoid
creating a further step in multi_locale_build. So the execution would be
something like this:

transform_locales() would have the main logic for reading the json file and it
would pass to _transform_search_engines() which would reorder the search engines
for each locale by rewriting the region.properties and would modify the json
data structure that would be later been written back in list.json, inside
transform_locales(), once _transform_search_engines() has ended

-        whitelist = _SEARCH_ENGINE_ORDER.get(loc,
-                                             _SEARCH_ENGINE_ORDER_DEFAULT)
-        white_re = re.compile("^(%s).*$" % "|".join(whitelist))
-
-        # Read engine IDs from list.json, discard engines not on whitelist
-        all_engine_ids = []
-        engine_ids = []
-        removed_engine_ids = []
-
-        # Mozilla default list does not contain locale bn-BD, so we create it
-        # and use the values from locale bn-IN
-        if loc == "bn-BD":
-            data[_KEY_LOC].update({loc: {_KEY_DEF: {_KEY_VDF: data[_KEY_LOC]
-                                   ["bn-IN"][_KEY_DEF][_KEY_VDF]}}})
-        # Mozilla default list does not contain locale wo, so we use the
-        # default order. In case they will not support any other locales in
-        # the future, we want the build to fail, to decide which order to use
-        elif loc == "wo":
-            data[_KEY_LOC].update({loc: {_KEY_DEF:
-                                   {_KEY_VDF: _SEARCH_ENGINE_ORDER_DEFAULT}}})
-
-        for item in data[_KEY_LOC][loc][_KEY_DEF][_KEY_VDF]:
-            all_engine_ids.append(item)
-            if len(item) > 0:
-                if white_re.match(item):
-                    engine_ids.append(item)
-                else:
-                    removed_engine_ids.append(item)
-
-        if _ECOSIA_ID in whitelist and _ECOSIA_ID not in all_engine_ids:
-            all_engine_ids.append(_ECOSIA_ID)
-            engine_ids.append(_ECOSIA_ID)
-
-        # Make sure we still have search engines left
-        if len(engine_ids) == 0:
-            logger.fatal("No search engines left over for '%s'" % loc)
-
-        data[_KEY_LOC][loc][_KEY_DEF][_KEY_VDF] = all_engine_ids
-
-    # Save our changes to list.json
-    with open(os.path.join(search_list_path, _LIST_JSON), 'w') as outfile:
-        json.dump(data, outfile, indent=4, sort_keys=True)
+    with open(list_json_path, 'r') as json_file:
+        data = json.load(json_file)
+
+        # set default search engine order
+        data['default'][_DEF_ENGINES] = _SEARCH_ENGINE_ORDER_DEFAULT
+
+        for locale in locales:
+            locale_path = os.path.join(chrome_path, locale, "locale", locale)
+            if os.path.exists(locale_path):
+
+                # Mozilla default list does not contain locale bn-BD,
+                # so we create it and use the values from locale bn-IN
+                if locale == 'bn-BD':
+                    data['locales'].update({locale: {'default':
+                                           {_DEF_ENGINES:
+                        data['locales']['bn-IN']['default'][_DEF_ENGINES]}}})
+                # Mozilla default list does not contain locale wo, so we use
+                # the default order. In case they will not support any other
+                # locales in the future, we want the build to fail, to decide
+                #  which order to use
+                elif locale == 'wo':
+                    data['locales'].update({locale: {'default':
+                                           {_DEF_ENGINES:
+                                            _SEARCH_ENGINE_ORDER_DEFAULT}}})
+
+                _transform_locale(data, locale, project_dir, locale_path,
+                                  logger)
+                _generate_browser_search(locale, locale_path, res_path,
+                                         project_dir)
+            else:
+                logger.error("Missing folder for locale '%s' in path: %s" %
+                             (locale, locale_path))
+
+        # Save changes to list.json
+        with open(list_json_path, 'w') as outfile:
+            json.dump(data, outfile, indent=4, sort_keys=True)
+
+        # Generate search.json for each locale
+        for locale in locales:
+            locale_path = os.path.join(chrome_path, locale, "locale", locale)
+            _generate_search_json(locale, locale_path, project_dir)
+
+        for locale in values_locales:
+            locale_path = os.path.join(res_path, "values-" + locale)
+            _transform_values_locale(locale, locale_path, logger)