(derived from bug 1289050 comment #43) In bug 1289050, we'll require js.msg and all other .msg files for JSErrorFormatString to be in right encoding. currently it should be in ASCII, and we'll change the expected encoding to UTF-8 maybe shortly after bug 1289050. We should statically check the encoding (and hopefully format, parameter count, etc).

(just a thought for other bug) maybe, we could statically parse js.msg etc, and generate a function that receives formatting parameter, and reports it? so that we don't have to scan the format string on runtime.

summary: * added JS_MSG_FILES to moz.build, that is a list of js.msg-like files that defines JSErrorFormatString array * files in JS_MSG_FILES are checked the following things on compile time: * encoding of text currently ASCII, will changed to UTF-8 once js::ExpandErrorArgumentsVA is changed * parameter count * parameter syntar * added scheme definition to each js.msg-like file, as macro may differ for each file

Comment on attachment 8794414 [details] [diff] [review] Statically check js.msg encoding and syntax. Review of attachment 8794414 [details] [diff] [review]: ----------------------------------------------------------------- This will definitely need review from a build system peer. Main suggestion: can we make the script work more like check_spidermonkey_style.py? Basically grab the *.msg files from |hg manifest| (or similar for git), so we don't need to change moz.build files, and then process all of them. That way you might also be able to write some tests (see the expected_output code in check_spidermonkey_style.py). What do you think? :) ::: dom/bindings/Errors.msg @@ +95,5 @@ > MSG_DEF(MSG_SW_INSTALL_ERROR, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} encountered an error during installation.") > MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") > MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a typed array on SharedArrayBuffer") > MSG_DEF(MSG_CACHE_ADD_FAILED_RESPONSE, 3, JSEXN_TYPEERR, "Cache got {0} response with bad status {1} while trying to add request {2}") > +MSG_DEF(MSG_SW_UPDATE_BAD_REGISTRATION, 2, JSEXN_TYPEERR, "Failed to update the ServiceWorker for scope {0} because the registration has been {1} since the update was scheduled.") Good find! ::: js/src/shell/moz.build @@ +17,5 @@ > 'OSObject.cpp' > ] > > +JS_MSG_FILES += [ > + '../jsshell.msg', File a follow-up to move jsshell.msg to js/src/shell/ ? ::: js/xpconnect/src/jsshell.msg @@ +13,2 @@ > MSG_DEF(JSSMSG_NOT_AN_ERROR, 0, 0, JSEXN_ERR, "<Error #0 is reserved>") > MSG_DEF(JSSMSG_CANT_OPEN, 1, 2, JSEXN_ERR, "can't open {0}: {1}") If we remove the second column, then every js.msg file will have (name, count, _, text) format right? Can we remove js_msg_scheme then and check for that? ::: python/mozbuild/mozbuild/action/process_js_msg_files.py @@ +1,5 @@ > +# 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/. > + > +from __future__ import absolute_import, print_function, unicode_literals Please add a comment here explaining what this file does, and why. @@ +76,5 @@ > + > + return msg_list > + > +def is_ascii(s): > + return all(ord(c) < 0x80 for c in s) Can we add some tests for this? Does the build system have a test suite for these Python scripts? @@ +81,5 @@ > + > +def check_encoding(msg): > + if not is_ascii(msg['text']): > + error('Text should be in ASCII encoding', msg) > + return False I think it would be nicer to throw an exception here (and other places), instead of using return values. We can add a try-except to main() if needed.

(In reply to Jan de Mooij [:jandem] from comment #3) > Main suggestion: can we make the script work more like > check_spidermonkey_style.py? Basically grab the *.msg files from |hg > manifest| (or similar for git), so we don't need to change moz.build files, > and then process all of them. We have dom/base/domerr.msg and js/xpconnect/src/xpc.msg for non-JS error messages, but we could blacklist/ignore these. Another solution is to rename "our" msg files to *.jsmsg, or something.

*.msg files to list out all error messages is just a bad interface that we should get rid of. Format strings that all users have to get right, manual munging of the parameters into const char*, tying exception type to message, &. -- it's all just bad. So, polishing anything here isn't worth it, IMO. Frankly, my mental model of what should be done for this would be the equivalent of running |file| on the various files and checking for ASCII/UTF-8/whatever in the text output (except cross-platform, of course). There's nothing so wrong with the existing runtime (in debug) checking of parameters that it's worth beefing up build-time checking to deal with it, IMO.

Changed to follow check_spidermonkey_style.py style, and now it lists all files and checks .msg files, except domerr.msg and xpc.msg. then, removed the syntax check, and now it just checks it's in ascii encoding. now it's simple enough and I keep using returning True/False instead of throwing exception.

Comment on attachment 8795096 [details] [diff] [review] Check encoding of js.msg-like files. Review of attachment 8795096 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks! Please also request review from a build system peer: these changes are pretty trivial, but I don't think I'm allowed to review them. ::: config/check_js_msg_encoding.py @@ +3,5 @@ > +# 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 script checks encoding of the files that defines JSErrorFormatString. Nit: s/defines/define/, JSErrorFormatStrings (plural) @@ +13,5 @@ > + > +import sys > +from check_utils import get_all_toplevel_filenames > + > +scriptname = 'check_js_msg_files.py'; It doesn't match the name of this file anymore. You can use __file__ or sys.argv[0] to get the name/path of this script dynamically, maybe use os.path.basename to get only the filename.

Thank you for reviewing :) addressed review comments. gps, can you review build system part?

Comment on attachment 8795363 [details] [diff] [review] Check encoding of js.msg-like files. r=jandem Review of attachment 8795363 [details] [diff] [review]: ----------------------------------------------------------------- After you remove the Makefile.in bits, you don't need a build peer review :) ::: js/src/Makefile.in @@ +88,5 @@ > check-masm:: > (cd $(srcdir) && $(PYTHON) $(topsrcdir)/config/check_macroassembler_style.py); > > +check-js-msg:: > + (cd $(topsrcdir) && $(PYTHON) $(topsrcdir)/config/check_js_msg_encoding.py); I don't see this actually called. So please remove it.

(In reply to Gregory Szorc [:gps] from comment #9) > Comment on attachment 8795363 [details] [diff] [review] > Check encoding of js.msg-like files. r=jandem > > Review of attachment 8795363 [details] [diff] [review]: > ----------------------------------------------------------------- > > After you remove the Makefile.in bits, you don't need a build peer review :) > > ::: js/src/Makefile.in > @@ +88,5 @@ > > check-masm:: > > (cd $(srcdir) && $(PYTHON) $(topsrcdir)/config/check_macroassembler_style.py); > > > > +check-js-msg:: > > + (cd $(topsrcdir) && $(PYTHON) $(topsrcdir)/config/check_js_msg_encoding.py); > > I don't see this actually called. So please remove it. oops, I forgot to add it to `check` dependency.

Added check-js-msg to check. so this should be executed in SM automation jobs.

Comment on attachment 8795564 [details] [diff] [review] Check encoding of js.msg-like files. r=jandem Review of attachment 8795564 [details] [diff] [review]: ----------------------------------------------------------------- Cool. If all these style checks need is a source checkout and/or a minimal build system environment and are platform independent, we should convert them to TaskCluster tasks. They will complete quicker and won't cause build jobs to take longer to execute (because they won't make `make check` slower). And, there is some good tooling around linting coming down the pipeline (think automatically running lints as part of submitting to review). ahal or I can help you with the TaskCluster conversion. That can be done as a follow-up bug, of course.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf12f9b2f41c143b30e267f6750864a63d8fd204 Bug 1304970 - Check encoding of js.msg-like files. r=jandem,gps