902592 - Manifestparser should throw useful exceptions in strict mode

Status: NEW

(Testing :: Mozbase, defect)

Description

[Gregory Szorc [:gps]]

I'm hooking manifest parsing up to the build system. I'd like to use strict mode to parse manifests. However, when strict mode is enabled and when there is a violation, an assertion is raised. Unfortunately, this is reported as something like:

Traceback (most recent call last):
  File "./config.status", line 928, in <module>
    config_status(**args)
  File "/Users/gps/src/firefox/build/ConfigStatus.py", line 127, in config_status
    summary = backend.consume(definitions)
  File "/Users/gps/src/firefox/python/mozbuild/mozbuild/backend/base.py", line 176, in consume
    for obj in objs:
  File "/Users/gps/src/firefox/python/mozbuild/mozbuild/frontend/emitter.py", line 47, in emit
    for o in self.emit_from_sandbox(out):
  File "/Users/gps/src/firefox/python/mozbuild/mozbuild/frontend/emitter.py", line 152, in emit_from_sandbox
    m.read(path)
  File "/Users/gps/src/firefox/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py", line 441, in read
    self._read(here, filename, defaults)
  File "/Users/gps/src/firefox/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py", line 378, in _read
    sections = read_ini(fp=filename, variables=defaults, strict=self.strict)
  File "/Users/gps/src/firefox/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py", line 329, in read_ini
    assert key not in current_section
AssertionError

This error message is not actionable. The best moz.build parsing can do is identify the file with the issue. Developers *will* accidentally make strict violations and need a way to identify where in the file the error occurs. Can we get manifest parsing to throw an error message with more details? Even better would be a custom Exception-derived class that contained all the metadata and we could just trap and stringify.

Comment 1

[Karan Singh]

I'd like to work on this bug if possible, thanks.

Comment 2

[:Gijs (he/him)]

So apparently this caused us to have a longtime-broken condition in at least one manifest (bug 1096804 ran into this when it committed test code that didn't parse (so it broke lint) but that passed mochitest-bc -- because the test was turned off permanently, using the following condition:

skip-if = os != win

(note lack of quotes around "win"). I wouldn't be surprised if there were more broken manifests in the tree.

Glandium mentioned a-team owns the test manifest parser... Andrew, can you suggest someone who could drive this? :-)

Comment 3

[Andrew Halberstadt [:ahal]]

To enable strict mode, we can just flip strict=False to True:
https://dxr.mozilla.org/mozilla-central/search&redirect=false&q=ext%3Apy%20regexp%3A%22%20TestManifest%5C%28.*%5C%29%22%20-path%3Apython%2Fmozbuild%2F%20-path%3Atesting/mozbase/manifestparser%2F

This bug seems to be about better error messages, is that the problem here? I don't think better error messages should block flipping that boolean, as a confused developer is still better than silent manifest failures. And we already have strict mode enabled for xpcshell anyway.

Comment 4

[Andrew Halberstadt [:ahal]]

Ugh, dxr is not liking the escaped version of that url.. Here's the search term I was trying to link:
ext:py regexp:" TestManifest\(.*\)" -path:python/mozbuild/ -path:testing/mozbase/manifestparser/

Comment 5

[Gregory Szorc [:gps]]

Bug 902609 is where xpcshell switched to strict mode. I can't find anything in my inbox related to enabling strict mode elsewhere. I think globally enabling strict mode for all manifests and eventually removing support for non-strict mode from the manifest parser is a noble goal. We've certainly been trending more and more towards failing faster when it comes to the build configuration. The fact that we can accidentally disable tests due to poor manifest syntax is a massive foot gun :/

Comment 6

[Andrew Halberstadt [:ahal]]

Strict mode actually is enabled by default:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#47

It's just mochitest that seems to pass in strict=False. I'd be ok with removing strict mode altogether if you feel strongly about it, but I'm equally ok with leaving it in and frowning upon it.