Closed Bug 1418652 Opened 5 years ago Closed 5 years ago

Migrate comm-central configure options to moz.configure

Categories

(Thunderbird :: Build Config, enhancement, P3)

Product:
Thunderbird
Component:
Build Config
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 59.0

People

(Reporter: tomprince, Assigned: tomprince)

Details

Attachments

(4 files)

Bug 1418652: Migrate configure options to moz.configure;
59 bytes, text/x-review-board-request
Fallen
: review+
frg
: review+
chmanchester
: review+
Details
Bug 1418652: Remove non-existant comm-centrall configure option;
59 bytes, text/x-review-board-request
chmanchester
: review+
Fallen
: review+
Details
Bug 1418652: Migrate comm-central configure options to moz.configure;
59 bytes, text/x-review-board-request
chmanchester
: review+
Fallen
: review+
Details
build-fail.log
16.96 KB, text/x-log
Details 
      No description provided.
Attached file build-fail.log — 
suite build failure with all three patches in the tree.
Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

see previous log.
Attachment #8929754 - Flags: review?(frgrahl) → review-
Missing a - 

@depends_if("--disable-ldap",

retesting
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.
Attachment #8929754 - Flags: review- → feedback+
> 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 on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

https://reviewboard.mozilla.org/r/201008/#review206344
Attachment #8929754 - Flags: review?(philipp) → review+
Comment on attachment 8929755 [details]
Bug 1418652: Remove non-existant comm-centrall configure option;

https://reviewboard.mozilla.org/r/201014/#review206346
Attachment #8929755 - Flags: review+
Comment on attachment 8929756 [details]
Bug 1418652: Migrate comm-central configure options to moz.configure;

https://reviewboard.mozilla.org/r/201016/#review206348
Attachment #8929756 - Flags: review+
Comment on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Great thanks
Attachment #8929754 - Flags: review?(frgrahl) → review+
Attachment #8929754 - Flags: review?(frgrahl)
Attachment #8929754 - Flags: review?(core-build-config-reviews)
Attachment #8929754 - Flags: review?(cmanchester)
Attachment #8929754 - Flags: review+
Attachment #8929755 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8929756 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
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.
Attachment #8929754 - Flags: review?(cmanchester)
Comment on attachment 8929755 [details]
Bug 1418652: Remove non-existant comm-centrall configure option;

https://reviewboard.mozilla.org/r/201014/#review207036
Attachment #8929755 - Flags: review?(cmanchester) → review+
Comment on attachment 8929756 [details]
Bug 1418652: Migrate comm-central configure options to moz.configure;

https://reviewboard.mozilla.org/r/201016/#review207040
Attachment #8929756 - Flags: review?(cmanchester) → review+
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.
Attachment #8929754 - Flags: review?(core-build-config-reviews)
Pushed by mozilla@hocat.ca:
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 on attachment 8929754 [details]
Bug 1418652: Migrate configure options to moz.configure;

Didn't do a retest but looks good.
Attachment #8929754 - Flags: review?(frgrahl) → review+
https://hg.mozilla.org/mozilla-central/rev/5e6f9a196d67
https://hg.mozilla.org/mozilla-central/rev/ef9f30bea96d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/df5aec613e96
Migrate configure options to moz.configure; r=Fallen,frg,chmanchester
You need to log in before you can comment on or make changes to this bug.