Android packaging should use standard packaging code

RESOLVED FIXED

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: mwu, Assigned: mwu)

Tracking

Trunk
All
Android
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(4 attachments, 8 obsolete attachments)

529 bytes, patch
mfinkle
: review+
Details | Diff | Splinter Review
6.43 KB, patch
Details | Diff | Splinter Review
4.16 KB, patch
jhford
: review+
Details | Diff | Splinter Review
15.77 KB, patch
jhford
: review+
Details | Diff | Splinter Review
Assignee

Description

9 years ago
No description provided.
tracking-fennec: --- → 2.0b2+
Assignee: nobody → mwu
Assignee

Comment 1

9 years ago
This also makes localization work.
Attachment #479637 - Flags: review?(ted.mielczarek)
Assignee

Comment 2

9 years ago
This patch relies on the patch in bug 588607 which removes utils from embedding/android. Bug 575751 is also recommended to improve results from this patch.
Assignee

Comment 3

9 years ago
http://hg.mozilla.org/users/mwu_mozilla.com/patchpile/ is a patch queue that has this patch and the patches in bug 588607. That plus the patch in bug 575751 for the mobile frontend is recommended for testing.
Assignee

Comment 4

9 years ago
Just the first 7 patches are necessary for the queue - hg qpush 6
Comment on attachment 479637 [details] [diff] [review]
Switch to packager.mk to generate APKs

This patch may break how Android signing is done for nightlies and release as it changes the hook (the $(JARSIGNER) call) we were using during the make package step to sign the package as either a nightly or release.

I won't know for sure until we get this patch queue on to staging and run it thru the process, which will happen tonight or first thing tomorrow.

Updated

9 years ago
Duplicate of this bug: 562461
Assignee

Updated

9 years ago
Depends on: 601023
Assignee

Comment 7

9 years ago
Attachment #479637 - Attachment is obsolete: true
Attachment #479966 - Flags: review?(ted.mielczarek)
Attachment #479637 - Flags: review?(ted.mielczarek)
Tested "make package", "make upload" and "make package-tests" on a staging environment and they all are working.

"make package" does not make available the unsigned unaligned apk file that RelEng needs currently to do release signing, and while the method that mwu showed us to resign an apk does work - it's a change in process that we want to avoid right now so we have requested that the file be made available and uploaded.

After the madness that is 4.0 beta is over we can retool our resigning steps to not require it.
Comment on attachment 481641 [details] [diff] [review]
adjust android factory steps to use standard package code

This should work.
We need to coordinate landing with mwu, and watch to make sure uploads a) work and b) upload to the right location.
Attachment #481641 - Flags: review?(aki) → review+
When can we land this?
I'd prefer:

a) soon; l10n depends on this, and we want that in place for beta2
b) earlier in the day, so we can monitor+fix anything that goes wrong.
Assignee

Comment 12

9 years ago
Ted, will you be able to do this review soon? Looks like releng wants to get this in as early as possible. This version of the patch has no dependencies on other patches.
Attachment #479966 - Attachment is obsolete: true
Attachment #481775 - Flags: review?(ted.mielczarek)
Attachment #479966 - Flags: review?(ted.mielczarek)
Assignee

Comment 13

9 years ago
This patch prevents us from trying to upload debs when building for Android.
Attachment #481776 - Flags: review?(mark.finkle)
I'll take a look at it today.
Attachment #481776 - Flags: review?(mark.finkle) → review+
Comment on attachment 481775 [details] [diff] [review]
Switch to packager.mk to generate APKs, v3

>diff --git a/toolkit/locales/l10n.mk b/toolkit/locales/l10n.mk
>--- a/toolkit/locales/l10n.mk
>+++ b/toolkit/locales/l10n.mk
>@@ -165,7 +165,7 @@ ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> 	mv $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_APPNAME)/$(_APPNAME)/Contents/Resources/$(AB).lproj $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_APPNAME)/$(_APPNAME)/Contents/Resources/en.lproj
> endif
> endif
>-ifdef MOZ_OMNIJAR
>+ifeq ($(MOZ_OMNIJAR),1)
> 	@(cd $(STAGEDIST) && $(UNPACK_OMNIJAR))
> endif
> 	$(MAKE) clobber-zip AB_CD=$(AB_CD)
>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -55,7 +55,11 @@ else
>    ifeq (,$(filter-out gtk2 qt, $(MOZ_WIDGET_TOOLKIT)))
>       MOZ_PKG_FORMAT  = BZ2
>    else
>-      MOZ_PKG_FORMAT  = TGZ
>+      ifeq (Android,$(OS_TARGET))
>+          MOZ_PKG_FORMAT = APK
>+      else
>+          MOZ_PKG_FORMAT = TGZ
>+      endif
>    endif
> endif
> endif
>@@ -147,6 +151,67 @@ INNER_MAKE_PACKAGE	= rm -f app.7z && \
> INNER_UNMAKE_PACKAGE	= $(CYGWIN_WRAPPER) 7z x $(UNPACKAGE) && \
>   mv core $(MOZ_PKG_DIR)
> endif
>+ifeq ($(MOZ_PKG_FORMAT),APK)
>+
>+# we have custom stuff for Android
>+MOZ_OMNIJAR = 0

You should be able to just do MOZ_OMNIJAR= and continue using "ifdef MOZ_OMNIJAR". It works in my local testing, is there a reason it didn't work for you? (We don't really ever use 0/1 for boolean makefile vars, just unset/set.)

>+
>+JAVA_CLASSPATH = $(ANDROID_SDK)/android.jar
>+include $(topsrcdir)/config/android-common.mk
>+
>+JARSIGNER ?= echo
>+
>+DIST_FILES = \

This continues to be awful. I have no doubt that this will someday cause someone to break Android builds because something isn't getting packaged.

>+PKG_SUFFIX      = .apk
>+INNER_MAKE_PACKAGE	= \

Then again, this is worse. Can we please bribe khuey to rewrite packaging completely in Python and make this all less horrible?

Aside from all the awfulness that you've just shuffled from one makefile to another, this looks fine.
Attachment #481775 - Flags: review?(ted.mielczarek) → review+
Assignee

Comment 16

9 years ago
(In reply to comment #13)
> Created attachment 481776 [details] [diff] [review]
> Only upload deb on OS_LINUX
> 
> This patch prevents us from trying to upload debs when building for Android.

http://hg.mozilla.org/mobile-browser/rev/e5481aaa831d
Assignee

Comment 17

9 years ago
(In reply to comment #15)
> Then again, this is worse. Can we please bribe khuey to rewrite packaging
> completely in Python and make this all less horrible?
> 

khuey, what's your price?
(In reply to comment #17)
> (In reply to comment #15)
> > Then again, this is worse. Can we please bribe khuey to rewrite packaging
> > completely in Python and make this all less horrible?
> > 
> 
> khuey, what's your price?

releng back's mobile dev's bid to assist in this happening before the singularity
To finish 511648?  Does it actually block this?  I could knock it out this weekend if it's needed.
No, it doesn't block anything. It just makes me sad every time I have to look at that bunch of shell gunk. I just know it's going to break at some point.
Ah, if we're talking more than packager.mk, my price goes up significantly.
Ok, we have to revisit Bear's patch to not break Try Android uploads.
Posted patch this should fix try uploads (obsolete) — Splinter Review
Attachment #481898 - Flags: review?(jhford)
Comment on attachment 481898 [details] [diff] [review]
this should fix try uploads

looks good.  If I remember correctly, the main difference between the android build factory and the desktop build factory was the packaging and uploading bits.

Should we look at merging the two factories in the future?

Regardless, r+ assuming this passed/passes testing

Updated

9 years ago
Attachment #481641 - Attachment is obsolete: true
Assignee

Comment 25

9 years ago
Review comment addressed. Patch tested. Checkin comment added.
Attachment #481775 - Attachment is obsolete: true
Posted patch add sendchanges after upload (obsolete) — Splinter Review
tested in staging; i was able to upload+send all 4 sendchanges.
Attachment #481898 - Attachment is obsolete: true
Attachment #481988 - Flags: review?(jhford)
Attachment #481898 - Flags: review?(jhford)
Assignee

Comment 28

9 years ago
I was planning to add --with-system-zlib directly to configure.in as part of the dlopen bug (this bug doesn't require --with-system-zlib afaik), but it shouldn't hurt either.

Updated

9 years ago
Blocks: 603266
Comment on attachment 481988 [details] [diff] [review]
add sendchanges after upload

>diff --git a/misc.py b/misc.py
>--- a/misc.py
>+++ b/misc.py
>@@ -2159,16 +2159,17 @@ def generateMobileBranchObjects(config, 
>             'mozRevision': pf.get('mozilla_revision'),
>             'mobileRevision': pf.get('mobile_revision'),
>             'ausUser': config.get('aus2_user', None),
>             'ausSshKey': config.get('aus2_ssh_key', None),
>             'ausBaseUploadDir': config.get('aus2_mobile_base_upload_dir', None),
>             'ausHost': config['aus2_host'],
>             'downloadBaseURL': config['mobile_download_base_url'],
>             'updatePlatform': pf.get('update_platform', None),
>+            'talosMasters': None,

Shouldn't we also have unittest masters here?  I would also prefer that we don't hard code the None in here.  Instead, if we don't want sendchanges, lets set the talos and unit test master list to be empty lists in the config file.

>         if pf.get('enable_mobile_nightly', config.get('enable_mobile_nightly', True)):
>             builddir= '%s-nightly' % builddir_base
>             builder_name = '%s nightly' % base_name
>             nightly_kwargs = deepcopy(factory_kwargs)
>             nightly_kwargs['nightly'] = True
>-            nightly_kwargs['uploadSymbols'] = pf.get('upload_symbols', False)

wait, why is this patch disabling symbol generation and uploading?

>             nightly_kwargs['createSnippet'] = createSnippet
> 
>             factory = factory_class(**nightly_kwargs)
> 
>             builder ={
>                 'name': builder_name,
>                 'slavenames': pf.get('slaves'),
>                 'builddir': builddir,
>diff --git a/process/factory.py b/process/factory.py
>--- a/process/factory.py
>+++ b/process/factory.py
>@@ -133,22 +133,24 @@ def postUploadCmdPrefix(upload_dir=None,
>                 cmd[i] = a.fmtstring
>         return WithProperties(' '.join(cmd))
> 
> def parse_make_upload(rc, stdout, stderr):
>     ''' This function takes the output and return code from running
>     the upload make target and returns a dictionary of important
>     file urls.'''
>     retval = {}
>-    for m in re.findall("^(https?://.*?\.(?:tar\.bz2|dmg|zip))",
>+    for m in re.findall("^(https?://.*?\.(?:tar\.bz2|dmg|zip|apk))",
>                         "\n".join([stdout, stderr]), re.M):
>         if m.endswith("crashreporter-symbols.zip"):
>             retval['symbolsUrl'] = m
>         elif m.endswith("tests.tar.bz2") or m.endswith("tests.zip"):
>             retval['testsUrl'] = m
>+        elif m.endswith("apk") and 'unsigned' in m:
>+            retval['unsignedApkUrl'] = m

Will the 'gecko-unaligned.apk' file be an issue and should we account for it here as well?

If we don't need to use the urls, we can use the continue keyword instead of setting the properties.

>@@ -5805,57 +5810,106 @@ class MobileBuildFactory(MozillaBuildFac
>             command=['python', 'config/printconfigsetting.py',
>                      '%s/dist/bin/application.ini' % (self.objdir),
>                      'App', 'BuildID'],
>             property='buildid',
>             workdir='%s/%s' % (self.baseWorkDir, self.branchName),
>             description=['getting', 'buildid'],
>             descriptionDone=['got', 'buildid']
>         )
>+        self.addStep(SetProperty,
>+         name='set_got_revision',
>+         command=['hg', 'identify', '-i'],
>+         workdir='%s/%s/mobile' % (self.baseWorkDir, self.branchName),
>+         property='got_revision'
>+        )

Hmm, if we're going to do this, lets use a composite mozilla:mobile setting instead of the mobile-browser revision exclusively.  Neither changeset id is useful on its own.  This is what we do on the 'mobile' configuration.

>+        sendchangePlatform = None
>+        if self.talosMasters:
>+            if self.platform == 'android-r7':
>+                sendchangePlatform = 'android'
>+            if 'linux' in self.platform:
>+                sendchangePlatform = 'linux'

Shouldn't this also be checking for unittest masters as well?

Instead of deciding whether to run unit tests based on sendchangePlatform, lets use something like
          
          if self.talosMasters:
>+            talosBranch = "%s-%s-talos" % (self.branchName, sendchangePlatform)
>+            unittestBranch = "%s-%s-unittest" % (self.branchName, sendchangePlatform)
>+            for master, warn, retries in self.talosMasters:
>+                self.addStep(SendChangeStep(
>+                 name='sendchange_%s' % master,
>+                 warnOnFailure=warn,
>+                 master=master,
>+                 retries=retries,
>+                 branch=talosBranch,
>+                 revision=WithProperties("%(got_revision)s"),
>+                 files=[WithProperties('%(packageUrl)s')],
>+                 user="sendchange")
>+                )

and do unit tests in their own logic with something like
          
          if self.unittestMasters:
>+            for master, warn, retries in self.unittestMasters:
>+                self.addStep(SendChangeStep(
>+                 name='sendchange_%s' % master,
>+                 warnOnFailure=warn,
>+                 master=master,
>+                 retries=retries,
>+                 branch=unittestBranch,
>+                 revision=WithProperties('%(got_revision)s'),
>+                 files=[WithProperties('%(packageUrl)s'),
>+                         WithProperties('%(testsUrl)s')],
>+                 user="sendchange-unittest")
>+                )
> class MobileDesktopBuildFactory(MobileBuildFactory):
>     def __init__(self, packageGlobList=['-r', 'mobile/dist/*.tar.bz2',
>                                         'xulrunner/dist/*.tar.bz2'],
>                  **kwargs):
>         """This class creates a desktop fennec build.  -r in package glob
>         is to ensure that all files are uploaded as this is the first
>         option given to scp.  hack alert!"""
>@@ -7664,17 +7718,17 @@ class AndroidBuildFactory(MobileBuildFac
>         self.addBaseRepoSteps()
>         self.getMozconfig()
>         self.addPreBuildSteps()
>         self.addBuildSteps()
>         self.addPackageSteps()
>         if self.createSnippet:
>             self.addUpdateSteps()
>         self.addSymbolSteps()
>-        self.addUploadSteps(platform=self.uploadPlatform)
>+        self.addMakeUploadSteps()

Yay!

>         if self.triggerBuilds:
>             self.addTriggeredBuildsSteps()
>         if self.buildsBeforeReboot and self.buildsBeforeReboot > 0:
>             self.addPeriodicRebootSteps()
> 
>     def addPreCleanSteps(self):
>         self.addStep(ShellCommand,
>                 name='rm_cltbld_logs',


The rest of the patch looks good.  My main concern and reason for the r- is how we specify that we either want or don't want sendchanges.
Attachment #481988 - Flags: review?(jhford) → review-
Comment on attachment 481989 [details] [diff] [review]
add talosMasters to android; --with-system-zlib

looks good
Attachment #481989 - Flags: review?(jhford) → review+
(In reply to comment #29)
> Shouldn't we also have unittest masters here?

/me takes unittests out for a later patch.

> wait, why is this patch disabling symbol generation and uploading?

oops.

> >+        elif m.endswith("apk") and 'unsigned' in m:
> >+            retval['unsignedApkUrl'] = m
> 
> Will the 'gecko-unaligned.apk' file be an issue and should we account for it
> here as well?

Nope, mwu's patch takes that out aiui.

> If we don't need to use the urls, we can use the continue keyword instead of
> setting the properties.

We'll need to sendchange at some point with the unsigned-unaligned apk for automated signing. Unless there's something wrong with setting it, I'd like to keep it.

> >+        self.addStep(SetProperty,
> >+         name='set_got_revision',
> >+         command=['hg', 'identify', '-i'],
> >+         workdir='%s/%s/mobile' % (self.baseWorkDir, self.branchName),
> >+         property='got_revision'
> >+        )
> 
> Hmm, if we're going to do this, lets use a composite mozilla:mobile setting
> instead of the mobile-browser revision exclusively.  Neither changeset id is
> useful on its own.  This is what we do on the 'mobile' configuration.

As I pointed out in person, if Talos uses this, we need to use the mobile revision for graphs.

If it doesn't, I don't see a reason to send a revision.

> >+        sendchangePlatform = None
> >+        if self.talosMasters:
> >+            if self.platform == 'android-r7':
> >+                sendchangePlatform = 'android'
> >+            if 'linux' in self.platform:
> >+                sendchangePlatform = 'linux'
> 
> Shouldn't this also be checking for unittest masters as well?

Not if I take it out.
Posted patch android 080 upload configs (obsolete) — Splinter Review
Same thing, but add an empty talos_masters to each non-android platform.
Attachment #481989 - Attachment is obsolete: true
Attachment #482291 - Flags: review?(jhford)
Posted patch android 080 upload custom (obsolete) — Splinter Review
Take out oops, take out unittests, don't default to None for talosMasters.
Passes checkconfig.
Attachment #481988 - Attachment is obsolete: true
Attachment #482292 - Flags: review?(jhford)
As discussed. Passes checkconfig.
Attachment #482292 - Attachment is obsolete: true
Attachment #482313 - Flags: review?(jhford)
Attachment #482292 - Flags: review?(jhford)
Comment on attachment 482313 [details] [diff] [review]
set got_revision of mozillarev:mobilerev

>diff --git a/misc.py b/misc.py
>--- a/misc.py
>+++ b/misc.py
>@@ -2159,16 +2159,17 @@ def generateMobileBranchObjects(config, 
>             'mozRevision': pf.get('mozilla_revision'),
>             'mobileRevision': pf.get('mobile_revision'),
>             'ausUser': config.get('aus2_user', None),
>             'ausSshKey': config.get('aus2_ssh_key', None),
>             'ausBaseUploadDir': config.get('aus2_mobile_base_upload_dir', None),
>             'ausHost': config['aus2_host'],
>             'downloadBaseURL': config['mobile_download_base_url'],
>             'updatePlatform': pf.get('update_platform', None),
>+            'talosMasters': pf['talos_masters'],

Should we use .get('talos_masters', []) to avoid a KeyError if the config file doesn't have a talos_masters key?

>-
>+        sendchangePlatform = None

can this default to self.platform?

r=me with these two changes
Attachment #482313 - Flags: review?(jhford) → review+

Updated

9 years ago
Attachment #481989 - Attachment is obsolete: false

Updated

9 years ago
Attachment #482291 - Attachment is obsolete: true
Attachment #482291 - Flags: review?(jhford)
Assignee

Comment 36

9 years ago
Landed on m-c and tm and it seems to be working. Thanks everyone!

http://hg.mozilla.org/mozilla-central/rev/354b5ce1181d
http://hg.mozilla.org/tracemonkey/rev/98f601d658c4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
It looks like the nightly build needs to be updated:
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1286964131.1286969507.13925.gz

it's using "make package" but still trying to run:
bash -c find build/mozilla-central/objdir/embedding/android -maxdepth 1 -type f -name fennec.apk

The onchange builds and the nothumb nightly are working correctly.
Depends on: 604566

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.