1728536 - --disable-maintenance-service fails to build

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Alex Kontos]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Firefox/91.0 Waterfox/91.0a1

Steps to reproduce:

Build mozilla-central with mingw-w64 or standard toolchain targetting Windows and ac_add_options --disable-maintenance-service.

Actual results:

toolkit/mozapps/update/updater/updater.cpp:3436:13: error: use of undeclared identifier 'IsSecureUpdateStatusSucceeded'
 8:21.41         if (IsSecureUpdateStatusSucceeded(updateStatusSucceeded) &&
 8:21.41             ^
 8:21.41 1 error generated.

Expected results:

Build should complete.

It appears that IsSecureUpdateStatusSucceeded is only declared when MOZ_MAINTENANCE_SERVICE is, here.

in Bug 1658711, the guards wrapping the if block in MOZ_MAINTENANCE_SERVICE were removed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Application Update' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Nick Alexander :nalexander [he/him]]

Alex: I'd review a patch to wrap the block in MOZ_MAINTENANCE_SERVICE, if you want to supply one.

Comment 3

[Alex Kontos]

Sorry, missed this! Yes, not a problem at all. Currently on holiday but will submit a patch when I’m back 👍

Comment 4

[Robin Steuber (she/her) [:bytesized]]

Setting S4 severity because this is a build problem that does not affect any of our standard releases.

Comment 5

[Alex Kontos]

Attachment: Patch to add correct guards

(In reply to Nick Alexander :nalexander [he/him] from comment #2)

Alex: I'd review a patch to wrap the block in MOZ_MAINTENANCE_SERVICE, if you want to supply one.

Here's the patch I've used (and haven't noticed any issues with so far).

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1728536 - Fix compilation error with `--disable-maintenance-service`. r?#application-update-reviewers

This was a regression from Bug 1658711.

Comment 7

[Nick Alexander :nalexander [he/him]]

Sorry for the long delay. I've submitted a modified version of this patch to Phab for review.

Comment 8

[Marco Castelluccio [:marco]]

Set release status flags based on info from the regressing bug 1658711

Comment 9

[Robin Steuber (she/her) [:bytesized]]

I finally have a moment to take a look at this. It looks to me like the correct thing to do is to make the relevant functions available without MOZ_MAINTENANCE_SERVICE being defined rather than to remove the code using those functions. I should have a patch up shortly.

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1728536 - Fix --disable-maintenance-service build capability r=nalexander!

Building without support for the maintenance service currently does not work because the definitions of EnvHasValue and IsSecureUpdateStatusSucceeded are ifdef'ed out despite being used. This patch fixes this by including those functions, even if compiling without support for the maintenance service.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28941c9d2525
Fix --disable-maintenance-service build capability r=nalexander,application-update-reviewers

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/28941c9d2525

Comment 13

[Pascal Chevrel:pascalc]

Is that something we would benefit from uplifting to beta or can I mark this bug as wontfix for 95?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Pascal Chevrel:pascalc from comment #13)

Is that something we would benefit from uplifting to beta or can I mark this bug as wontfix for 95?

This is fallout from a change in Firefox 81, so I see no reason to uplift. Thanks for checking!

Comment 15

[Alex Kontos]

Would it be possible to have this pulled in to ESR91 as well?

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Alex Kontos from comment #15)

Would it be possible to have this pulled in to ESR91 as well?

ESR91 makes more sense than uplift to Beta 94, IMO.

Comment 17

[Robin Steuber (she/her) [:bytesized]]

(In reply to Alex Kontos from comment #15)

Would it be possible to have this pulled in to ESR91 as well?

Hmm. I've spent a little time thinking about this. The user impact seems low to non-existent, depending on how you quantify it. But the risk is also low to non-existent. I think it's reasonable to uplift to ESR.

Comment 18

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 9248111 [details]
Bug 1728536 - Fix --disable-maintenance-service build capability r=nalexander!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch really doesn't affect Firefox users, because we don't build Firefox with this build flag. But not having the patch does make it more difficult for Firefox forks that use this build flag to incorporate upstream changes and build successfully.
• User impact if declined: Build bustage in certain non-default configurations.
• Fix Landed on Version: 96
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch strictly changes #ifdefs that determine what functions ought to be defined. The changes should only really be relevant when the --disable-maintenance-service flag is specified. So this patch should have no effect on official Mozilla builds, which do not use that flag.
• String or UUID changes made by this patch: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9248111 [details]
Bug 1728536 - Fix --disable-maintenance-service build capability r=nalexander!

NPOTB for Firefox, makes life easier for downstream distros. Approved for 91.4esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9248111 [details]
Bug 1728536 - Fix --disable-maintenance-service build capability r=nalexander!

Actually this needs rebasing for ESR91.

Comment 21

[Robin Steuber (she/her) [:bytesized]]

Attachment: esr91.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch really doesn't affect Firefox users, because we don't build Firefox with this build flag. But not having the patch does make it more difficult for Firefox forks that use this build flag to incorporate upstream changes and build successfully.
• User impact if declined: Build bustage in certain non-default configurations.
• Fix Landed on Version: 96
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch strictly changes #ifdefs that determine what functions ought to be defined. The changes should only really be relevant when the --disable-maintenance-service flag is specified. So this patch should have no effect on official Mozilla builds, which do not use that flag.
• String or UUID changes made by this patch: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9250309 [details] [diff] [review]
esr91.patch

Approved for 91.4esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/dd5cd481c039

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/882637f560a4