1418652 - Migrate comm-central configure options to moz.configure

Status: RESOLVED FIXED

(Thunderbird :: Build Config, enhancement, P3)

Comment 1

[Tom Prince [:tomprince]]

Attachment: Bug 1418652: Migrate configure options to moz.configure;

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

Comment 2

[Tom Prince [:tomprince]]

Attachment: Bug 1418652: Remove non-existant comm-centrall configure option;

It was removed in Bug 1318966.

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

Comment 3

[Tom Prince [:tomprince]]

Attachment: Bug 1418652: Migrate comm-central configure options to moz.configure;

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

Comment 4

[Tom Prince [:tomprince]]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

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

Comment 5

[Tom Prince [:tomprince]]

Comment on attachment 8929755 [details]
Bug 1418652: Remove non-existant comm-centrall configure option;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201014/diff/1-2/

Comment 6

[Tom Prince [:tomprince]]

Comment on attachment 8929756 [details]
Bug 1418652: Migrate comm-central configure options to moz.configure;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201016/diff/1-2/

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: build-fail.log

suite build failure with all three patches in the tree.

Comment 8

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

see previous log.

Comment 9

[Frank-Rainer Grahl (:frg)]

Missing a - 

@depends_if("--disable-ldap",

retesting

Comment 10

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

> mailnews/moz.configure

+@depends_if("-disable-ldap",
needs to be
+option("--disable-ldap",

NIT: The option and depends_if statements also use single and double quotes inconsistently.

And it was possible to enable calendar by just using MOZ_CALENDAR=1 in confvars.sh. We recently added this. This is no longer honored and should be fixed.

Otherwise works fine with the one fix.

Comment 11

[Tom Prince [:tomprince]]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201008/diff/1-2/

Comment 12

[Tom Prince [:tomprince]]

> And it was possible to enable calendar by just using MOZ_CALENDAR=1 in confvars.sh. We recently added this. This is no longer honored and should be fixed.

The equivalent to this is to add `imply_options('--enable-calendar', True)` in moz.configure. I've gone and moved that setting over.

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

https://reviewboard.mozilla.org/r/201008/#review206344

Comment 14

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8929755 [details]
Bug 1418652: Remove non-existant comm-centrall configure option;

https://reviewboard.mozilla.org/r/201014/#review206346

Comment 15

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8929756 [details]
Bug 1418652: Migrate comm-central configure options to moz.configure;

https://reviewboard.mozilla.org/r/201016/#review206348

Comment 16

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Great thanks

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

https://reviewboard.mozilla.org/r/201008/#review206602

::: mailnews/moz.configure:32
(Diff revision 2)
> +@depends_if('--enable-ldap')
> +def ldap(mapi):
> +    return True
> +
> +set_config('MOZ_LDAP_XPCOM', ldap)
> +set_define('MOZ_LDAP_XPCOM', ldap)

`set_define` corresponds to `AC_DEFINE`. Based on mail/configure.in, for this and MOZ_CALENDAR it appears you don't need `set_define`.

::: mailnews/moz.configure:38
(Diff revision 2)
> +option('--disable-mapi',
> +       help='Disable MAPI support')
> +
> +@depends_if('--enable-mapi', when=target_is_windows)
> +def mapi_support(mapi):
> +    return True

This doesn't look quite right, the value of `mapi_support` doesn't vary based on the option, so passing `--disable-mapi` will not have an effect.

I think, based on reading configure.in, we want this to be an option that only exists when building windows (so put the `when=target_is_windows` in the option definition).

::: suite/moz.configure:11
(Diff revision 2)
> +include('../mailnews/moz.configure')
> +imply_option('--enable-calendar', True)

I think `imply_option`s are intended to go before the option definition, but this might work too.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8929755 [details]
Bug 1418652: Remove non-existant comm-centrall configure option;

https://reviewboard.mozilla.org/r/201014/#review207036

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8929756 [details]
Bug 1418652: Migrate comm-central configure options to moz.configure;

https://reviewboard.mozilla.org/r/201016/#review207040

Comment 20

[Tom Prince [:tomprince]]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201008/diff/2-3/

Comment 21

[Tom Prince [:tomprince]]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

https://reviewboard.mozilla.org/r/201008/#review206602

> `set_define` corresponds to `AC_DEFINE`. Based on mail/configure.in, for this and MOZ_CALENDAR it appears you don't need `set_define`.

Yes.

> This doesn't look quite right, the value of `mapi_support` doesn't vary based on the option, so passing `--disable-mapi` will not have an effect.
> 
> I think, based on reading configure.in, we want this to be an option that only exists when building windows (so put the `when=target_is_windows` in the option definition).

My understanding is that `depends_if` is only evaluated if the arguments to it are non-null.

You are correct about where `when` should be, and I've moved it to the option.

> I think `imply_option`s are intended to go before the option definition, but this might work too.

Huh. That does appear to be how `imply_option` is documented, but it looks like the implementation doesn't currently care. In any case, I've moved the include to the end.

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

https://reviewboard.mozilla.org/r/201008/#review207050

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e6f9a196d67
Remove non-existant comm-centrall configure option; r=chmanchester,Fallen
https://hg.mozilla.org/integration/autoland/rev/ef9f30bea96d
Migrate comm-central configure options to moz.configure; r=chmanchester,Fallen

Comment 24

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Didn't do a retest but looks good.

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5e6f9a196d67
https://hg.mozilla.org/mozilla-central/rev/ef9f30bea96d

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/df5aec613e96
Migrate configure options to moz.configure; r=Fallen,frg,chmanchester