Bug 1333564 - [manifestparser] Stop supporting ';' as a valid comment character, r?jmaher draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Wed, 25 Jan 2017 14:38:37 -0500
changeset 466825 feff0c1c04f933b583b3641222177607eef5bee9
parent 466824 54f17191340ace72a9cefa5591d4f79f83bd334c
child 466826 7eca443a73dff58c39dddc932a423fa8aa086b03
push id43008
push userahalberstadt@mozilla.com
push dateThu, 26 Jan 2017 17:34:22 +0000
reviewersjmaher
bugs1333564
milestone54.0a1
Bug 1333564 - [manifestparser] Stop supporting ';' as a valid comment character, r?jmaher It turns out there are shockingly few cases of manifestparser manifests that actually use the ';' character as a comment. Because we will soon allow inline comments, deprecating the use of ';' will ensure that values are allowed to have semicolons in them. Even without inline comments, might as well enforce consistency across manifests. MozReview-Commit-ID: AEPPQFdNXG0
python/mozbuild/mozbuild/backend/recursivemake.py
python/mozbuild/mozbuild/test/backend/test_recursivemake.py
testing/marionette/harness/marionette_harness/tests/unit-tests.ini
testing/mozbase/docs/manifestparser.rst
testing/mozbase/manifestparser/manifestparser/ini.py
testing/mozbase/manifestparser/tests/comment-example.ini
testing/mozbase/manifestparser/tests/test_read_ini.py
testing/xpcshell/example/unit/xpcshell.ini
toolkit/components/timermanager/tests/unit/xpcshell.ini
toolkit/mozapps/update/tests/chrome/chrome.ini
toolkit/mozapps/update/tests/unit_aus_update/xpcshell.ini
toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
--- a/python/mozbuild/mozbuild/backend/recursivemake.py
+++ b/python/mozbuild/mozbuild/backend/recursivemake.py
@@ -1428,17 +1428,17 @@ class RecursiveMakeBackend(CommonBackend
 
         for k, manifest in manifests.items():
             with self._write_file(mozpath.join(man_dir, k)) as fh:
                 manifest.write(fileobj=fh)
 
     def _write_master_test_manifest(self, path, manifests):
         with self._write_file(path) as master:
             master.write(
-                '; THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT MODIFY BY HAND.\n\n')
+                '# THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT MODIFY BY HAND.\n\n')
 
             for manifest in sorted(manifests):
                 master.write('[include:%s]\n' % manifest)
 
     class Substitution(object):
         """BaseConfigSubstitution-like class for use with _create_makefile."""
         __slots__ = (
             'input_path',
--- a/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
+++ b/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ -514,17 +514,17 @@ class TestRecursiveMakeBackend(BackendTe
         tests_dir = mozpath.join(env.topobjdir, '_tests')
         m_master = mozpath.join(tests_dir, 'testing', 'mochitest', 'tests', 'mochitest.ini')
         x_master = mozpath.join(tests_dir, 'xpcshell', 'xpcshell.ini')
         self.assertTrue(os.path.exists(m_master))
         self.assertTrue(os.path.exists(x_master))
 
         lines = [l.strip() for l in open(x_master, 'rt').readlines()]
         self.assertEqual(lines, [
-            '; THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT MODIFY BY HAND.',
+            '# THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT MODIFY BY HAND.',
             '',
             '[include:dir1/xpcshell.ini]',
             '[include:xpcshell.ini]',
         ])
 
         all_tests_path = mozpath.join(env.topobjdir, 'all-tests.pkl')
         self.assertTrue(os.path.exists(all_tests_path))
 
--- a/testing/marionette/harness/marionette_harness/tests/unit-tests.ini
+++ b/testing/marionette/harness/marionette_harness/tests/unit-tests.ini
@@ -1,11 +1,11 @@
-; marionette unit tests
+# marionette unit tests
 [include:unit/unit-tests.ini]
 
-; layout tests
+# layout tests
 [include:../../../../../layout/base/tests/marionette/manifest.ini]
 
-; microformats tests
+# microformats tests
 [include:../../../../../toolkit/components/microformats/manifest.ini]
 
-; migration tests
+# migration tests
 [include:../../../../../browser/components/migration/tests/marionette/manifest.ini]
--- a/testing/mozbase/docs/manifestparser.rst
+++ b/testing/mozbase/docs/manifestparser.rst
@@ -131,24 +131,23 @@ section, without adding possible include
 
 .. code-block:: text
 
     [parent:../manifest.ini]
 
 Manifests are included relative to the directory of the manifest with
 the `[include:]` directive unless they are absolute paths.
 
-By default you can use both '#' and ';' as comment characters. Comments
-must start on a new line, inline comments are not supported.
+By default you can use '#' as a comment character. Comments must start on
+a new line, inline comments are not supported.
 
 .. code-block:: text
 
     [roses.js]
     # a valid comment
-    ; another valid comment
     color = red # not a valid comment
 
 In the example above, the 'color' property will have the value 'red #
 not a valid comment'.
 
 Special variable server-root
 ````````````````````````````
 There is a special variable called `server-root` used for paths on the system.
--- a/testing/mozbase/manifestparser/manifestparser/ini.py
+++ b/testing/mozbase/manifestparser/manifestparser/ini.py
@@ -3,32 +3,33 @@
 # You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
 
 __all__ = ['read_ini', 'combine_fields']
 
 
 def read_ini(fp, variables=None, default='DEFAULT', defaults_only=False,
-             comments=(';', '#'), separators=('=', ':'), strict=True,
-             handle_defaults=True):
+             comments=None, separators=None, strict=True, handle_defaults=True):
     """
     read an .ini file and return a list of [(section, values)]
     - fp : file pointer or path to read
     - variables : default set of variables
     - default : name of the section for the default section
     - defaults_only : if True, return the default section only
     - comments : characters that if they start a line denote a comment
     - separators : strings that denote key, value separation in order
     - strict : whether to be strict about parsing
     - handle_defaults : whether to incorporate defaults into each section
     """
 
     # variables
     variables = variables or {}
+    comments = comments or ('#',)
+    separators = separators or ('=', ':')
     sections = []
     key = value = None
     section_names = set()
     if isinstance(fp, basestring):
         fp = file(fp)
 
     # read the lines
     for (linenum, line) in enumerate(fp.read().splitlines(), start=1):
--- a/testing/mozbase/manifestparser/tests/comment-example.ini
+++ b/testing/mozbase/manifestparser/tests/comment-example.ini
@@ -1,11 +1,11 @@
-; See https://bugzilla.mozilla.org/show_bug.cgi?id=813674
+# See https://bugzilla.mozilla.org/show_bug.cgi?id=813674
 
 [test_0180_fileInUse_xp_win_complete.js]
 [test_0181_fileInUse_xp_win_partial.js]
 [test_0182_rmrfdirFileInUse_xp_win_complete.js]
 [test_0183_rmrfdirFileInUse_xp_win_partial.js]
 [test_0184_fileInUse_xp_win_complete.js]
 [test_0185_fileInUse_xp_win_partial.js]
 [test_0186_rmrfdirFileInUse_xp_win_complete.js]
 [test_0187_rmrfdirFileInUse_xp_win_partial.js]
-; [test_0202_app_launch_apply_update_dirlocked.js] # Test disabled, bug 757632
\ No newline at end of file
+# [test_0202_app_launch_apply_update_dirlocked.js] # Test disabled, bug 757632
--- a/testing/mozbase/manifestparser/tests/test_read_ini.py
+++ b/testing/mozbase/manifestparser/tests/test_read_ini.py
@@ -41,32 +41,11 @@ kittens = true # This test requires kitt
         # inline comment delimeter (for "backwards compatability"):
         # http://docs.python.org/2/library/configparser.html
         buffer.seek(0)
         parser = ConfigParser()
         parser.readfp(buffer)
         control = parser.get('test_felinicity.py', 'kittens')
         self.assertEqual(result, control)
 
-        # test ';' inline comments (really, the lack thereof)
-        string = string.replace('#', ';')
-        buffer = StringIO()
-        buffer.write(string)
-        buffer.seek(0)
-        result = read_ini(buffer)[0][1]['kittens']
-        self.assertEqual(result, "true ; This test requires kittens")
-
-        # compare this to ConfigParser
-        # python 2.7 ConfigParser *does* support ';' as an
-        # inline comment delimeter (ibid).
-        # Python 3.x configparser, OTOH, does not support
-        # inline-comments by default.  It does support their specification,
-        # though they are weakly discouraged:
-        # http://docs.python.org/dev/library/configparser.html
-        buffer.seek(0)
-        parser = ConfigParser()
-        parser.readfp(buffer)
-        control = parser.get('test_felinicity.py', 'kittens')
-        self.assertNotEqual(result, control)
-
 
 if __name__ == '__main__':
     mozunit.main()
--- a/testing/xpcshell/example/unit/xpcshell.ini
+++ b/testing/xpcshell/example/unit/xpcshell.ini
@@ -1,11 +1,11 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 [DEFAULT]
 head =
 skip-if = toolkit == 'gonk'
 support-files =
   subdir/file.txt
   file.txt
   import_module.jsm
--- a/toolkit/components/timermanager/tests/unit/xpcshell.ini
+++ b/toolkit/components/timermanager/tests/unit/xpcshell.ini
@@ -1,8 +1,8 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 [DEFAULT]
 head = 
 
 [consumerNotifications.js]
--- a/toolkit/mozapps/update/tests/chrome/chrome.ini
+++ b/toolkit/mozapps/update/tests/chrome/chrome.ini
@@ -1,20 +1,20 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 [DEFAULT]
 tags = appupdate
 support-files =
   utils.js
   update.sjs
 
-; mochitest-chrome tests must start with "test_" and are executed in sorted
-; order and not in the order specified in the manifest.
+# mochitest-chrome tests must start with "test_" and are executed in sorted
+# order and not in the order specified in the manifest.
 [test_0010_background_basic.xul]
 [test_0011_check_basic.xul]
 [test_0012_check_basic_staging.xul]
 skip-if = asan
 reason = Bug 1168003
 [test_0013_check_no_updates.xul]
 [test_0014_check_error_xml_malformed.xul]
 [test_0061_check_verifyFailPartial_noComplete.xul]
--- a/toolkit/mozapps/update/tests/unit_aus_update/xpcshell.ini
+++ b/toolkit/mozapps/update/tests/unit_aus_update/xpcshell.ini
@@ -1,11 +1,11 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 [DEFAULT]
 tags = appupdate
 head = head_update.js
 
 [canCheckForAndCanApplyUpdates.js]
 [urlConstruction.js]
 [updateManagerXML.js]
--- a/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
+++ b/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
@@ -1,15 +1,15 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-; Tests that require the updater binary. These tests should never run on Android
-; which doesn't use the updater binary as other applications do and are excluded
-; from running the tests in the moz.build file.
+# Tests that require the updater binary. These tests should never run on Android
+# which doesn't use the updater binary as other applications do and are excluded
+# from running the tests in the moz.build file.
 
 [DEFAULT]
 tags = appupdate
 head = head_update.js
 
 [marSuccessComplete.js]
 skip-if = os == 'win' && debug && (os_version == '5.1' || os_version == '5.2')
 reason = bug 1291985
--- a/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
+++ b/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
@@ -1,13 +1,13 @@
-; This Source Code Form is subject to the terms of the Mozilla Public
-; License, v. 2.0. If a copy of the MPL was not distributed with this
-; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-; Tests that require the updater binary and the maintenance service.
+# Tests that require the updater binary and the maintenance service.
 
 [DEFAULT]
 tags = appupdate
 head = head_update.js
 
 [bootstrapSvc.js]
 skip-if = os == 'win' && debug && (os_version == '5.1' || os_version == '5.2')
 reason = bug 1291985