1612190 - Remove MOZ_CALENDAR build config flag

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Paul Morris [:pmorris]]

Once Lightning is integrated into TB (in bug 1608610), there shouldn't be a need for the MOZ_CALENDAR build flag and related code. This bug is for removing them.

Comment 1

[Worcester12345]

(In reply to Paul Morris [:pmorris] from comment #0)

Once Lightning is integrated into TB (in bug 1608610), there shouldn't be a need for the MOZ_CALENDAR build flag and related code. This bug is for removing them.

Good?

Comment 2

[Paul Morris [:pmorris]]

Attachment: remove-moz-calendar-build-flag-0.patch

I think this is everything to be removed for this, but I'm not super familiar with this part of the code.

After this change developers will need to remove this line from their mozconfig file:
ac_add_options --enable-calendar

I've started a local build to test this, but there's some bustage currently so I'm going to go ahead and post this. Feel free to cancel the review until I can confirm this works if you like.

Comment 3

[Paul Morris [:pmorris]]

I spoke too soon. My local build succeeded, so this is ready for review.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9125881 [details] [diff] [review]
remove-moz-calendar-build-flag-0.patch

Review of attachment 9125881 [details] [diff] [review]:
-----------------------------------------------------------------

I expected there to be more. You did miss taking the actual line from the mozconfig at mail/config/mozconfigs/common.

While we're looking at this I'd like to untwist some of the DIRS logic. Have mail/app.mozbuild point to calendar instead of calendar/lightning and move the directives that reference ".." into the right moz.build. It seems to work, although I did have to take out the CONFIGURE_SUBST_FILES line from calendar/moz.build (that was wrong anyway). Confused yet?

::: mail/app.mozbuild
@@ +18,5 @@
>      DIRS += ['/%s/extensions' % CONFIG['mozreltopsrcdir']]
>  
>  DIRS += ['/%s' % CONFIG['MOZ_BRANDING_DIRECTORY']]
>  
> +DIRS += ['/%s/calendar/lightning' % CONFIG['commreltopsrcdir']]

This can be combined with the rows above and below it.

Comment 5

[Ben Bucksch (:BenB)]

Please land this (or something like it) ASAP (as in, right now), because the normal Thunderbird build is broken right now, see bug 1615124.

This is now a tree bustage fix.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1612190-remove-moz-calendar-build-flag-1.patch

Okay, this is the same patch with my requested changes, I'll land it when the tree opens again.

We should also land this on beta, since things are broken there too without the --enable-calendar option.

Comment 7

[Paul Morris [:pmorris]]

Here's a try run I did earlier today with two patches for this bug. It doesn't look great, and I haven't had time to look into it further. I was about to upload the first patch from it, when I saw Geoff beat me to it. I just tried to do a try run with just the first patch (but the tree is closed).
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e6f3d2e7f629338795f4a31714770b011fdc6672

You did miss taking the actual line from the mozconfig at mail/config/mozconfigs/common.

(facepalm) Would you believe I had actually removed that line but then neglected to save the file. Yesterday was not my day.

Comment 8

[Paul Morris [:pmorris]]

Attachment: 1612190-remove-moz-calendar-build-flag-2.patch

There were a couple more lines to remove in package-manifest.in that got left out of my initial patch. This removes them as well.

Comment 9

[Ben Bucksch (:BenB)]

Comment on attachment 9126292 [details] [diff] [review]
1612190-remove-moz-calendar-build-flag-2.patch

Review of attachment 9126292 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app.mozbuild
@@ +20,3 @@
>  DIRS += [
> +    '/%s' % CONFIG['MOZ_BRANDING_DIRECTORY']
> +    '/%s/calendar/lightning' % CONFIG['commreltopsrcdir']

2 commas missing at the end of the lines.

With this change, this fixes bug 1615124. Tested with clobber build and running TB.

r=BenB with this change.

Comment 10

[Ben Bucksch (:BenB)]

:pmorris, can you land this patch today, with the 2 commas added, please?

Comment 11

[Ben Bucksch (:BenB)]

FWIW, removing this flag this will break the build for Linux distros that have this flag set. I checked Debian and Gentoo, and they both use the flag. If you want to be nice, you can make another followup patch, where you re-introduce the build flag, but it does nothing, just outputs a warning that the flag should be removed from mozconfig.

Comment 12

[Paul Morris [:pmorris]]

Thanks BenB. Will do. I had fixed those 2 missing commas earlier today, but (unintentionally) only in the second DIRS patch (not uploaded to this bug yet because of a failing try run with it). But they need to go in the MOZ_CALENDAR patch. I have a local build and a try run with the fixed patch (with commas added) going now. Should have it landed soon.

Comment 13

[Paul Morris [:pmorris]]

Oh, good thought on the Linux distros.

Comment 14

[Ben Bucksch (:BenB)]

I.e. in mailnews/moz.configure:

+option('--enable-calendar', default=False,
+       help='Deprecated. Please remove it from your mozconfig.')

Comment 15

[Ben Bucksch (:BenB)]

Attachment: Same patch as v2, but commas fixed, and let deprecated flag not break build

Comment 16

[Ben Bucksch (:BenB)]

Comment on attachment 9126300 [details] [diff] [review]
Same patch as v2, but commas fixed, and let deprecated flag not break build

[Approval Request Comment]
Regression caused by (bug #): 1612190
User impact if declined:
No patch: Build breaks for developers without --enable-calendar
Only patch v2: Build breaks for developers with --enable-calendar, and for Linux distros (Debian, Gentoo etc.)
Testing completed (on c-c, etc.): configure run, not a complete build yet
Risk to taking this patch (and alternatives if risky): build breaks (but it's already broken)

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Same patch as v2, but commas fixed, and deprecated flag don't break build

(Sorry, I had uploaded the wrong patch)

Comment 18

[Ben Bucksch (:BenB)]

The latest patch builds and runs TB, even with --enable-calendar set.

Comment 19

[Paul Morris [:pmorris]]

Comment on attachment 9126301 [details] [diff] [review]
Same patch as v2, but commas fixed, and deprecated flag don't break build

Review of attachment 9126301 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding the deprecated --enable-calendar part.  I had one question on that.

::: mailnews/moz.configure
@@ +63,5 @@
> +
> +
> +@depends_if('--enable-calendar')
> +def calendar_deprecated(arg):
> +    return True

Hm, did you mean `calendar_deprecated` here?  I'm not sure what this is for.  Like, is it supposed to be a replacement for the `def calendar(arg)` or something different?

Comment 20

[Ben Bucksch (:BenB)]

calendar_deprecated` ... I'm not sure what this is for.

configure error-ed out without a @depends_if that uses the option, so I added this.
Later, we might also print a message there, but that's polish, which can happen later.

Comment 21

[Ben Bucksch (:BenB)]

I did 2 clobber builds with the current patch v4, one each way, to confirm that the build works with --enable-calendar and without.
From my side, and as far as I can see, this is ready to land.

Comment 22

[Paul Morris [:pmorris]]

Comment on attachment 9126301 [details] [diff] [review]
Same patch as v2, but commas fixed, and deprecated flag don't break build

Review of attachment 9126301 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, the additions look good, and will be good to have for the Linux distributions.  The try run with the earlier version of the patch (without the --enable-calendar deprecation parts) looks good: 
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a78fab81d5259506fc0be9d05d70d85f663b4175
I'm going to go ahead and land the patch on c-c.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d450c0bd78b3
Remove MOZ_CALENDAR build config flag and related code. r=darktrojan,BenB

Comment 24

[Paul Morris [:pmorris]]

This should land for TB beta 74 if possible. Without the patch, builds of beta 74 will be broken unless the --enable-calendar option set. That's my understanding, and also what Ben's reported.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/591b7de150d5
Improve the DIRS logic in calendar moz.build files. r=darktrojan DONTBUILD

Comment 26

[Rob Lemley [:rjl]]

Thunderbird 74.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/498037e1ef0f
Thunderbird 74.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/d21a540ff744

Comment 27

[Magnus Melin [:mkmelin]]

We keep the target milestone for when it first landed.