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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 1

[Marco Castelluccio [:marco]]

What about using MOZ_ARCH instead of CPU_ARCH here: https://mxr.mozilla.org/mozilla-central/source/configure.in#3821

Comment 2

[Mike Hommey [:glandium]]

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

Comment 3

[Marco Castelluccio [:marco]]

Can we use MOZ_ARCH if MOZ_ARCH is set and CPU_ARCH otherwise?

Comment 4

[Mike Hommey [:glandium]]

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

Comment 5

[Marco Castelluccio [:marco]]

(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).

Comment 6

[Mike Hommey [:glandium]]

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

Comment 7

[(not currently active) Ted Mielczarek]

Can we wedge MOZ_PKG_SPECIAL in there, since that's what we're using for package names?

Comment 8

[Marco Castelluccio [:marco]]

Attachment: 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 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Change update url in mobile.js using MOZ_PKG_SPECIAL

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Traditionally we've made these sorts of changes at a lower level, so that they affect blocklist queries too. Is that possible here ?

Comment 12

[(not currently active) Ted Mielczarek]

I'm not really sure what the right way to fix this is. CCing some people who know the update/blocklist code.

Comment 13

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

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

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 16

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

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)

Comment 17

[Nick Thomas [:nthomas] (UTC+12)]

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

[Armen [:armenzg]]

I will email the metrics team to see who can weigh in.

Comment 19

[Daniel Einspanjer [:dre] [:deinspanjer]]

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.

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

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

[Pedro Alves]

Correct. s/Platform/Build Target/

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

The build target for the v7 builds was regressed by this change, changing from Android_arm-eabi-gcc3 to Android_arm-eabi-gcc3-.

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

Seems to me the unconditional change to DEFINES is the issue here, and the patch needs fixing up.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

So MOZ_PKG_SPECIAL is always defined and therefore we never use the #else block

Comment 26

[Justin Wood (:Callek)]

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 <[e-mail]>
>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.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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...

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Crud. No ARMv6 builds on Try.

Comment 29

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

The Try build worked for ARMv6:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0c42437fd6aa/try-android-armv6/

I see "%BUILD_TARGET%-armv6" in mobile.js

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

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:

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 640504 [details] [diff] [review]
fix armv7

build changes, no risk. approving.