1232421 - manifest.py parses every # as a comment, while preprocessor.py will only match leading # lines as commented-out lines

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Gijs (he/him)]

STR:

1. add the following line to a jar.mn file:

% override chrome://browser/content/foo.svg#yoink chrome://browser/content/foo.svg#boink

2. run ./mach build

It works! (expected)

3. run ./mach package

It breaks! (not expected)

Specifically:

10:59:43     INFO -  	c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/browser/installer/package-manifest.in ../../dist ../../dist/firefox \
10:59:43     INFO -  Traceback (most recent call last):
10:59:43     INFO -    File "c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 406, in <module>
10:59:43     INFO -      main()
10:59:43     INFO -    File "c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 348, in main
10:59:43     INFO -      preprocess_manifest(sink, args.manifest, defines)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 202, in preprocess_manifest
10:59:43     INFO -      preprocess(manifest, PackageManifestParser(sink), defines)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 193, in preprocess
10:59:43     INFO -      pp.do_include(input)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozbuild\preprocessor.py", line 780, in do_include
10:59:43     INFO -      self.handleLine(l)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozbuild\preprocessor.py", line 558, in handleLine
10:59:43     INFO -      self.write(aLine)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozbuild\preprocessor.py", line 438, in write
10:59:43     INFO -      self.out.write(filteredLine)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 181, in write
10:59:43     INFO -      self._parser.handle_line(str)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 166, in handle_line
10:59:43     INFO -      self._sink.add(self._component, str)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 384, in add
10:59:43     INFO -      self.packager.add(dest, f)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 262, in add
10:59:43     INFO -      self._add_manifest_file(path, file)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\packager\__init__.py", line 285, in _add_manifest_file
10:59:43     INFO -      for e in parse_manifest(base, path, file.open()):
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\chrome\manifest.py", line 358, in parse_manifest
10:59:43     INFO -      e = parse_manifest_line(base, line)
10:59:43     INFO -    File "c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\python\mozbuild\mozpack\chrome\manifest.py", line 342, in parse_manifest_line
10:59:43     INFO -      return MANIFESTS_TYPES[cmd[0]](base, *cmd[1:])
10:59:43     INFO -  TypeError: __init__() takes at least 4 arguments (3 given)


I believe that the comment regexp:

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/chrome/manifest.py#328

is too liberal and should only match line-leading #, the same way that the preprocessor does.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1232421 - force matching the start of the line for comments, r?glandium

Bug 1232421 - force matching the start of the line for comments, r?gps

Comment 2

[:Gijs (he/him)]

I'm still allowing leading whitespace on the line because of this jar.mn file:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/loop/jar.mn#33

(where I don't really understand how that file isn't breaking things right now, unless the answer is https://developer.mozilla.org/en/docs/Chrome_Registration#comments )

Comment 3

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/27845/#review25009

::: python/mozbuild/mozpack/chrome/manifest.py:328
(Diff revision 1)
> -MANIFEST_RE = re.compile(r'\s*#.*$')
> +MANIFEST_RE = re.compile(r'^\s*#.*$')

It seems to me we should still consider non-line-leading # if it's preceded by a whitespace.

Comment 4

[Mike Hommey [:glandium]]

(In reply to :Gijs Kruitbosch from comment #0)
> % override chrome://browser/content/foo.svg#yoink chrome://browser/content/foo.svg#boink

Wow, is this actually supported?

Comment 5

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #3)
> https://reviewboard.mozilla.org/r/27845/#review25009
> 
> ::: python/mozbuild/mozpack/chrome/manifest.py:328
> (Diff revision 1)
> > -MANIFEST_RE = re.compile(r'\s*#.*$')
> > +MANIFEST_RE = re.compile(r'^\s*#.*$')
> 
> It seems to me we should still consider non-line-leading # if it's preceded
> by a whitespace.

Why? The regular preprocessor doesn't. Why keep the inconsistency? Ni for this.

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #0)
> > % override chrome://browser/content/foo.svg#yoink chrome://browser/content/foo.svg#boink
> 
> Wow, is this actually supported?

The opposite isn't supported, ie:

override foo.svg bar.svg

doesn't work if you actually ask for foo.svg#whatever as an image - you still get foo.svg instead of bar.svg. I'd like to fix that, but short-term we just need to fix the inconsistency between these two parsers.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8698153 [details]
MozReview Request: Bug 1232421 - force matching the start of the line for comments, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27845/diff/1-2/

Comment 7

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/27845/#review25141

::: python/mozbuild/mozpack/chrome/manifest.py:328
(Diff revision 2)
> -MANIFEST_RE = re.compile(r'\s*#.*$')
> +MANIFEST_RE = re.compile(r'(\s+|^)#.*$')

The parser that actually matters is the one in xpcom/components/ManifestParser.cpp, and it only considers lines starting with a # as comments. (as in ^#).

Come to think of it, technically, this is what the parser in chrome/manifest.py should be using, and the jar manifest parser should munge things appropriately if we consider that jar.mn files may have comments at the end of those lines. It's also the jar.mn parser's job to remove the whitespaces between the % and the chrome manifest keyword, since that's part of the jar.mn syntax, not the chrome manifest syntax itself.

So all in all, ^#.*$ would be the right thing to have here. The jar.mn parser already removes whitespaces between % and the chrome manifest entry, but doesn't handle comments at the end of the line, which we don't have anyways, so it's just fine.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8698153 [details]
MozReview Request: Bug 1232421 - force matching the start of the line for comments, r?glandium

Looks like mozreview didn't propagate the r+.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d0f136a93ad3

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/45a3b79cc3a4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0f136a93ad3
https://hg.mozilla.org/mozilla-central/rev/45a3b79cc3a4

Comment 12

[Richard Marti (:Paenglab)]

Gijs, please could you ask for uplift for aurora? I'd like then to ask for uplift of bug 1191230 to be also in the ESR versions.

Comment 13

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 14

[:Gijs (he/him)]

Comment on attachment 8699434 [details] [diff] [review]
Patch for aurora

Approval Request Comment
[Feature/regressing bug #]: blocking using SVG with overrides in jar.mn files
[User impact if declined]: can't land bug 1232421 on aurora
[Describe test coverage new/current, TreeHerder]: there's a test, this patch modifies it
[Risks and why]: 1-line build system change for a system that has tests, aurora only just branched 3 days ago - very low risk
[String/UUID change made/needed]: no

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8699434 [details] [diff] [review]
Patch for aurora

Ahhhh, regexp :p
Taking it in aurora.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38f291c5c2bb