1333564 - Manifestparser should support inline comments everywhere (or nowhere)

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

Currently you can add inline comments to conditional keys like 'skip-if' or 'subsuite-if', but nowhere else.

This is bad behaviour, we shouldn't be special casing keys like this. I'd prefer if inline comments were globally supported, though doing this properly (i.e solving all the edge cases), is actually a fairly hard problem, and why manifestparser hasn't supported inline comments before.

For example, this is a comment:
[foo]
bar = baz  # fleem

But these are not comments:
[foo]
bar = https://foo.com/bar#fleem
baz = "foo#bar"

Comment 1

[Andrew Halberstadt [:ahal]]

Note if this gets fixed and bug 1333506 hasn't been backed out yet, please revert that change as part of this bug.

Comment 2

[Andrew Halberstadt [:ahal]]

I ended up backing out the change in bug 1333506.

For the record, the configparser stdlib also disables inline comments by default, though they do have an option to enable them. Maybe I can see how they implement it and try to copy them.

Comment 3

[Andrew Halberstadt [:ahal]]

In the long run, I think we should use the configobj library as the backend ini parser for manifestparser. They use several advanced regexes to properly parse out inline comments. Besides, maintaining our own ini parser is kind of crazy. But doing that is a big change.

For the short term (in this bug), I'm thinking we should just copy what configparser does.. That is, disable inline comments by default but allow consumers to enable them, with an appropriate warning that doing so means comment characters will no longer be valid in the values.

Notably, a comment character will only start a comment if it is at the start of the line, or preceded by a whitespace. This will at least solve the issue for urls.

I'll file a follow-up for manifestparser using ConfigObj instead.

Comment 4

[Andrew Halberstadt [:ahal]]

Found the old WONTFIX'ed bug on this.

It looks like most of the concerns where around python version support. And indeed, we did want to use configparser at some point, but couldn't due to differences between python 2.6 vs 2.7. That is no longer in issue, though I believe configobj is strictly better than configparser.

Also, it looks like implement the python 3 behaviour of configparser was considered in the old bug, which is basically what I'm proposing to do here.

Comment 5

[Andrew Halberstadt [:ahal]]

I also think that anytime we enable inline comments, we should remove the ';' character as a valid comment to minimize the chances of something going wrong.. We should probably just remove it by default, the '#' character is by far the more popular one.

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1333564 - [manifestparser] Allow multi-character comment tokens,

Review commit: https://reviewboard.mozilla.org/r/107514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107514/

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1333564 - [manifestparser] Stop supporting ';' as a valid comment character,

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.

Review commit: https://reviewboard.mozilla.org/r/107516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107516/

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1333564 - [manifestparser] Add support for inline comments,

Previously manifestparser only supported inline comments on conditional keys, such as
skip-if, fails-if, subsuite-if, etc. But on any other key name, inline comments weren't
supported. This was a bad situation because people saw these skip-if comments, and
naturally assumed inline comments worked everywhere. Then they were surprised when things
broke in mysterious ways.

This patch removes the special-casing for skip-if and friends and allows inline comments
everywhere. A caveat to this, is that ' #' is no longer a valid substring in a value.

Review commit: https://reviewboard.mozilla.org/r/107518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107518/

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1333564 - [manifestparser] Include manifest path when raising exceptions from the ini parser,

Review commit: https://reviewboard.mozilla.org/r/107520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107520/

Comment 10

[Andrew Halberstadt [:ahal]]

Because this could potentially change the behaviour of manifests that have '#' or ';' characters as values, I did a full try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aea22c206b5a6d11ce579e17e0496a4f0d873f3b

I also did a bunch of dxr regex searching to try and find cases where this might cause problems. I think based on both results, we should be in the clear.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8830800 [details]
Bug 1333564 - [manifestparser] Allow multi-character comment tokens,

https://reviewboard.mozilla.org/r/107514/#review108638

nice simple change

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8830801 [details]
Bug 1333564 - [manifestparser] Stop supporting ';' as a valid comment character,

https://reviewboard.mozilla.org/r/107516/#review108644

I will assume you have converted all of the comment lines starting with ';'

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8830802 [details]
Bug 1333564 - [manifestparser] Add support for inline comments,

https://reviewboard.mozilla.org/r/107518/#review108648

::: testing/mozbase/manifestparser/manifestparser/ini.py:159
(Diff revision 1)
>          if field_name not in field_patterns or field_name not in global_vars:
>              final_mapping[field_name] = value
>              continue
>          global_value = global_vars[field_name]
>          pattern = field_patterns[field_name]
> -        final_mapping[field_name] = pattern % (
> +        final_mapping[field_name] = pattern % (global_value, value)

as a related note, this type of variable % subst_vars, is really hard to read- when I cannot see what pattern is easily in a review, then it leads me to believe we have a case for errors :(

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8830803 [details]
Bug 1333564 - [manifestparser] Include manifest path when raising exceptions from the ini parser,

https://reviewboard.mozilla.org/r/107520/#review108662

++ for better error messages.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76bd6b8d073b
[manifestparser] Allow multi-character comment tokens, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/0afb6eb46538
[manifestparser] Stop supporting ';' as a valid comment character, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/e1724b21517c
[manifestparser] Add support for inline comments, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/93502f9988b9
[manifestparser] Include manifest path when raising exceptions from the ini parser, r=jmaher

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76bd6b8d073b
https://hg.mozilla.org/mozilla-central/rev/0afb6eb46538
https://hg.mozilla.org/mozilla-central/rev/e1724b21517c
https://hg.mozilla.org/mozilla-central/rev/93502f9988b9

Comment 17

[aleth [:aleth]]

There seems to be something wrong with the error message being produced when comments begin with ; - note the missing filename. The actual error message "No sections found" might also be improveable?

 0:13.59 ==============================
 0:13.59 ERROR PROCESSING MOZBUILD FILE
 0:13.59 ==============================
 0:13.59 
 0:13.59 The error occurred while processing the following file:
 0:13.59 
 0:13.59     /Users/helvellyn/buildhg/cc1/chat/components/src/moz.build
 0:13.59 
 0:13.59 The error was triggered on line 6 of this file:
 0:13.59 
 0:13.59     XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell.ini']
 0:13.59 
 0:13.59 An error was encountered as part of executing the file itself. The error appears to be the fault of the script.
 0:13.59 
 0:13.59 The error as reported by Python is:
 0:13.59 
 0:13.59     ["Exception: Error parsing manifest file 'unknown', line 1: No sections found\n"]

Comment 18

[Andrew Halberstadt [:ahal]]

The missing filename depends on how the list of manifests was initially passed into ManifestParser. For example, it's possible to pass in either a list of path strings, or a list of file-like objects. If the latter is used, we look for a 'name' attribute, and if that doesn't exist then we simply print 'unknown'. If you have control over this list, make sure there is a 'name' attribute on the file objects.

But yes, I think the wording there could definitely be improved.. I'll file a new bug.

Comment 19

[Andrew Halberstadt [:ahal]]

Also, looks like it's the build system that is failing to add a 'name' attribute.. I'll see if I can fix that as well.

Comment 20

[Stefan Plewako [:stef]]

Does this affect l10n also? https://dxr.mozilla.org/l10n-central/search?q=ext%3Aini&redirect=false

Comment 21

[Andrew Halberstadt [:ahal]]

No, this only affects test manifests (things parsed by /testing/mozbase/manifestparser, e.g mochitest.ini, xpcshell.ini, etc). Those are not manifestparser manifests.