Land sync and crypto components on trunk

RESOLVED FIXED in 1.4

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconnor, Assigned: philikon)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking-fx-sync1.4 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 21 obsolete attachments)

1.38 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
1.47 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
651.96 KB, patch
Details | Diff | Splinter Review
2.95 KB, text/plain
Details
13.38 KB, patch
ted
: review+
Details | Diff | Splinter Review
6.68 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
These should land as-is, with the appropriate build glue.

The intent is for this to land as a standalone component, available at build time.
(Reporter)

Updated

7 years ago
Assignee: nobody → edilee
Depends on: 570636

Updated

7 years ago
Flags: blocking-fx-sync1.4+
Target Milestone: --- → 1.4

Comment 1

7 years ago
Sounds like we want services to be a new top-level directory in mozilla-central. Under that we'll have 2 directories for crypto and sync.

The Makefile under services should probably just have those two as PARALLEL_DIRS that are conditionally added ifdef SERVICES_SYNC ?

SERVICES_SYNC would probably get configure-d based on --enable-services-sync

The crypto directory would have public and src with the Makefile DIRS = public src

crypto/public has IWeaveCrypto.idl with XPIDLSRCS = to build and MODULE = crypto

crypto/src has WeaveCrypto.js with EXTRA_COMPONENTS = to have the component located correctly and perhaps MODULE = crypto_s or _src?

sync/src doesn't need to compile anything but needs to declare the EXTRA_JS_MODULES and EXTRA_COMPONENTS = Weave.js (for about: registration and perhaps resource: mapping https://developer.mozilla.org/en/Using_JavaScript_code_modules#Programmatically_adding_aliases)

If EXTRA_JS_MODULES doesn't allow subdirectory, we might need to add to config/rules.mk for a special EXTRA_SYNC_MODULES ?

sync/test would point to XPCSHELL_TESTS = unit with the actual tests living under sync/test/unit. I believe head_ files will automatically get loaded.
Assignee: edilee → philipp

Comment 2

7 years ago
Not sure what's the magic for getting the new services directory picked up.

For hg convert of fx-sync repository to mozilla-central, we might eventually have a tracemonkey-like repository for service-specific landings that later merge to mozilla-central. But to start, we'll need to hg merge mozilla-central and the hg converted fx-sync.

Comment 3

7 years ago
Here's some the build changes from adding the ipc top-level directory and --disable-ipc flags:

http://hg.mozilla.org/mozilla-central/rev/fc6ed914e44e

I'm not sure if we want --disable-services-sync or --enable-services-sync
I'm going with --enable-services-sync so far.
Created attachment 452159 [details]
hg convert filemap

This is the filemap I use to get fx-sync into mozilla-central. Here's how to use it:

1. hg clone .../fx-sync
2. hg clone .../mozilla-central
3. hg convert --filemap attachment fx-sync sync-for-mc
4. cd mozilla-central
5. hg pull -f ../sync-for-mc
6. hg merge; hg commit

I'm guessing steps 3-6 are repeatable whenever fx-sync has updates. The 'hg pull' step in step 5 should recognize commits we already have merged.
Created attachment 452165 [details] [diff] [review]
Hook Sync into the build system

This hooks Sync into the build system underneath the 'services' top-level directory. Apply this patch on a repository that has been merged with an hg converted fx-sync repo.
Created attachment 452167 [details] [diff] [review]
Quick fix: make constants.js importable

Make constants.js importable. This is a quick fix to get things going on trunk for now, we should really be applying this to the fx-sync repo.
Created attachment 452168 [details] [diff] [review]
Quick fix: provide resource://services-sync

Alias resource://gre/modules/services-sync to resource://services-sync. This too should probably land in fx-sync.
Tests (make xpcshell-tests) fail with 

  OBJDIR/_tests/xpcshell/test_services_sync/unit/head_http_server.js:2: ReferenceError: Cu is not defined

That's because the trunk xpcshell test harness (testing/xpcshell/remoteexpcshelltests.py) looks for head_*.js and thus misses head.js which happens to be where Cu et.al. are defined. Renaming head.js to head_first.js or similar should fix the problem.

Updated

7 years ago
Depends on: 572970
After renaming head.js to head_first.js, tests still fail:

  uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: OBJDIR/_tests/xpcshell/test_services_sync/unit/head_first.js :: <TOP_LEVEL> :: line 9"  data: no]

The relevant line in head_first.js (head.js) is:

  let appinfo = Cc["@mozilla.org/xre/app-info;1"].
                getService(Ci.nsIXULAppInfo);
(Assignee)

Updated

7 years ago
Depends on: 573015
(Assignee)

Updated

7 years ago
Depends on: 572436
Comment on attachment 452167 [details] [diff] [review]
Quick fix: make constants.js importable

With bug 572970 landed this quick fix is no longer needed.
(Assignee)

Updated

7 years ago
Attachment #452167 - Attachment is obsolete: true
Created attachment 452286 [details]
hg convert filemap (v2)

Update hg convert filemap after bug 572970 was landed: no longer exclude prefs.js.in. Do exclude services/sync/tests/unit/Makefile, though.
Attachment #452159 - Attachment is obsolete: true

Updated

7 years ago
Depends on: 573108
Created attachment 452326 [details] [diff] [review]
[LANDED] Map resource://services-sync to resource://gre/modules/...

This is identical to the previous patch except that
a) it's against fx-sync (where it should end up)
b) it doesn't do anything if resource://services-sync already exists (add-on case)
Attachment #452168 - Attachment is obsolete: true
Attachment #452326 - Flags: review?(mconnor)
(Reporter)

Updated

7 years ago
Attachment #452326 - Flags: review?(mconnor) → review+

Comment 14

7 years ago
http://hg.mozilla.org/services/fx-sync/rev/03a87dd91969
If resource://services-sync isn't defined yet, alias it to resource://gre/modules/services-sync.
Created attachment 452366 [details]
hg convert filemap (v3)

Update hg convert filemap to include locales directory (in the convention as used by mozilla-central).
Attachment #452286 - Attachment is obsolete: true
Created attachment 452367 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services'

Splitting previous patch into two: this now provides the necessary build glue to make everything below the 'services' directory. New: support for 'locale' directory.
Attachment #452367 - Flags: review?(mconnor)
Created attachment 452368 [details] [diff] [review]
mozilla-central: hook 'services' into the build system

Second part of the previously combined patch: hook the 'services' directory into the global mozilla-central build system.
Attachment #452165 - Attachment is obsolete: true
Attachment #452368 - Flags: review?(mconnor)
(Reporter)

Updated

7 years ago
Attachment #452367 - Flags: review?(mconnor) → review+
(Reporter)

Updated

7 years ago
Attachment #452368 - Flags: review?(mconnor) → review+
Created attachment 452374 [details] [diff] [review]
fx-sync: handle absence of debug preference gracefully

Handle absence of debug preference gracefully in WeaveCrypto.js.
Attachment #452374 - Flags: review?(mconnor)

Comment 19

7 years ago
(In reply to comment #16)
> Created an attachment (id=452367) [details]
> mozilla-central: Makefiles and build helpers for 'services'
> 
> Splitting previous patch into two: this now provides the necessary build glue
> to make everything below the 'services' directory. New: support for 'locale'
> directory.

This really should be locales/ and probably even live at the top of services/ - please check with Axel on that before landing in mozilla-central, so that the L10n infrastructure fits what we actually need to have things working well.
(In reply to comment #19)
> This really should be locales/

Both singular and plural spellings seem to be used equally throughout mozilla-central :/. We were using 'locale' in the add-on so I stuck with it.
Comment on attachment 452366 [details]
hg convert filemap (v3)

This won't work with the l10n build systems.

Whether this is services/locales/en-US/(sync) or services/sync/locales/en-US doesn't matter to how l10n is set up, fwiw.
Attachment #452366 - Flags: review-
(In reply to comment #21)
> Whether this is services/locales/en-US/(sync) or services/sync/locales/en-US
> doesn't matter to how l10n is set up, fwiw.

So s/locale/locales/ would do the job?
For the l10n build systems to have a chance to pick this up, the new module will need to make it into the l10n.ini files. From the bug so far, I'm not sure if this is supposed to be part of toolkit or firefox, the "what's supposed to be localized" pieces go into different files.

But s/locale/locales/ is a good start, yes.

Side note, I guess the configure options should be run by ted.
(In reply to comment #23)
> For the l10n build systems to have a chance to pick this up, the new module
> will need to make it into the l10n.ini files. From the bug so far, I'm not sure
> if this is supposed to be part of toolkit or firefox, the "what's supposed to
> be localized" pieces go into different files.

It's really neither (which is why we're landing it a new top level dir), but it's more toolkit than browser, so we'll be hooking into toolkit/locales/l10n.ini. Thanks!
(Reporter)

Updated

7 years ago
Attachment #452374 - Flags: review?(mconnor) → review+

Comment 25

7 years ago
(In reply to comment #24)
> It's really neither (which is why we're landing it a new top level dir)

For this case, "toolkit" means "the platform", and toolkit's l10n.ini is being included by all Mozilla-based applications - I guess that's what we want for this part.
(Reporter)

Comment 26

7 years ago
Perhaps we should add our own l10n.ini ?  sync will likely not be built by all toolkit consumers, at least to start.
It's really a question on whether comm-central apps will have services on. KaiRo says for SeaMonkey that'd be a yes, so a separate l10n.ini sounds like a good idea.
Created attachment 452390 [details] [diff] [review]
[LANDED] fx-sync: harmonize locale directory structure with m-c conventions
Attachment #452390 - Flags: review?(mconnor)
(Reporter)

Updated

7 years ago
Attachment #452390 - Flags: review?(mconnor) → review+

Comment 29

7 years ago
http://hg.mozilla.org/services/fx-sync/rev/fbba677a4ad9
Harmonize locale directory structure with mozilla-central conventions.
(Assignee)

Updated

7 years ago
Attachment #452326 - Attachment description: Map resource://services-sync to resource://gre/modules/... → [LANDED] Map resource://services-sync to resource://gre/modules/...
(Assignee)

Updated

7 years ago
Attachment #452390 - Attachment description: fx-sync: harmonize locale directory structure with m-c conventions → [LANDED] fx-sync: harmonize locale directory structure with m-c conventions
Created attachment 452398 [details]
hg convert filemap (v4)

Updated hg convert filemap now that fx-sync matches the directory layout conventions of mozilla-central for locales (removed the rename).
Attachment #452366 - Attachment is obsolete: true
Created attachment 452404 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services' (v2)

* Fixed nit in licensing header.
* s/locale/locales
* Fix jar.mn to yield the same chrome URIs as the add-on
* Provide our own l10n.ini
Attachment #452367 - Attachment is obsolete: true
Attachment #452404 - Flags: review?(mconnor)
Created attachment 452405 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v2)

Hook services/sync/locales/l10n.ini into browser/locales/l10n.ini.
Attachment #452368 - Attachment is obsolete: true
Attachment #452405 - Flags: review?(mconnor)
(Reporter)

Updated

7 years ago
Attachment #452404 - Flags: review?(mconnor) → review+
(Reporter)

Updated

7 years ago
Attachment #452405 - Flags: review?(mconnor) → review+

Comment 33

7 years ago
Do we want the sync tags to land? Probably not so remove the "convert-repo update tags" changeset when landing on mozilla-central?

Comment 34

7 years ago
Actually nevermind, I wasn't running hg convert correctly.

How much revision history do we want to keep? Do we want to keep at least the history from when modules and such lived in source/modules? Right now the history starts from it moved to services/sync.

I've pushed the convert + 3 patches (crypto, makefiles, buildsystem) to try just to make sure nothing blows up.
Depends on: 573217
We'll need some buildmaster magic for the releng build infra to pick up the new dirs, I filed bug 573217 on that.

As that's a tad laborious, we should have the full set of new top-level dirs worked out. Is 'services/sync' all that we'll need for sync?

For the landing order, it should be fine to have the releng patch land a good deal before the mozilla-central landing, having additional dirs in that logic shouldn't hurt.
Background: l10n.ini is used to run compare-locales locally, and the dashboard uses it for build logic, releng decided to drop that code, and thus needs a patch.

Mardak, do you want me to do a local testrun whether things work like we'd expect wrt to repacks and compare-locales etc? If so, could you create a patch for me to test? hg diff -r -r would probably do it on your tree that you pushed to try.
(In reply to comment #34)
> How much revision history do we want to keep? Do we want to keep at least the
> history from when modules and such lived in source/modules? Right now the
> history starts from it moved to services/sync.

Hmm, I thought hg convert would follow renames back in history. If we want to include history from before the rename, we'll just need to add "include source" to the filemap.
(In reply to comment #35)
> As that's a tad laborious, we should have the full set of new top-level dirs
> worked out. Is 'services/sync' all that we'll need for sync?

In terms of l10n, yes. The UI parts for sync are landing in browser (see bug 571897).
Thanks, updated the releng bug.
Comment on attachment 452405 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v2)

You'll also need to hook services/sync up with the repacks, that's in 
http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#184, probably something like

        @$(MAKE) -C ../../services/sync/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1
Created attachment 452467 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v3)

Updated: hook services/sync into the repacks. Thanks, Axel!
Attachment #452405 - Attachment is obsolete: true
Attachment #452467 - Flags: review?(mconnor)

Comment 41

7 years ago
http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

(In reply to comment #36)
> include history from before the rename, we'll just need to add "include source"
> to the filemap.
We also need to pick what they're renamed to. And we need to make sure the names don't overlap as it'll cause problems.. So something like source -> services/sync/source and modules -> services/sync/modules ? I guess we don't need to worry about the chrome bits yet.
(In reply to comment #41)
> http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

Looks pretty green to me :)

> We also need to pick what they're renamed to. And we need to make sure the
> names don't overlap as it'll cause problems.. So something like source ->
> services/sync/source and modules -> services/sync/modules ? I guess we don't
> need to worry about the chrome bits yet.

Yeah. Probably something like this then:

include source
rename source services/sync

(modules was a subdir of source, no?)

Comment 43

7 years ago
Nod. You knew what I meant ;) And seems to work pretty well.. Better than fx-sync repository! It seems like the files were never moved :p

include modules
rename modules services/sync/modules
include source/modules
rename source/modules services/sync/modules
include services
...

It's as if thunder originally created the file at services/sync/modules/service.js ;)

Comment 44

7 years ago
Oh and in terms of merging, we'll want to clean up branches and tags?

hg up 1.0.x && hg ci --close-branch -m 'Close fx-sync 1.0.x branch.'
hg up 1.2.x && hg ci --close-branch -m 'Close fx-sync 1.2.x branch.'
hg up 1.3.x && hg ci --close-branch -m 'Close fx-sync 1.3.x branch.'

and strip out all fx-sync related .hgtags (not sure why it got included with the convert, but we can exclude it explicitly perhaps? first time I ran it, it also did .hgignore.. ??)
(In reply to comment #41)
> http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

Perhaps we should also have try-server do builds with --enable-services-sync (if there's no way to have it call configure with extra arguments we could hardcode MOZ_SERVICES_SYNC=1 in configure.in).

Comment 46

7 years ago
Created attachment 452471 [details] [diff] [review]
fx-sync super diff
slightly off topic, I was wondering, do you guys want to change the provider of the chrome urls from weave to sync?
(In reply to comment #47)
> slightly off topic, I was wondering, do you guys want to change the provider of
> the chrome urls from weave to sync?

Eventually yes, but not now :). Thanks for noticing, though.

Comment 49

7 years ago
I pushed a build with MOZ_SERVICES_SYNC=1 I think..
http://hg.mozilla.org/try/rev/b4a53290abf0

Didn't seem to copy modules into services-sync:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/edward.lee@engineering.uiuc.edu-b4a53290abf0/tryserver-macosx/

And tests didn't seem to run.
(In reply to comment #49)
> Didn't seem to copy modules into services-sync:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/edward.lee@engineering.uiuc.edu-b4a53290abf0/tryserver-macosx/

Hmm... If I build with --enable-services-sync, MinefieldDebug.app contains modules/services-sync. If I then do 'make package', the resulting .dmg will contain it, too.
(Reporter)

Comment 51

7 years ago
Ed, can you retry with a mozconfig-extra (tryserver build docs have this)
Comment on attachment 452404 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services' (v2)

>diff --git a/services/crypto/Makefile.in b/services/crypto/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/services/crypto/Makefile.in
>+DIRS = idl src

FYI, we're no longer separating interfaces and code into subdirectories. This just adds extra invocations of make and slows down the build. Just put the contents of idl/Makefile.in and src/Makefile.in directly into this Makefile instead. (There's not a whole lot going on in those Makefiles anyway.)
Comment on attachment 452467 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v3)

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -6936,6 +6936,15 @@
> AC_SUBST(MOZ_THEME_FASTSTRIPE)
> 
> dnl ========================================================
>+dnl = Mozilla Sync
>+dnl ========================================================
>+MOZ_ARG_ENABLE_BOOL(services-sync,
>+[  --enable-services-sync  Enable Mozilla Sync components],
>+    MOZ_SERVICES_SYNC=1,
>+    MOZ_SERVICES_SYNC= )
>+AC_SUBST(MOZ_SERVICES_SYNC)

Is there a compelling reason to add this as a configure option? If you just want it to be controllable per-app, then you can simply have the AC_SUBST line, and have apps set it in confvars.sh. (More configure options means more things that people will put in their mozconfigs, and more funny build configurations that we wind up supporting.)
(Reporter)

Updated

7 years ago
Attachment #452467 - Flags: review?(mconnor) → review+

Comment 54

7 years ago
Created attachment 452678 [details]
hg convert filemap (v5)

Added more include/rename entries to preserve various history across renames of files that we're interested in. Also renames files so it's as if they've always had the right names.

There's a bug with hg convert that I've patched locally that sometimes resulted in files disappearing if a renamed file is already renamed by convert:
http://mercurial.selenic.com/bts/issue2243
Attachment #452398 - Attachment is obsolete: true

Updated

7 years ago
Attachment #452678 - Attachment mime type: application/octet-stream → text/plain

Comment 55

7 years ago
Created attachment 452680 [details] [diff] [review]
f-sync super diff

I've been pushing these super diffs (as one changeset) to try because pushing the converted history seems to result in 504 gateway timeout or something.

Seems to build on maemo and fails on linux:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277109725.1277112471.18474.gz
make[5]: *** No rule to make target `FormNotifier.js', needed by `libs'.  Stop.

The commands I've been using to create the repo:
hg clone mozilla-central sync-trunk
hg clone -b default weave weave-default
hg convert --filemap filemap weave-default sync-trunk
hg -R sync-trunk strip tip
hg -R sync-trunk merge
hg -R sync-trunk ci -m 'Merge fx-sync to mozilla-central.'

I clone weave to weave-default to only convert the default branch so the other branches like 1.3.x don't need to be closed before landing on mozilla-central. Pretend hg convert does the right thing with my mercurial patch. The strip tip is to remove the .hgtags auto-generated commit.
Attachment #452471 - Attachment is obsolete: true

Comment 56

7 years ago
Created attachment 452750 [details] [diff] [review]
fx-sync super diff

Not sure why FormNotifier.js disappeared from the previous super diff, but I'm guessing that's why the build failed. Trying again with full history..
Attachment #452680 - Attachment is obsolete: true

Comment 57

7 years ago
First build for the latest push has appeared for maemo.. unpacking it doesn't seem to have services-sync stuff:
https://build.mozilla.org/tryserver-builds/edward.lee@engineering.uiuc.edu-try-18337e111a7a/

But then again, maemo builds have never failed, so seems like we'll need to tweak some other makefiles for mobile-browser ?
I suspect you're missing something in http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in

Comment 59

7 years ago
(In reply to comment #58)
> I suspect you're missing something in
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in
Thanks. Seems like we need to explicitly add our 1 crypto and 2 sync components, but modules are getting added fine.

Also, seems like tests are failing:
  /builds/slave/tryserver-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_services_sync/unit/head_http_server.js:2: ReferenceError: Cu is not defined

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277145373.1277146201.2013.gz

Comment 60

7 years ago
Locally adding the missing components to the os x build, I get this error while starting up:

Error: Ci.IWeaveCrypto is undefined
Source File: Minefield.app/Contents/MacOS/components/WeaveCrypto.js
Line: 471

Where are we supposed to put IWeaveCrypto.xpt? Doing the idl thing doesn't add it as an interface?

Comment 61

7 years ago
The xpt should 1) be in components and 2) should be listed in packages-manifest. In normal builds, it will be linked together with other xpts into browser.xpt due to step 2, but for manual testing, just doing step 1 in a normal build should work fine.
(In reply to comment #52)
> FYI, we're no longer separating interfaces and code into subdirectories. This
> just adds extra invocations of make and slows down the build. Just put the
> contents of idl/Makefile.in and src/Makefile.in directly into this Makefile
> instead. (There's not a whole lot going on in those Makefiles anyway.)

That's good to know, will do!

(In reply to comment #53)
> Is there a compelling reason to add this as a configure option? If you just
> want it to be controllable per-app, then you can simply have the AC_SUBST line,
> and have apps set it in confvars.sh. (More configure options means more things
> that people will put in their mozconfigs, and more funny build configurations
> that we wind up supporting.)

Fair enough. I didn't have a particular reason for it other than that it was suggested by mconnor and/or Mardak I think ;). I'm fine with just having the AC_SUBST line.
I'm thinking we may want to have bug 573194 block this... It's not strictly necessary in that it's easy to fix on extension side before next merge or fix in m-c post-merge, but we might as well do it now.

Updated

7 years ago
Depends on: 569355

Comment 64

7 years ago
Created attachment 452900 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services' (v3)

Flatten crypto directory (idl/src -> .) and sync (src -> .) for idl and components bits. Also add a pref.js as remapped from bug 569355.

+PREF_JS_EXPORTS = $(srcdir)/services-sync.js
Attachment #452900 - Flags: review?(mconnor)

Comment 65

7 years ago
Created attachment 452904 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v4)

Add components: .xpt and 3 .js to package-manifest.in plus .xpt for removed (for when it's packaged into browser.xpt)

--enable-services-sync is still there though..
Attachment #452404 - Attachment is obsolete: true
Attachment #452467 - Attachment is obsolete: true
Attachment #452904 - Flags: review?(mconnor)

Comment 66

7 years ago
(In reply to comment #65)
> Created an attachment (id=452904) [details]
> mozilla-central: hook 'services' into the build system (v4)

Note that it's usually best to request review from ted or some build system peer for those build system changes.

Updated

7 years ago
Attachment #452900 - Flags: review?(mconnor)

Updated

7 years ago
Attachment #452904 - Flags: review?(mconnor)

Comment 67

7 years ago
(In reply to comment #10)
> After renaming head.js to head_first.js, tests still fail:
> 
>   uncaught exception: [Exception... "Component returned failure code:
> 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult:
> "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
> OBJDIR/_tests/xpcshell/test_services_sync/unit/head_first.js :: <TOP_LEVEL> ::
> line 9"  data: no]
> 
> The relevant line in head_first.js (head.js) is:
> 
>   let appinfo = Cc["@mozilla.org/xre/app-info;1"].
>                 getService(Ci.nsIXULAppInfo);

Yeah, tryservers are giving that too:

  uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: /builds/slave/tryserver-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_services_sync/unit/head_first.js :: <TOP_LEVEL> :: line 9"  data: no]

Not sure why it's failing for us as it seems like other xpcshell tests use that as well.. ?
(In reply to comment #67)

> > The relevant line in head_first.js (head.js) is:
> > 
> >   let appinfo = Cc["@mozilla.org/xre/app-info;1"].
> >                 getService(Ci.nsIXULAppInfo);
> 
> Yeah, tryservers are giving that too:
> 
>   uncaught exception: [Exception... "Component returned failure code:
> 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult:
> "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
> /builds/slave/tryserver-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_services_sync/unit/head_first.js
> :: <TOP_LEVEL> :: line 9"  data: no]
> 
> Not sure why it's failing for us as it seems like other xpcshell tests use that
> as well.. ?

Are you guys making your own fake nsIXULAppInfo? Like this http://mxr.mozilla.org/mozilla-central/source/chrome/test/unit/test_bug399707.js#45

Updated

7 years ago
Depends on: 573668

Updated

7 years ago
Depends on: 573679

Updated

7 years ago
Depends on: 573691

Comment 69

7 years ago
Comment on attachment 452374 [details] [diff] [review]
fx-sync: handle absence of debug preference gracefully

We'll land with prefs so no need to handle missing default
Attachment #452374 - Attachment is obsolete: true

Comment 70

7 years ago
Created attachment 453008 [details]
hg convert filemap (v6)

Updated filemap for flatter src/components structure and adds history for tests, etc.
Attachment #452678 - Attachment is obsolete: true

Comment 71

7 years ago
Created attachment 453009 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v5)
Attachment #452904 - Attachment is obsolete: true
Attachment #453009 - Flags: review?(ted.mielczarek)

Comment 72

7 years ago
Created attachment 453010 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services' (v4)
Attachment #452900 - Attachment is obsolete: true
Attachment #453010 - Flags: review?(ted.mielczarek)

Comment 73

7 years ago
uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: /builds/slave/tryserver-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_services_sync/unit/head_first.js :: <TOP_LEVEL> :: line 68"  data: no]

Cu.import("resource://services-sync/log4moz.js");

Weave.js component is being loaded:
*** registering Weave.js: [ Weave Service, about:sync-log, about:sync-log.1 ]

But it's not registering it's resource://gre/modules/services-sync mapping to resouce://services-sync until app-startup.

We'll need to register it earlier or register it as part of a head_

Updated

7 years ago
Depends on: 573740

Updated

7 years ago
Depends on: 573842

Updated

7 years ago
Depends on: 573870

Comment 74

7 years ago
Created attachment 453265 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v6)

Missed the pref file.
Attachment #453009 - Attachment is obsolete: true
Attachment #453265 - Flags: review?(ted.mielczarek)
Attachment #453009 - Flags: review?(ted.mielczarek)

Comment 75

7 years ago
Woo!

Your Try Server test (d9d65b8ef3f5) was successfully completed on mac on builder: OS X 10.5.2 tryserver debug test xpcshell.

Summary of test results:
TinderboxSummaryMessage: s: try-mac-slave12
xpcshell: 840/0

All tests pass! (And seems like running the build with the sync addon (latest) works correctly too!)
Created attachment 453383 [details] [diff] [review]
mozilla-central: hardcode constants

On trunk we want to hardcode values that are normally filled in during the Sync add-on build process.
Attachment #453383 - Flags: review?(mconnor)
Comment on attachment 453010 [details] [diff] [review]
mozilla-central: Makefiles and build helpers for 'services' (v4)

>diff --git a/services/sync/locales/Makefile.in b/services/sync/locales/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/services/sync/locales/Makefile.in

Does this need to be its own Makefile, or could you just move the jar.mn up one level? (Presumably you could keep the .properties files in locales/ if you wanted, and just change the paths in the jar.mn).

Looks fine otherwise.
Attachment #453010 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 453265 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v6)

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -213,16 +213,17 @@
> @BINPATH@/components/pref.xpt
> @BINPATH@/components/prefetch.xpt
> @BINPATH@/components/profile.xpt
> @BINPATH@/components/proxyObject.xpt
> @BINPATH@/components/rdf.xpt
> @BINPATH@/components/satchel.xpt
> @BINPATH@/components/saxparser.xpt
> @BINPATH@/components/sessionstore.xpt
>+@BINPATH@/components/services-crypto.xpt

#ifdef MOZ_SERVICES_SYNC, please (and the other bits)

>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in
>--- a/browser/installer/removed-files.in
>+++ b/browser/installer/removed-files.in
>@@ -659,16 +659,17 @@ xpicleanup@BIN_SUFFIX@
>   components/profile.xpt
>   components/progressDlg.xpt
>   components/proxyObjInst.xpt
>   components/rdf.xpt
>   components/safebrowsing.xpt
>   components/satchel.xpt
>   components/saxparser.xpt
>   components/search.xpt
>+  components/services-crypto.xpt

Why is this being both added and removed???

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -8584,16 +8584,22 @@ if test "$MOZ_MORKREADER"; then
>   AC_DEFINE(MOZ_MORKREADER)
> fi
> 
> dnl Build Places if required
> if test "$MOZ_PLACES"; then
>   AC_DEFINE(MOZ_PLACES)
> fi
> 
>+dnl Build Sync Services if required
>+AC_SUBST(MOZ_SERVICES_SYNC)
>+if test "$MOZ_SERVICES_SYNC"; then

test -n "$MOZ_SERVICES_SYNC" is what we usually use (although I notice that the test above this isn't).

r=me with those fixes.
Attachment #453265 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #77)
> (From update of attachment 453010 [details] [diff] [review])
> >diff --git a/services/sync/locales/Makefile.in b/services/sync/locales/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/services/sync/locales/Makefile.in
> 
> Does this need to be its own Makefile, or could you just move the jar.mn up one
> level? (Presumably you could keep the .properties files in locales/ if you
> wanted, and just change the paths in the jar.mn).
> 
> Looks fine otherwise.

I think we need to have that makefile for l10n repacks, or at least should have it to keep things consistent with how we do it for the other l10n modules.

Comment 80

7 years ago
Created attachment 453521 [details] [diff] [review]
mozilla-central: hook 'services' into the build system (v7)

ifdef around package-manifest.in bits, removed removed-files, test -n

Leaving the makefiles unchanged from Axel's comments.
Attachment #453265 - Attachment is obsolete: true

Comment 81

7 years ago
http://hg.mozilla.org/mozilla-central/rev/3ff4b9aa29c7
Makefiles and build helpers for 'services'. 

http://hg.mozilla.org/mozilla-central/rev/227db4ad8cdf
Hook 'services' into the build system with MOZ_SERVICES_SYNC not-yet-set as a browser confvar. 

Full pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=227db4ad8cdf
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 574380
Comment on attachment 453383 [details] [diff] [review]
mozilla-central: hardcode constants

This patch is obsolete here since this bug is RESOLVED FIXED. See bug 574380 to follow up.
Attachment #453383 - Attachment is obsolete: true
Attachment #453383 - Flags: review?(mconnor)

Updated

7 years ago
Blocks: 576970
An update to the list of steps from comment #5.

1. hg clone .../fx-sync
2. hg clone .../mozilla-central
3. hg convert --filemap attachment fx-sync sync-for-mc
4. cd sync-for-mc
5. hg strip tip (this removes extraneous commit for tags)
6. (remove last line from .hg/shamap since that changeset was stripped)
7. cd ../mozilla-central
8. hg pull -f --branch=default ../sync-for-mc (branch=default makes sure only relevant changesets are brought in)
9. hg merge; hg commit
Blocks: 579175
We missed a piece of the l10n story, and thus the sync strings are still not exposed on the dashboard.

l10n repacks with l10n-merge on are fine, as they're picking up the strings from en-US, even though en-US doesn't even have those strings on yet. Not sure if that's worth fixing, depends a lot on the timeline of making this unconditional for fx. Is there a bug on that? Didn't find one naively.
Checked in v6 of the filemap and a bash script to perform steps 1 through 6 from comment 83: http://hg.mozilla.org/services/fx-sync/rev/4eab6b53d2c5
Are there any plans (bug) to remove MOZ_SERVICES_SYNC? I wonder whether I should just assume MOZ_SERVICES_SYNC=1 for the SeaMonkey UI parts, which would spare me enabling preprocessing in several jar.mn files.
(Reporter)

Comment 87

7 years ago
At this time, there are no plans to remove that, as I don't expect all apps to build it.  It's up to you for the preprocessing, but I also don't know what you're optimizing for there.

Comment 88

7 years ago
What might be reasonable is erroring out on building FF (or SM, but that's our problem) when it's not built and getting rid of it in browser/ (i.e. in the UI), just for making things cleaner.
(Reporter)

Comment 89

7 years ago
That feels like shuffling deck chairs, at this point.  Definitely not something that's at all a priority.  SM is, of course, free to act as they see fit.
You need to log in before you can comment on or make changes to this bug.