Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 3 bugs)

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

3 years ago
No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8779126 [details]
Bug 1292463 - Add MOZ_AUTOMATION to python configure.

https://reviewboard.mozilla.org/r/70158/#review67896
Attachment #8779126 - Flags: review?(cmanchester) → review+

Comment 8

3 years ago
mozreview-review
Comment on attachment 8779127 [details]
Bug 1292463 - Move --enable-warnings-as-errors to python configure.

https://reviewboard.mozilla.org/r/70160/#review67898
Attachment #8779127 - Flags: review?(cmanchester) → review+

Comment 9

3 years ago
mozreview-review
Comment on attachment 8779128 [details]
Bug 1292463 - Always set --enable-warnings-as-errors for MOZ_AUTOMATION builds.

https://reviewboard.mozilla.org/r/70162/#review67900
Attachment #8779128 - Flags: review?(cmanchester) → review+

Comment 10

3 years ago
mozreview-review
Comment on attachment 8779129 [details]
Bug 1292463 - Set MOZ_PGO subst/config from python configure.

https://reviewboard.mozilla.org/r/70164/#review67902
Attachment #8779129 - Flags: review?(cmanchester) → review+

Comment 11

3 years ago
mozreview-review
Comment on attachment 8779130 [details]
Bug 1292463 - Rename compilechecks.configure and test_header_checks.py.

https://reviewboard.mozilla.org/r/70166/#review67904
Attachment #8779130 - Flags: review?(cmanchester) → review+

Comment 12

3 years ago
mozreview-review
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.
Assignee

Comment 13

3 years ago
(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

3 years ago
mozreview-review
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

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

https://reviewboard.mozilla.org/r/70168/#review67912
Attachment #8779131 - Flags: review?(cmanchester) → review+

Comment 16

3 years ago
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
Assignee

Comment 17

3 years ago
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.
Assignee

Updated

3 years ago
Flags: needinfo?(cmanchester)
Assignee

Updated

3 years ago
Blocks: 1293874
Assignee

Updated

3 years ago
Blocks: 1293877
Assignee

Comment 18

3 years ago
(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
Not sure what this needinfo is for. Everything looks in order here.
Flags: needinfo?(cmanchester)

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.