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)
Thunderbird
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: tomprince, Assigned: tomprince)
Details
Attachments
(4 files)
59 bytes,
text/x-review-board-request
|
Fallen
:
review+
frg
:
review+
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
Fallen
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
Fallen
:
review+
|
Details |
16.96 KB,
text/x-log
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
||
Comment 7•5 years ago
|
||
suite build failure with all three patches in the tree.
![]() |
||
Comment 8•5 years ago
|
||
Comment on attachment 8929754 [details] Bug 1418652: Migrate configure options to moz.configure; see previous log.
Attachment #8929754 -
Flags: review?(frgrahl) → review-
![]() |
||
Comment 9•5 years ago
|
||
Missing a - @depends_if("--disable-ldap", retesting
![]() |
||
Comment 10•5 years ago
|
||
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•5 years ago
|
||
> 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•5 years ago
|
||
mozreview-review |
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 14•5 years ago
|
||
mozreview-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 15•5 years ago
|
||
mozreview-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 16•5 years ago
|
||
Comment on attachment 8929754 [details] Bug 1418652: Migrate configure options to moz.configure; Great thanks
Attachment #8929754 -
Flags: review?(frgrahl) → review+
Updated•5 years ago
|
Attachment #8929754 -
Flags: review?(frgrahl)
Attachment #8929754 -
Flags: review?(core-build-config-reviews)
Attachment #8929754 -
Flags: review?(cmanchester)
Attachment #8929754 -
Flags: review+
Updated•5 years ago
|
Attachment #8929755 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Updated•5 years ago
|
Attachment #8929756 -
Flags: review?(core-build-config-reviews) → review?(cmanchester)
Comment 17•5 years ago
|
||
mozreview-review |
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 18•5 years ago
|
||
mozreview-review |
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 19•5 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Assignee | ||
Comment 21•5 years ago
|
||
mozreview-review-reply |
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.
Updated•5 years ago
|
Attachment #8929754 -
Flags: review?(core-build-config-reviews)
Comment 22•5 years ago
|
||
mozreview-review |
Comment on attachment 8929754 [details] Bug 1418652: Migrate configure options to moz.configure; https://reviewboard.mozilla.org/r/201008/#review207050
Attachment #8929754 -
Flags: review+
Comment 23•5 years ago
|
||
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 24•5 years ago
|
||
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+
Comment 25•5 years ago
|
||
bugherder |
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
Comment 26•5 years ago
|
||
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.
Description
•