armv6 builds of Native Fennec need to use unique %BUILD_TARGET% in update queries

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nthomas, Unassigned)

Tracking

Trunk
mozilla16
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
I'm assuming that nightly updates for Nightly/Aurora are wanted and a priority for the v6 builds, so adjust as necessary if that's not true.

Right now the v6 and v7 arm builds query AUS with the same expansion for %BUILD_TARGET%, so they're indistinguishable and won't be able to serve them different files. Eg, here's what Apache sees if I point a v6 dep build at
 GET /update/4/Fennec/16.0a1/20120621135913/Android_arm-eabi-gcc3/en-US/default/Linux%202.6.36.3/default/default/16.0a1/update.xml?force=1
and here's non-v6 nightly
 GET /update/4/Fennec/16.0a1/20120621053048/Android_arm-eabi-gcc3/en-US/nightly/Linux%202.6.36.3/default/default/16.0a1/update.xml?force=1 HTTP/1.1

A value of 'Android_armv6-eabi-gcc3' was suggested in bug 723946 for the v6 builds. This would need to be baked in at build-time so that it can be used at
 http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2639
or at least some sort of override like 
 http://mxr.mozilla.org/mozilla-central/source/mobile/xul/app/mobile.js#499
does for xul.
(Reporter)

Updated

5 years ago
Blocks: 723946
What about using MOZ_ARCH instead of CPU_ARCH here: https://mxr.mozilla.org/mozilla-central/source/configure.in#3821
(In reply to Marco Castelluccio from comment #1)
> What about using MOZ_ARCH instead of CPU_ARCH here:
> https://mxr.mozilla.org/mozilla-central/source/configure.in#3821

MOZ_ARCH is not always set. In fact, it's only set for armv6 builds.
Can we use MOZ_ARCH if MOZ_ARCH is set and CPU_ARCH otherwise?
(In reply to Mike Hommey [:glandium] from comment #2)
> (In reply to Marco Castelluccio from comment #1)
> > What about using MOZ_ARCH instead of CPU_ARCH here:
> > https://mxr.mozilla.org/mozilla-central/source/configure.in#3821
> 
> MOZ_ARCH is not always set. In fact, it's only set for armv6 builds.

Ah, it's also set on armv7 Android builds, and iOS builds, in which case it's "toolchain-default".

Using MOZ_ARCH there would also change the meaning of the XPCOM ABI, and that would need to be very carefully thought.
(In reply to Mike Hommey [:glandium] from comment #4)
> Using MOZ_ARCH there would also change the meaning of the XPCOM ABI, and
> that would need to be very carefully thought.

We could set MOZ_ARCH as "armv6" when we're building for armv6 and as CPU_ARCH when building for other platforms. And then use MOZ_ARCH there.

Or we could set CPU_ARCH differently for armv6 and armv7 (this would need some changes in the codebase, but it's in my opinion better).
(In reply to Marco Castelluccio from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Using MOZ_ARCH there would also change the meaning of the XPCOM ABI, and
> > that would need to be very carefully thought.
> 
> We could set MOZ_ARCH as "armv6" when we're building for armv6 and as
> CPU_ARCH when building for other platforms. And then use MOZ_ARCH there.

That would break the meaning and use of MOZ_ARCH.

> Or we could set CPU_ARCH differently for armv6 and armv7 (this would need
> some changes in the codebase, but it's in my opinion better).

Same remark as in comment 4 about XPCOM ABI.
Can we wedge MOZ_PKG_SPECIAL in there, since that's what we're using for package names?
Created attachment 636810 [details] [diff] [review]
Patch

(In reply to Ted Mielczarek [:ted] from comment #7)
> Can we wedge MOZ_PKG_SPECIAL in there, since that's what we're using for
> package names?

Is this acceptable?
Comment on attachment 636810 [details] [diff] [review]
Patch

This looks like a plausible solution. However, MOZ_PKG_SPECIAL isn't in $(DEFINES) by default, so you'd also have to edit mobile/android/app/Makefile.in and add the value to DEFINES if it is set.
Created attachment 636891 [details] [diff] [review]
Change update url in mobile.js using MOZ_PKG_SPECIAL
Attachment #636810 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
Traditionally we've made these sorts of changes at a lower level, so that they affect blocklist queries too. Is that possible here ?
I'm not really sure what the right way to fix this is. CCing some people who know the update/blocklist code.
Attachment #636891 - Flags: review?(nrthomas)
Attachment #636891 - Flags: review?(mark.finkle)
(In reply to Ted Mielczarek [:ted] from comment #12)
> I'm not really sure what the right way to fix this is. CCing some people who
> know the update/blocklist code.
If there is a new param that needs to be sent I'm ok with adding a new replace param to the app.update.url preference which would then be replaced by the the update service though since this is a one-off which is only needed for this platform it would be simpler to just hardcode it in the preference itself as the patch above does.

As for where and how the param is placed in the url, AUS and metrics are both consumers of this so it really needs input from nthomas and the metrics team should be notified as well.
(In reply to Nick Thomas [:nthomas] from comment #11)
> Traditionally we've made these sorts of changes at a lower level, so that
> they affect blocklist queries too. Is that possible here ?

We always send the same blocklist file, regardless of the params in the URL. Thosee extra params in the blocklist URL are mostly (entirely?) just for stats. Though... we may want to get blocklist stats on armv6, in which case see bug 763628.
Comment on attachment 636891 [details] [diff] [review]
Change update url in mobile.js using MOZ_PKG_SPECIAL

Looks like we have some consensus on the approach. The patch looks good to me.
Attachment #636891 - Flags: review?(mark.finkle) → review+
Attachment #636891 - Flags: review?(nrthomas) → review?(robert.bugzilla)
Comment on attachment 636891 [details] [diff] [review]
Change update url in mobile.js using MOZ_PKG_SPECIAL

I'm fine with this as long as metrics and nthomas are as well (comment #13)
Attachment #636891 - Flags: review?(robert.bugzilla) → review+
(Reporter)

Comment 17

5 years ago
While this approach can get us updates on Nightly/Aurora, please note we will no visibility to ADIs if we only change the AUS ping (dunno about Google Play if v6 builds get to beta/release), hence the query about doing this at a lower level. I briefly looked at the MOZ_ARCH and CPU_ARCH situation in the build system and it seems quite complicated, so I can see why you might prefer preprocessing the prefs file.

Comment 18

5 years ago
I will email the metrics team to see who can weigh in.
Changing the value that appears in the platform field of blocklist is fine.  That field is used for ad-hoc analysis, and people looking at the data are expected to be able to make sense of what it contains.  Metrics is fine with this change.
(Reporter)

Comment 20

5 years ago
Presumably 'platform' is the metrics jargon for %BUILD_TARGET% in
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#209 ?

209 pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/3/%APP_ID%/%APP_VERSION%/%PRODUCT%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PING_COUNT%/%TOTAL_PING_COUNT%/%DAYS_SINCE_LAST_PING%/");

Comment 21

5 years ago
Correct. s/Platform/Build Target/
https://hg.mozilla.org/mozilla-central/rev/b95c12c7e6d1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Reporter)

Updated

5 years ago
Depends on: 772045
(Reporter)

Comment 23

5 years ago
The build target for the v7 builds was regressed by this change, changing from Android_arm-eabi-gcc3 to Android_arm-eabi-gcc3-.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 24

5 years ago
Seems to me the unconditional change to DEFINES is the issue here, and the patch needs fixing up.
So MOZ_PKG_SPECIAL is always defined and therefore we never use the #else block
Comment on attachment 636891 [details] [diff] [review]
Change update url in mobile.js using MOZ_PKG_SPECIAL

># HG changeset patch
># Parent 5c07a681371d46d1bc86af397b6d712fdd870e2b
># User Marco Castelluccio <mar.castelluccio@studenti.unina.it>
>Bug 767864 - armv6 builds of Native Fennec need to use unique %BUILD_TARGET% in update queries
>
>diff --git a/mobile/android/app/Makefile.in b/mobile/android/app/Makefile.in
>--- a/mobile/android/app/Makefile.in
>+++ b/mobile/android/app/Makefile.in
>@@ -58,16 +58,17 @@ NSDISTMODE = copy
> include $(topsrcdir)/config/rules.mk
> 
> APP_ICON = mobile
> 
> DEFINES += \
>   -DAPP_NAME=$(MOZ_APP_NAME) \
>   -DAPP_VERSION=$(MOZ_APP_VERSION) \
>   -DMOZ_UPDATER=$(MOZ_UPDATER) \
>+  -DMOZ_PKG_SPECIAL=$(MOZ_PKG_SPECIAL) \
>   $(NULL)
> 
> ifeq ($(OS_ARCH),WINNT)
> REDIT_PATH = $(LIBXUL_DIST)/bin
> endif

Easy solution here would be to remove that added line THEN:

ifneq (,$(MOZ_PKG_SPECIAL))
DEFINES += -DMOZ_PKG_SPECIAL=$(MOZ_PKG_SPECIAL)
endif

And no need to modify the prefs.js file any more than was already done.
Created attachment 640504 [details] [diff] [review]
fix armv7

This patch conditionally adds MOZ_PKG_SPECIAL to DEFINES and ARMv7 works again. I see "%BUILD_TARGET%", not "%BUILD_TARGET%-" in mobile.js

I did not test that this still works correctly for ARMv6, but based on other makefiles I assume it will. Pushing to try for verification...
Attachment #640504 - Flags: review?(nrthomas)
Crud. No ARMv6 builds on Try.
(Reporter)

Comment 29

5 years ago
I see prior builds name 'Android Armv6 try build', which seem to get run if you have '-p all' in the try chooser string. You could try '-p android-armv6' first though.
(Reporter)

Updated

5 years ago
Attachment #640504 - Flags: review?(nrthomas) → review+
The Try build worked for ARMv6:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mfinkle@mozilla.com-0c42437fd6aa/try-android-armv6/

I see "%BUILD_TARGET%-armv6" in mobile.js
https://hg.mozilla.org/mozilla-central/rev/102443258731
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 636891 [details] [diff] [review]
Change update url in mobile.js using MOZ_PKG_SPECIAL

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no seperate update channel for ARMv6
Testing completed (on m-c, etc.): landed on m-c when it was Fx16
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #636891 - Flags: approval-mozilla-beta?
Attachment #640504 - Flags: approval-mozilla-beta?
Attachment #636891 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 640504 [details] [diff] [review]
fix armv7

build changes, no risk. approving.
Attachment #640504 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.