Comment on attachment 8779126 [details] Bug 1292463 - Add MOZ_AUTOMATION to python configure. https://reviewboard.mozilla.org/r/70158/#review67896

Comment on attachment 8779127 [details] Bug 1292463 - Move --enable-warnings-as-errors to python configure. https://reviewboard.mozilla.org/r/70160/#review67898

Comment on attachment 8779128 [details] Bug 1292463 - Always set --enable-warnings-as-errors for MOZ_AUTOMATION builds. https://reviewboard.mozilla.org/r/70162/#review67900

Comment on attachment 8779129 [details] Bug 1292463 - Set MOZ_PGO subst/config from python configure. https://reviewboard.mozilla.org/r/70164/#review67902

Comment on attachment 8779130 [details] Bug 1292463 - Rename compilechecks.configure and test_header_checks.py. https://reviewboard.mozilla.org/r/70166/#review67904

Comment on attachment 8779127 [details] Bug 1292463 - Move --enable-warnings-as-errors to python configure. https://reviewboard.mozilla.org/r/70160/#review67906 ::: build/moz.configure/warnings.configure:7 (Diff revision 1) > +option('--enable-warnings-as-errors', env='MOZ_ENABLE_WARNINGS_AS_ERRORS', > + help='Enable treating warnings as errors') This should probably be js_option.

(In reply to Chris Manchester (:chmanchester) from comment #12) > Comment on attachment 8779127 [details] > Bug 1292463 - Move --enable-warnings-as-errors to python configure. > > https://reviewboard.mozilla.org/r/70160/#review67906 > > ::: build/moz.configure/warnings.configure:7 > (Diff revision 1) > > +option('--enable-warnings-as-errors', env='MOZ_ENABLE_WARNINGS_AS_ERRORS', > > + help='Enable treating warnings as errors') > > This should probably be js_option. Good point.

Comment on attachment 8779131 [details] Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure. https://reviewboard.mozilla.org/r/70168/#review67910 ::: build/moz.configure/compile-checks.configure:107 (Diff revision 1) > + if not when: > + when = depends('--help')(lambda _: True) Below it looks like you changed depends_when to be just like depends when `when` is None, so this can go away. ::: build/moz.configure/compile-checks.configure:131 (Diff revision 1) > + def result(c, when): > + if when and c.type in ('clang', 'gcc'): > + return True Is this intentionally colliding with `result` below, so that when the first check fails, we don't run the second one? If not, let's call this something else. If so... that's tricky enough to deserve a comment. ::: python/mozbuild/mozbuild/test/configure/test_compile_checks.py:265 (Diff revision 1) > checking for baz/foo-bar.h... no > checking for baz-quux/foo-bar.h... no > ''')) > > > +class TestWarningChecks(BaseCompileChecks): It might be good to have a test with an unsupported warning flag.

Comment on attachment 8779131 [details] Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure. https://reviewboard.mozilla.org/r/70168/#review67912

Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b446567ab8 Add MOZ_AUTOMATION to python configure. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e40bebd737 Move --enable-warnings-as-errors to python configure. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/305fce838f23 Always set --enable-warnings-as-errors for MOZ_AUTOMATION builds. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/7023c2aa407d Set MOZ_PGO subst/config from python configure. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e203c8eaa6 Rename compilechecks.configure and test_header_checks.py. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/0310ad696bd3 Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure. r=chmanchester

Erm. I happen to have started to land before seeing this review, and consequently didn't address those comments when landing. (In reply to Chris Manchester (:chmanchester) from comment #14) > Comment on attachment 8779131 [details] > Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure. > > https://reviewboard.mozilla.org/r/70168/#review67910 > > ::: build/moz.configure/compile-checks.configure:107 > (Diff revision 1) > > + if not when: > > + when = depends('--help')(lambda _: True) > > Below it looks like you changed depends_when to be just like depends when > `when` is None, so this can go away. Mmmmm I'm also touching that further in bug 1293579, let's go with a followup after that bug lands. > ::: build/moz.configure/compile-checks.configure:131 > (Diff revision 1) > > + def result(c, when): > > + if when and c.type in ('clang', 'gcc'): > > + return True > > Is this intentionally colliding with `result` below, so that when the first > check fails, we don't run the second one? If not, let's call this something > else. If so... that's tricky enough to deserve a comment. We have this pattern in several other places, none of which are commented. I'm not completely convinced this needs a comment. > ::: python/mozbuild/mozbuild/test/configure/test_compile_checks.py:265 > (Diff revision 1) > > checking for baz/foo-bar.h... no > > checking for baz-quux/foo-bar.h... no > > ''')) > > > > > > +class TestWarningChecks(BaseCompileChecks): > > It might be good to have a test with an unsupported warning flag. Will file a followup for this. I thought about it, but iirc, the test helper doesn't really support that in its current form, so it would require some fiddling.

(In reply to Mike Hommey [:glandium] from comment #17) > Mmmmm I'm also touching that further in bug 1293579, let's go with a > followup after that bug lands. Filed bug 1293874 > Will file a followup for this. I thought about it, but iirc, the test helper > doesn't really support that in its current form, so it would require some > fiddling. Filed bug 1293877

https://hg.mozilla.org/mozilla-central/rev/f5b446567ab8 https://hg.mozilla.org/mozilla-central/rev/b0e40bebd737 https://hg.mozilla.org/mozilla-central/rev/305fce838f23 https://hg.mozilla.org/mozilla-central/rev/7023c2aa407d https://hg.mozilla.org/mozilla-central/rev/f3e203c8eaa6 https://hg.mozilla.org/mozilla-central/rev/0310ad696bd3

Not sure what this needinfo is for. Everything looks in order here.