1152997 - Cleanup app update build to exclude android

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Android no longer uses app update for nightly builds but when they changed very little was cleaned up. This bug should make it so Android no longer includes app update files, etc.

Note: there is also bug 1152634 to get Android to no longer re-purpose the MOZ_UPDATER define.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Brian, I'll get a build peer to review this but can I get your feedback on this? Android used to use app update for nightly builds and no longer use it. They re-purposed the MOZ_UPDATER define so until bug 1152634 is fixed I can't just use MOZ_UPDATER to determine whether app update should be included in the build but this gets things most of the way there. I'm especially curious whether you can think of a better way to accomplish what I changed in toolkit/moz.build or if you think that is fine the way it is.

I've run these through try and they look fine. I've also built without app update without any problems.

Comment 2

[Robert Strong (they/them - no direct email)]

Try push with just about everything just in case
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b63cd846f484

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 8590523 [details] [diff] [review]
patch rev1

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

This seems cleaner to me, definitely needs a build peer though.

::: toolkit/moz.build
@@ +28,2 @@
>  
> +if CONFIG['MOZ_MAINTENANCE_SERVICE'] and CONFIG['OS_ARCH'] == 'WINNT':

I think the and here is redundant, we'll already give an error in configure.in before this
"Can only build with --enable-maintenance-service with a Windows target"

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8590960 [details] [diff] [review]
patch rev2

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

::: toolkit/components/moz.build
@@ +47,5 @@
>      'startup',
>      'statusfilter',
>      'telemetry',
>      'thumbnails',
> +    'timermanager',

It seems to me separating this is unnecessary.

::: toolkit/mozapps/update/common-standalone/moz.build
@@ +10,5 @@
>  
>  if CONFIG['OS_ARCH'] == 'WINNT':
>      USE_STATIC_LIBS = True
> +
> +FAIL_ON_WARNINGS = True

FAIL_ON_WARNINGS is unrelated to the changes here. That should go in its own patch if not its own bug.

Comment 6

[Robert Strong (they/them - no direct email)]

Glandium, the history of nsIUpdateTimerManager is that it was initially created for app update so we could keep track of long running timers across browser sessions. It has since had several other consumers that have nothing to do with app update and is used by all Mozilla based apps that I know of. App update on the other hand is not used by all Mozilla based apps and there are Firefox and other distributions that don't use app update at all as you know. I'd like to separate the directories to simplify the building of both of these without a bunch of ifdef's.

Can you suggest a better way to accomplish this?

Comment 7

[Robert Strong (they/them - no direct email)]

Filed bug 1154179 for adding FAIL_ON_WARNINGS to app update

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8590960 [details] [diff] [review]
patch rev2

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

considering the context added in comment 6, r+.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch - updated to comments

Comment 10

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/ce046fd1be02

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce046fd1be02

Comment 12

[Philip Chee]

==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file or one of the files it includes:

    c:/t1/hg/comm-central/mozilla/toolkit/mozapps/update/updater/moz.build

The error occurred when validating the result of the execution. The reported error is:

    USE_LIBS contains "updatecommon-standalone", which does not match any LIBRARY_NAME in the tree.

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to Philip Chee from comment #12)
> ==============================
> ERROR PROCESSING MOZBUILD FILE
> ==============================
> 
> The error occurred while processing the following file or one of the files
> it includes:
> 
>     c:/t1/hg/comm-central/mozilla/toolkit/mozapps/update/updater/moz.build
> 
> The error occurred when validating the result of the execution. The reported
> error is:
> 
>     USE_LIBS contains "updatecommon-standalone", which does not match any
> LIBRARY_NAME in the tree.
Which application? SeaMonkey? Did you try clobbering?

Comment 14

[Robert Strong (they/them - no direct email)]

For comment #13

Comment 15

[Robert Strong (they/them - no direct email)]

Appears to be SeaMonkey. I create a followup patch

Comment 16

[Philip Chee]

Attachment: WIP SeaMonkey bustage fix.

This WIP patch allows SeaMonkey builds to run. Still building. Will report result RSN

Comment 17

[Robert Strong (they/them - no direct email)]

Comment on attachment 8593027 [details] [diff] [review]
WIP SeaMonkey bustage fix.

># HG changeset patch
># Parent  3080262f9b50e42f51003c3b4fff1cfea6e5dc7c
>Bug 1152997
>
>diff --git a/toolkit/moz.build b/toolkit/moz.build
>--- a/toolkit/moz.build
>+++ b/toolkit/moz.build
>@@ -21,22 +21,26 @@ DIRS += [
>     'profile',
>     'themes',
>     'webapps',
> ]
> 
> if CONFIG['MOZ_UPDATER'] and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
>     DIRS += ['mozapps/update']
> 
>-if CONFIG['MOZ_MAINTENANCE_SERVICE']:
>+if CONFIG['MOZ_UPDATER'] or CONFIG['MOZ_MAINTENANCE_SERVICE']:
This also needs CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android'

> # Including mozapps/update/common-standalone allows the maintenance service
> # to be built so the maintenance service can be used for things other than
> # updating applications.
>     DIRS += [
>         'mozapps/update/common-standalone',
>+    ]
>+
>+if CONFIG['MOZ_MAINTENANCE_SERVICE']:
>+    DIRS += [
>         'components/maintenanceservice'
>     ]
> 
I'll r+ a new patch with the above after it has built successfully

Comment 18

[Robert Strong (they/them - no direct email)]

Specifically, android defines MOZ_UPDATER so it needs to be excluded from
if CONFIG['MOZ_UPDATER']

Comment 19

[Robert Strong (they/them - no direct email)]

The patch works for me with the following change mentioned in comment #17

>-if CONFIG['MOZ_MAINTENANCE_SERVICE']:
>+if CONFIG['MOZ_MAINTENANCE_SERVICE'] or CONFIG['MOZ_UPDATER'] and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':

All tests passed as well. I'll r+ a new patch when it is ready and thanks!

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: patch

I'll try to get this landed first thing in the morning.

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

I think this is a better approach

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59da9c60f8ae

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 8593403 [details] [diff] [review]
patch rev2

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

Thanks, r+ assuming it passes try.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e5010e6fca6a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5010e6fca6a

Comment 25

[[e-mail]]

(In reply to Robert Strong from comment #13)
> Which application? SeaMonkey? Did you try clobbering?

FYI I also saw it in my local Thunderbird build but I didn't try clobbering because your patch fixed my build.