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

RESOLVED FIXED in Firefox 45

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8698153 [details]
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
Attachment #8698153 - Flags: review?(gps)
(Assignee)

Comment 2

2 years ago
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 )
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.
(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?
Attachment #8698153 - Flags: review?(gps)
(Assignee)

Comment 5

2 years ago
(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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 6

2 years ago
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/
Attachment #8698153 - Attachment description: MozReview Request: Bug 1232421 - force matching the start of the line for comments, r?gps → MozReview Request: Bug 1232421 - force matching the start of the line for comments, r?glandium
Attachment #8698153 - Flags: review?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
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+.
Attachment #8698153 - Flags: review?(mh+mozilla) → review+

Comment 9

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

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d0f136a93ad3
https://hg.mozilla.org/mozilla-central/rev/45a3b79cc3a4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 13

2 years ago
Created attachment 8699434 [details] [diff] [review]
Patch for aurora
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
(Assignee)

Comment 14

2 years ago
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8699434 - Flags: approval-mozilla-aurora?
Comment on attachment 8699434 [details] [diff] [review]
Patch for aurora

Ahhhh, regexp :p
Taking it in aurora.
Attachment #8699434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/38f291c5c2bb
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.