1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Add MOZ_AUTOMATION to python configure.

Review commit: https://reviewboard.mozilla.org/r/70158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70158/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Move --enable-warnings-as-errors to python configure.

Review commit: https://reviewboard.mozilla.org/r/70160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70160/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Always set --enable-warnings-as-errors for MOZ_AUTOMATION builds.

And remove it from mozconfigs.

Review commit: https://reviewboard.mozilla.org/r/70162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70162/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Set MOZ_PGO subst/config from python configure.

Review commit: https://reviewboard.mozilla.org/r/70164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70164/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Rename compilechecks.configure and test_header_checks.py.

to, respectively, compile-checks.configure and test_compile_checks.py.

Review commit: https://reviewboard.mozilla.org/r/70166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70166/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure.

Review commit: https://reviewboard.mozilla.org/r/70168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70168/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8779126 [details]
Bug 1292463 - Add MOZ_AUTOMATION to python configure.

https://reviewboard.mozilla.org/r/70158/#review67896

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8779127 [details]
Bug 1292463 - Move --enable-warnings-as-errors to python configure.

https://reviewboard.mozilla.org/r/70160/#review67898

Comment 9

[Chris Manchester (limited bugmail, email directly)]

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 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8779129 [details]
Bug 1292463 - Set MOZ_PGO subst/config from python configure.

https://reviewboard.mozilla.org/r/70164/#review67902

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8779130 [details]
Bug 1292463 - Rename compilechecks.configure and test_header_checks.py.

https://reviewboard.mozilla.org/r/70166/#review67904

Comment 12

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 13

[Mike Hommey [:glandium]]

(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 14

[Chris Manchester (limited bugmail, email directly)]

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 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8779131 [details]
Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure.

https://reviewboard.mozilla.org/r/70168/#review67912

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Mike Hommey [:glandium]]

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.

Comment 18

[Mike Hommey [:glandium]]

(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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Chris Manchester (limited bugmail, email directly)]

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