Closed Bug 1612190 Opened 10 months ago Closed 10 months ago

Remove MOZ_CALENDAR build config flag

Categories

(Thunderbird :: General, task)

task
Not set
blocker

Tracking

(thunderbird74 fixed)

RESOLVED FIXED
Thunderbird 75.0
Tracking Status
thunderbird74 --- fixed

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(2 files, 3 obsolete files)

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.

(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?

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.

Attachment #9125881 - Flags: review?(geoff)

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

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.
Attachment #9125881 - Flags: review?(geoff) → feedback+
Blocks: 1615124

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.

Severity: normal → blocker
Status: NEW → ASSIGNED

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.

Attachment #9126289 - Flags: review+
Attachment #9126289 - Flags: approval-comm-beta?
Attachment #9125881 - Attachment is obsolete: true

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.

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.

Attachment #9126289 - Attachment is obsolete: true
Attachment #9126289 - Flags: approval-comm-beta?
Attachment #9126292 - Flags: review+
Attachment #9126292 - Flags: approval-comm-beta?
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.
Attachment #9126292 - Flags: review+

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

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.

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.

Oh, good thought on the Linux distros.

I.e. in mailnews/moz.configure:

+option('--enable-calendar', default=False,
+       help='Deprecated. Please remove it from your mozconfig.')
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)
Attachment #9126300 - Flags: approval-comm-beta?

(Sorry, I had uploaded the wrong patch)

Attachment #9126300 - Attachment is obsolete: true
Attachment #9126300 - Flags: review?(paul)
Attachment #9126300 - Flags: approval-comm-beta?
Attachment #9126301 - Flags: review?(paul)
Attachment #9126301 - Flags: approval-comm-beta?

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

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?

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.

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 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.
Attachment #9126301 - Flags: review?(paul) → review+

Pushed by paul@paulwmorris.com:
https://hg.mozilla.org/comm-central/rev/d450c0bd78b3
Remove MOZ_CALENDAR build config flag and related code. r=darktrojan,BenB

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 75.0

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.

Attachment #9126292 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9126301 - Flags: approval-comm-beta? → approval-comm-beta+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/591b7de150d5
Improve the DIRS logic in calendar moz.build files. r=darktrojan DONTBUILD
Target Milestone: Thunderbird 75.0 → Thunderbird 74.0

We keep the target milestone for when it first landed.

Target Milestone: Thunderbird 74.0 → Thunderbird 75.0
You need to log in before you can comment on or make changes to this bug.