critically important. This is joining mobile code & sync code to make the apk that we are going to ship.

Working on this now.

This works. I haven't tested without INCLUDE_NATIVE_SYNC for a while, so that might be busted. This is also hacky as shit, but within time constraints I'm happy. Note that with INCLUDE_NATIVE_SYNC set, it relies on additions from running https://github.com/mozilla-services/android-sync/blob/develop/fennec-merge-jar.sh which generates all of the manifests and copies things.

First patch got bitrotted *twice*. This seems to work. Note that I removed all of the conditionals, mainly because I wasn't confident in testing propagation of variables from shell -> make -> Preprocessor.py (if it's even possible!), or how releng would actually specify what they want to build. Depends on the import script to build. It's 3am, so I'm dumping this here for review and going to bed :)

Comment on attachment 581571 [details] [diff] [review] Proposed patch. v2 Review of attachment 581571 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +57,5 @@ > +SYNC_JAVA_CLASSPATH=$(SYNCDEPS) > +SYNC_JAVA_FILES=$(shell cat $(topsrcdir)/mobile/android/sync/java-sources.mn | tr '\n' ' ';) > +SYNC_RES_LAYOUT=$(shell cat $(topsrcdir)/mobile/android/sync/android-layout-resources.mn | tr '\n' ' ';) > +SYNC_RES_VALUES=$(shell cat $(topsrcdir)/mobile/android/sync/android-values-resources.mn | tr '\n' ' ';) > +SYNC_RES_XML=$(shell cat $(topsrcdir)/mobile/android/sync/android-xml-resources.mn | tr '\n' ' ';) might be cleaner to include a .mk file than to cat these

Here are three patches. The first is the majority of the work. The second is changes to the l10n build to include Sync's DTD. The third extends the first to preprocess the SyncAdapter manifest. This is a little bit hacky, because Make wasn't cooperating, but that's life.

(Not realistically expecting feedback from Axel; I believe he's on holiday.)

Example code drop MQ patch: <http://people.mozilla.com/~rnewman/sync-code-drop-patch-20111217T1724.tar.bz2>.

Comment on attachment 582612 [details] [diff] [review] Part 1: basic makefile changes. v3 Review of attachment 582612 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +58,5 @@ > +SYNC_JAVA_FILES=$(shell cat $(topsrcdir)/mobile/android/sync/java-sources.mn | tr '\n' ' ';) > +SYNC_PP_JAVA_FILES=$(shell cat $(topsrcdir)/mobile/android/sync/preprocess-sources.mn | tr '\n' ' ';) > +SYNC_RES_LAYOUT=$(shell cat $(topsrcdir)/mobile/android/sync/android-layout-resources.mn | tr '\n' ' ';) > +SYNC_RES_VALUES=$(shell cat $(topsrcdir)/mobile/android/sync/android-values-resources.mn | tr '\n' ' ';) > +SYNC_RES_XML=$(shell cat $(topsrcdir)/mobile/android/sync/android-xml-resources.mn | tr '\n' ' ';) as I said in comment 5, get rid of these calls to cat @@ +385,5 @@ > MOZ_ANDROID_DRAWABLES += mobile/android/base/resources/drawable/crash_reporter.png > RES_LAYOUT += res/layout/crash_reporter.xml > endif > > +MOZ_ANDROID_DRAWABLES += $(shell cat $(topsrcdir)/mobile/android/sync/android-drawable-resources.mn | tr '\n' ' ';) same here

Comment on attachment 582612 [details] [diff] [review] Part 1: basic makefile changes. v3 Review of attachment 582612 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +120,5 @@ > gfx/TextureReaper.java \ > gfx/TileLayer.java \ > gfx/ViewportMetrics.java \ > ui/PanZoomController.java \ > + $(SYNC_JAVA_FILES) \ move this to the top of the list to be consistent with the rest

Comment on attachment 582613 [details] [diff] [review] Part 2: locale build. v1 Review of attachment 582613 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/locales/Makefile.in @@ +51,1 @@ > STRINGSPATH = $(call core_abspath,$(call MERGE_FILE,android_strings.dtd)) should be able to do: STRINGSPATH = $(call core_abspath,$(call MERGE_FILE,android_strings.dtd sync_strings.dtd)) and then the second variable isn't required here or in strings.xml.in

Comment on attachment 582614 [details] [diff] [review] Part 3: SyncAdapter preprocessing. v1 Review of attachment 582614 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +457,5 @@ > $(DX) --dex --output=$@ $(SYNCDEPS) classes > > +# This needs to be preprocessed to match Fennec's browser authority. > +# Right now this leaves the preprocessed file in base/resources. Doing a direct > +# copy didn't seem to work. This needs fixing. I don't understand this comment. What's not working and what needs fixing? @@ +460,5 @@ > +# Right now this leaves the preprocessed file in base/resources. Doing a direct > +# copy didn't seem to work. This needs fixing. > +$(topsrcdir)/mobile/android/base/resources/xml/sync_syncadapter.xml: > + $(PYTHON) $(topsrcdir)/config/Preprocessor.py \ > + $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $(topsrcdir)/mobile/android/base/resources/xml/sync_syncadapter.xml.in > $@ add another \'s to get this to reasonable line length @@ +505,5 @@ > $(NSINSTALL) $(srcdir)/resources/values-v11/* res/values-v11 > > $(RES_XML): $(subst res/,$(srcdir)/resources/,$(RES_XML)) > $(NSINSTALL) -D res/xml > + $(NSINSTALL) $(srcdir)/resources/xml/*.xml res/xml/ $(NSINSTALL $< $@

(In reply to Brad Lassey [:blassey] from comment #13) > I don't understand this comment. What's not working and what needs fixing? My attempts to directly preprocess base/resources/xml/sync_syncadapter.xml.in to objdir/.../base/res/xml/sync_syncadapter.xml failed. These patches generate it in-place in base/resources, and allow the usual copy task to schlep it to the object directory. I am unwilling to spend *any* more time on this, so I wrote a comment to explain why `hg status` would show a new .xml file in the source tree after building. > $(NSINSTALL $< $@ I'd rather avoid making arbitrary improvements to the Makefile in this patch. This one-line change just avoids the .in file being copied with the .xml files. Please feel free to file a follow-up on someone with free time on the mobile team.

(In reply to Brad Lassey [:blassey] from comment #10) > as I said in comment 5, get rid of these calls to cat You actually said "might be cleaner", which in my world isn't an r- :) Happy to fix these nits in a follow-up some time next year. I will not be spending any more time on Makefiles until January 4th.

(In reply to Brad Lassey [:blassey] from comment #12) > Comment on attachment 582613 [details] [diff] [review] > Part 2: locale build. v1 > > Review of attachment 582613 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/locales/Makefile.in > @@ +51,1 @@ > > STRINGSPATH = $(call core_abspath,$(call MERGE_FILE,android_strings.dtd)) > > should be able to do: > STRINGSPATH = $(call core_abspath,$(call MERGE_FILE,android_strings.dtd > sync_strings.dtd)) > > and then the second variable isn't required here or in strings.xml.in MERGE_FILE basically switches between the merge dir, the l10n dir and en-US. Now if one of the two files is merged due to a missing string and the other isn't, they'll be picked up from completely different places. Thus they need to be included seperately, and need to be merged seperately.

(In reply to Brad Lassey [:blassey] from comment #12) > Comment on attachment 582613 [details] [diff] [review] > Part 2: locale build. v1 > > Review of attachment 582613 [details] [diff] [review]: ... > and then the second variable isn't required here or in strings.xml.in Axel confirmed that this is the correct approach. Please switch to r+. :)

Comment on attachment 582612 [details] [diff] [review] Part 1: basic makefile changes. v3 file a follow up bug to clean this up and get rid of the cats

Comment on attachment 582614 [details] [diff] [review] Part 3: SyncAdapter preprocessing. v1 Review of attachment 582614 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +457,5 @@ > $(DX) --dex --output=$@ $(SYNCDEPS) classes > > +# This needs to be preprocessed to match Fennec's browser authority. > +# Right now this leaves the preprocessed file in base/resources. Doing a direct > +# copy didn't seem to work. This needs fixing. file a follow up to get the generated files out of the src dir

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=913056d306e1 blassey et al: please take a look at the commits in the try push, and let me know of any comments ASAP. Note that this stacks on top of Sriram's patch (Bug 704682: Passwords needs to be exposed using a ContentProvider.). I hope that's done by the time I need to land this! There is an mq entry on the top that "prefs off" Sync for now. This uses a source drop method. As such, it has a public release dependency on about:license changes and MPL2. Erin is on top of those. If this is green in the morning, I'll land on m-c so I don't have to keep fighting bitrot. Thanks, everyone, for your help with this. /me zzzz

Landed, with no dependency on outstanding mobile bugs (preffed off, so it doesn't matter if it doesn't work). Filing follow-up right now. Thanks, everyone, for helping to get this done! https://hg.mozilla.org/mozilla-central/rev/81d34a32e031 https://hg.mozilla.org/mozilla-central/rev/7d1794f25fd3 https://hg.mozilla.org/mozilla-central/rev/462e372310a3 https://hg.mozilla.org/mozilla-central/rev/5c1e8120d743 https://hg.mozilla.org/mozilla-central/rev/b573033b92b6

We want this on aurora still, right?

(In reply to Axel Hecht [:Pike] from comment #22) > We want this on aurora still, right? FSVO "this", yes. I'm expecting to do another code drop soon, but there's no harm in having this base already in place on mozilla-aurora.

Comment on attachment 582612 [details] [diff] [review] Part 1: basic makefile changes. v3 Requesting approval-mozilla-aurora for all of the commits in Comment 21, of which these patches are a subset. Java-only preffed-off landing of first drop of Android Sync.

(In reply to Richard Newman [:rnewman] from comment #24) > Comment on attachment 582612 [details] [diff] [review] > Part 1: basic makefile changes. v3 > > Requesting approval-mozilla-aurora for all of the commits in Comment 21, of > which these patches are a subset. > > Java-only preffed-off landing of first drop of Android Sync. Would it be possible to build a try build of this on top of Aurora before landing it there? QA could then take a look. My concern is that if there's some issue with the uplift, updates could somehow be affected (which may not be caught until after our testing audience updates). Adding the qawanted keyword here regardless of how we move forward.

(In reply to Alex Keybl [:akeybl] from comment #25) > Would it be possible to build a try build of this on top of Aurora before > landing it there? QA could then take a look. My concern is that if there's > some issue with the uplift, updates could somehow be affected (which may not > be caught until after our testing audience updates). Yes indeed. And here it is! https://tbpl.mozilla.org/?tree=Try&rev=85fdd1f8db08

(In reply to Richard Newman [:rnewman] from comment #26) > https://tbpl.mozilla.org/?tree=Try&rev=85fdd1f8db08 … apparently Aurora branding breaks try: 07:51:19 < philor> rnewman: you have to go back to the first few pushes after the merge, find one that talks about "Android branding" and back it out 07:52:36 < philor> or just know what it does and reverse it, if it's as simple as just changing the branding in the mozconfig, I never actually look so here's a new try push for just Android: https://tbpl.mozilla.org/?tree=Try&rev=78883a55ceb8

The builds are at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rnewman@mozilla.com-78883a55ceb8/try-android/ is it ready for QA to examine the build to check the Sync functionality?

(In reply to Kevin Brosnan [:kbrosnan] from comment #28) > is it ready for QA to examine the build to > check the Sync functionality? There's nothing for QA to test; this is a preffed-off landing of code from December. Code and resources build, but no functionality is visible. The only thing QA could check at this point is that this landing doesn't affect the Fennec functionality in Aurora. (Which is absolutely worth doing!) (The goal of the m-c landing was to verify build integration and avoid bitrot for said integration. The goal of this Aurora landing is simply to keep pace with m-c to facilitate future landings, not to get features in front of users. That'll come soon.)

Richard - Can QA test the sign up process that happens in Android's Accounts area?

(In reply to Mark Finkle (:mfinkle) from comment #30) > Richard - Can QA test the sign up process that happens in Android's Accounts > area? Nope. I was intending for that to be live, but the discoverable UI in the Launcher to be hidden, but it didn't turn out that way. Alas. The UI will be visible for QA in my next drop (and I'd like to get something to Tony today), but that doesn't affect an Aurora landing of the build integration.

(In reply to Richard Newman [:rnewman] from comment #29) > (In reply to Kevin Brosnan [:kbrosnan] from comment #28) > > is it ready for QA to examine the build to > > check the Sync functionality? > > The only thing QA could check at this point is that this landing doesn't > affect the Fennec functionality in Aurora. (Which is absolutely worth doing!) Let's re-nominate for Aurora once QA has had the opportunity to do some testing (especially around updates). My biggest concern is basically breaking updates (or introducing a startup crash). Everything else can be resolved through yet another update :)

(In reply to Alex Keybl [:akeybl] from comment #32) > Let's re-nominate for Aurora once QA has had the opportunity to do some > testing (especially around updates). My biggest concern is basically > breaking updates (or introducing a startup crash). Everything else can be > resolved through yet another update :) Fair enough. Over to mobile QA!

For the record: we'll probably also want to land Bug 714874 along with this, just to make sure that our source dependencies don't add warning chatter to the build.

Aurora (see Bug 720933): https://hg.mozilla.org/releases/mozilla-aurora/rev/2bf61c2d0e29 https://hg.mozilla.org/releases/mozilla-aurora/rev/7b15cf9b89fa https://hg.mozilla.org/releases/mozilla-aurora/rev/50911c719782 https://hg.mozilla.org/releases/mozilla-aurora/rev/4c20c43a09a0 https://hg.mozilla.org/releases/mozilla-aurora/rev/62cdb08ef4d3

Marking verified.