Cleanup app update build to exclude android

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla40
x86_64
Windows 8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Posted patch patch rev1 (obsolete) — Splinter Review
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.
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Attachment #8590523 - Flags: feedback?(netzen)
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"
Attachment #8590523 - Flags: feedback?(netzen) → feedback+
Posted patch patch rev2 (obsolete) — Splinter Review
Attachment #8590523 - Attachment is obsolete: true
Attachment #8590960 - Flags: review?(mh+mozilla)
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.
Attachment #8590960 - Flags: review?(mh+mozilla) → feedback+
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?
Flags: needinfo?(mh+mozilla)
Filed bug 1154179 for adding FAIL_ON_WARNINGS to app update
Comment on attachment 8590960 [details] [diff] [review]
patch rev2

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

considering the context added in comment 6, r+.
Attachment #8590960 - Flags: feedback+ → review+
Flags: needinfo?(mh+mozilla)
Attachment #8592111 - Flags: review?(dtownsend)
Attachment #8592111 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/ce046fd1be02
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
==============================
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.
Flags: needinfo?(robert.strong.bugs)
(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?
Flags: needinfo?(robert.strong.bugs)
For comment #13
Flags: needinfo?(philip.chee)
Appears to be SeaMonkey. I create a followup patch
Flags: needinfo?(philip.chee)
Posted patch WIP SeaMonkey bustage fix. (obsolete) — Splinter Review
This WIP patch allows SeaMonkey builds to run. Still building. Will report result RSN
Attachment #8593027 - Flags: feedback?(robert.strong.bugs)
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
Specifically, android defines MOZ_UPDATER so it needs to be excluded from
if CONFIG['MOZ_UPDATER']
Attachment #8593027 - Flags: feedback?(robert.strong.bugs)
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!
Posted patch patch (obsolete) — Splinter Review
I'll try to get this landed first thing in the morning.
Attachment #8593027 - Attachment is obsolete: true
Attachment #8593291 - Flags: review?(netzen)
Attachment #8593291 - Flags: review?(netzen) → review+
Posted patch patch rev2Splinter Review
I think this is a better approach

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59da9c60f8ae
Attachment #8593291 - Attachment is obsolete: true
Attachment #8593403 - Flags: review?(netzen)
Comment on attachment 8593403 [details] [diff] [review]
patch rev2

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

Thanks, r+ assuming it passes try.
Attachment #8593403 - Flags: review?(netzen) → review+
(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.
Duplicate of this bug: 1155620
You need to log in before you can comment on or make changes to this bug.