Closed
Bug 567873
Opened 15 years ago
Closed 14 years ago
Android packaging should use standard packaging code
Categories
(Firefox Build System :: General, defect)
Tracking
(fennec2.0b2+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b2+ | --- |
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(4 files, 8 obsolete files)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jhford
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jhford
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•14 years ago
|
tracking-fennec: --- → 2.0b2+
Updated•14 years ago
|
Assignee: nobody → mwu
Assignee | ||
Comment 1•14 years ago
|
||
This also makes localization work.
Attachment #479637 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 2•14 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•14 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•14 years ago
|
||
Just the first 7 patches are necessary for the queue - hg qpush 6
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #479637 -
Attachment is obsolete: true
Attachment #479966 -
Flags: review?(ted.mielczarek)
Attachment #479637 -
Flags: review?(ted.mielczarek)
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
Attachment #481641 -
Flags: review?(aki)
Comment 10•14 years ago
|
||
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+
Comment 11•14 years ago
|
||
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•14 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•14 years ago
|
||
This patch prevents us from trying to upload debs when building for Android.
Attachment #481776 -
Flags: review?(mark.finkle)
Comment 14•14 years ago
|
||
I'll take a look at it today.
Updated•14 years ago
|
Attachment #481776 -
Flags: review?(mark.finkle) → review+
Comment 15•14 years ago
|
||
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•14 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•14 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?
Comment 18•14 years ago
|
||
(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.
Comment 20•14 years ago
|
||
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.
Comment 22•14 years ago
|
||
Ok, we have to revisit Bear's patch to not break Try Android uploads.
Comment 23•14 years ago
|
||
Attachment #481898 -
Flags: review?(jhford)
Comment 24•14 years ago
|
||
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•14 years ago
|
Attachment #481641 -
Attachment is obsolete: true
Assignee | ||
Comment 25•14 years ago
|
||
Review comment addressed. Patch tested. Checkin comment added.
Attachment #481775 -
Attachment is obsolete: true
Comment 26•14 years ago
|
||
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)
Comment 27•14 years ago
|
||
Attachment #481989 -
Flags: review?(jhford)
Assignee | ||
Comment 28•14 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.
Comment 29•14 years ago
|
||
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 30•14 years ago
|
||
Comment on attachment 481989 [details] [diff] [review]
add talosMasters to android; --with-system-zlib
looks good
Attachment #481989 -
Flags: review?(jhford) → review+
Comment 31•14 years ago
|
||
(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.
Comment 32•14 years ago
|
||
Same thing, but add an empty talos_masters to each non-android platform.
Attachment #481989 -
Attachment is obsolete: true
Attachment #482291 -
Flags: review?(jhford)
Comment 33•14 years ago
|
||
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)
Comment 34•14 years ago
|
||
As discussed. Passes checkconfig.
Attachment #482292 -
Attachment is obsolete: true
Attachment #482313 -
Flags: review?(jhford)
Attachment #482292 -
Flags: review?(jhford)
Comment 35•14 years ago
|
||
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•14 years ago
|
Attachment #481989 -
Attachment is obsolete: false
Updated•14 years ago
|
Attachment #482291 -
Attachment is obsolete: true
Attachment #482291 -
Flags: review?(jhford)
Assignee | ||
Comment 36•14 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: 14 years ago
Resolution: --- → FIXED
Comment 37•14 years ago
|
||
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.
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•