Closed Bug 745065 Opened 8 years ago Closed 8 years ago

Remove apps sync engine

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: anant, Assigned: anant)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

When the new AITC client (bug 744257) lands, the apps sync engine in the tree may removed as it duplicates functionality provided by AITC.
Depends on: 744257
Is there any need to wait?
No reason to wait, in fact Fabrice also suggests we remove it now so we can check in the changes we need to DOMApplicationRegistry.

I'll attach a patch shortly.
Blocks: 744257
No longer depends on: 744257
Blocks: 745069
Attached patch Remove App Sync Engine - v1 (obsolete) — Splinter Review
This patch removes the app sync engine, which will be replaced by the new AITC client.
Assignee: nobody → anant
Status: NEW → ASSIGNED
Attachment #614681 - Flags: review?(rnewman)
There are a bunch of removed-files.in files scattered in the tree. Do we need to update those as well? (I'm really not sure what they are used for.)
(In reply to Gregory Szorc [:gps] from comment #4)
> There are a bunch of removed-files.in files scattered in the tree. Do we
> need to update those as well? (I'm really not sure what they are used for.)

# Removed-files.in is processed at build time to create a list of files that
# should be removed during an application update.

So yes, files that were in a release at some point but should be removed in an update because they're no longer shipped, must be added to removed-files.in.
Ah, you're right; they are used to remove files on application updates, so we'll have to update that to make sure we don't leave apps.js lying around in modules/services-sync/engines/ on a Firefox update. I'll attach a patch to update removed-files.in.
Looking at removed-files.in, all of the sync engine files are already listed (line 1047). The blame for that line leads to bug 556644, which seems to indicate that since we switched to Omnijar it is unnecessary to add lines to removed-files.in.

Are there platforms where we still don't use Omnijar? If so we'd need to add an entry, but only if apps.js is also present on those platforms.
Add apps.js to removed-files.in. I've put it in two places (inside an #ifdef MOZ_OMNIJAR and outside) to be safe and remove the file from all platforms, whether or not we had Omnijar support on them.
Attachment #614681 - Attachment is obsolete: true
Attachment #614681 - Flags: review?(rnewman)
Attachment #614686 - Flags: review?(rnewman)
Comment on attachment 614686 [details] [diff] [review]
Remove App Sync Engine - v1.1

Review of attachment 614686 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. Please make sure that TPS and xpcshell tests pass.

You perhaps also want to revert some of the changes to webapps.jsm:

https://bug706545.bugzilla.mozilla.org/attachment.cgi?id=579890
Attachment #614686 - Flags: review?(rnewman) → review+
Thanks! The changes to Webapps.jsm will be done in bug 745069. Pushing to try to check tests.
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
Autoland Patchset:
	Patches: 614686
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b5099b5b4a39
Try run started, revision b5099b5b4a39. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b5099b5b4a39
Try run for b5099b5b4a39 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b5099b5b4a39
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-b5099b5b4a39
Whiteboard: [autoland-in-queue]
Pushed: https://hg.mozilla.org/services/services-central/rev/e24ad9cee741

Forgot the r= in the commit message! Clearly, it's been a while since I did a hg push...
We add this whiteboard for s-c pushes to help track the tree.
Whiteboard: [fixed in services]
https://tbpl.mozilla.org/?tree=Services-Central&rev=e24ad9cee741 shows a few oranges for this check-in. What is the general procedure when this occurs and how do we investigate which failures are real and which are intermittent?
(In reply to Anant Narayanan [:anant] from comment #15)
> https://tbpl.mozilla.org/?tree=Services-Central&rev=e24ad9cee741 shows a few
> oranges for this check-in. What is the general procedure when this occurs
> and how do we investigate which failures are real and which are intermittent?

You star them! Take a look at tbpl now, as I do so.
In general, if we break something like this it'll be in xpcshell-tests (the "X" in tbpl), and it'll usually be on all platforms. You're just seeing intermittent orange here.
Nice, thanks!

TPS tests are also run for check-ins to services-central, right? Where can I find out whether they succeeded?
The last TPS report I received was on the 10th, so it doesn't appear to have run for your push.

Will ping jgriffin.

You can run them yourself:

https://developer.mozilla.org/en/TPS

or you can ask jgriffin to add you to the mailing list if you want to keep an eye on them over time.
We're holding off on removing the sync engine as the decision to use AITC exclusively on all Gecko platforms is being revisited. In the meantime, the plan is to have both the classic sync engine for apps as well as AITC, behind prefs so only one is active at any given time.

Once clear consensus has been reached, one of these will likely go away.

https://hg.mozilla.org/services/services-central/rev/c019b4bbe9ff
Whiteboard: [fixed in services]
(In reply to Anant Narayanan [:anant] from comment #20)
> We're holding off on removing the sync engine as the decision to use AITC
> exclusively on all Gecko platforms is being revisited.

Does that mean that Bug 705910 needs to be reopened?
The code in bug 705910 did not end up landing in mozilla-central, while the code from bug 706545 did. As for a re-review of the latter, it may or may not be required, but I suggest that we wait for consensus on which app sync solution is the right one going forward before spending any time on it.
services train in QA today.  gps called out this bug, but it's actually a no action item, correct?
Whiteboard: [qa-]
Target Milestone: --- → mozilla15
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.