503429 - reorganize weave source code and update build system

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

Basic goals:

* Remove requirement for xulrunner SDK unless hacking on crypto
* Only rebuild crypto when specifically required
* Clearly separate out tools/scripts from main source
* Lower the bar for someone to check out and build a working copy
* Help avoid crufty XPIs by having everything stage to an easily-nuked objdir
* Make automation easier by making the build process cleaner 

Planned layout:

 weave/
   crypto/
     components/
     src/
     platform/
     Makefile
   source/
     chrome/
     components/
     defaults/
     modules/
     chrome.manifest.in
     install.rdf.in
     Makefile
   tests/
     harness/
     system/
     unit/
     readme.txt
   tools/
     build/
     docs/
     scripts/
   Makefile
   README

Comment 1

[Mike Connor [:mconnor]]

Attachment: patch

want to look at this in the morning, but it works, and tests pass on both Windows and Mac

Comment 2

[Mike Connor [:mconnor]]

To-do:

* Run tests in a testing dir, rather than dumping files into the source directory.
* Get told how little I know about writing Makefiles
* Document how the build system works now

Optional, may spin off to another bug:

* build-single-platform=1 build flag for people doing their own distros who don't need/want other binary packages

Comment 3

[Ed Lee :Mardak]

Attachment: single patch

.. what mconnor posted but merged into one patch and both crypto and source's Makefile is tagged as copied from ./Makefile. Also updated .hgignore to have ^dist/.. I probably should have left out the other .hgignore stuff though.

Comment 4

[Mike Connor [:mconnor]]

Cool, thanks, I'm too toasty to think about this stuff much.

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg has all of my changes to date, can you push those changes there? :)

Comment 5

[Ed Lee :Mardak]

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/rev/ecc2d2bc896b
Combine copy command for chrome/modules/components/defaults. 

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/rev/55ab327eee8b
Add ^dist/ to .hgignore.

Comment 6

[Mike Connor [:mconnor]]

(In reply to comment #2)
> To-do:
> 
> * Run tests in a testing dir, rather than dumping files into the source
> directory.

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/rev/2acbafdea535

> * Get told how little I know about writing Makefiles

comment 5 is a start

> * Document how the build system works now

Still a WIP:

https://wiki.mozilla.org/User:Mconnor/WeaveSourceReorg/BuildDocs#Building_the_Extension

> Optional, may spin off to another bug:
> 
> * build-single-platform=1 build flag for people doing their own distros who
> don't need/want other binary packages

Spun off to Bug 503703

Comment 7

[Mike Connor [:mconnor]]

Attachment: single patch

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg has the changesets

Comment 8

[Mike Connor [:mconnor]]

Attachment: full patch, has all of the file moves, etc

Comment 9

[Ed Lee :Mardak]

Just a note that people using extension links will need to point their profile/extensions/<weave guid> text file to weave/dist/stage

Comment 10

[Ed Lee :Mardak]

I merged in the changes from weave.. and botched the merge.

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/diff/5d4bfa1dcab5/Makefile
http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/diff/c6dc48fb44ab/Makefile

But actually.. I wonder if it's better to just first commit with the <<< >>> merge tags. Because then you can see how the conflicts were actually resolved.


Go go mercurial..

~/weave-codebase-reorg [---]$ hg merge
merging Makefile
warning: conflicts during merge.
merging Makefile failed!
merging source/chrome/content/authenticator.js and chrome/content/authenticator.js to source/chrome/content/authenticator.js
merging source/chrome/content/fx-weave-overlay.xul and chrome/content/fx-weave-overlay.xul to source/chrome/content/fx-weave-overlay.xul
merging source/defaults/preferences/sync.js and defaults/preferences/sync.js to source/defaults/preferences/sync.js
merging source/modules/base_records/crypto.js and modules/base_records/crypto.js to source/modules/base_records/crypto.js
merging source/modules/base_records/keys.js and modules/base_records/keys.js to source/modules/base_records/keys.js
merging source/modules/constants.js.in and modules/constants.js.in to source/modules/constants.js.in
merging source/modules/engines.js and modules/engines.js to source/modules/engines.js
merging source/modules/engines/bookmarks.js and modules/engines/bookmarks.js to source/modules/engines/bookmarks.js
merging source/modules/engines/clientData.js and modules/engines/clientData.js to source/modules/engines/clientData.js
merging source/modules/engines/cookies.js and modules/engines/cookies.js to source/modules/engines/cookies.js
merging source/modules/engines/extensions.js and modules/engines/extensions.js to source/modules/engines/extensions.js
merging source/modules/engines/forms.js and modules/engines/forms.js to source/modules/engines/forms.js
merging source/modules/engines/history.js and modules/engines/history.js to source/modules/engines/history.js
merging source/modules/engines/input.js and modules/engines/input.js to source/modules/engines/input.js
merging source/modules/engines/microformats.js and modules/engines/microformats.js to source/modules/engines/microformats.js
merging source/modules/engines/passwords.js and modules/engines/passwords.js to source/modules/engines/passwords.js
merging source/modules/engines/plugins.js and modules/engines/plugins.js to source/modules/engines/plugins.js
merging source/modules/engines/prefs.js and modules/engines/prefs.js to source/modules/engines/prefs.js
merging source/modules/engines/tabs.js and modules/engines/tabs.js to source/modules/engines/tabs.js
merging source/modules/engines/themes.js and modules/engines/themes.js to source/modules/engines/themes.js
merging source/modules/resource.js and modules/resource.js to source/modules/resource.js
merging source/modules/service.js and modules/service.js to source/modules/service.js
merging source/modules/stores.js and modules/stores.js to source/modules/stores.js
merging source/modules/trackers.js and modules/trackers.js to source/modules/trackers.js
merging source/modules/type_records/bookmark.js and modules/type_records/bookmark.js to source/modules/type_records/bookmark.js
merging source/modules/util.js and modules/util.js to source/modules/util.js
2 files updated, 26 files merged, 0 files removed, 1 files unresolved

Comment 11

[Ed Lee :Mardak]

There seems to be some differences between the output of the current and this new build system..

tmp1 is the current build result with .xpi and .jar unpacked. tmp2 is new.

$ diff -r tmp1 tmp2
Binary files tmp1/chrome/sync.jar and tmp2/chrome/sync.jar differ
diff -r tmp1/chrome.manifest tmp2/chrome.manifest
22,23c22,23
< # content weave chrome/content/
< content weave jar:chrome/sync.jar!/content/
---
> content weave chrome/content/
> # content weave jar:chrome/sync.jar!/content/
25,26c25,26
< # skin weave class/1.0 chrome/skin/
< skin weave class/1.0 jar:chrome/sync.jar!/skin/
---
> skin weave class/1.0 chrome/skin/
> # skin weave class/1.0 jar:chrome/sync.jar!/skin/
28,29c28,29
< # locale weave en-US chrome/locale/en-US/
< locale weave en-US jar:chrome/sync.jar!/locale/en-US/
---
> locale weave en-US chrome/locale/en-US/
> # locale weave en-US jar:chrome/sync.jar!/locale/en-US/
diff -r tmp1/install.rdf tmp2/install.rdf
57c57
<     <em:updateURL>https://people.mozilla.com/~cbeard/weave/dist/update-dev.rdf</em:updateURL>
---
>     
Only in tmp1/modules: constants.js.in
Binary files tmp1/platform/Darwin/components/WeaveCrypto.dylib and tmp2/platform/Darwin/components/WeaveCrypto.dylib differ

Somehow MAKECMDGOALS are resulting in different comment blocks.. I'm doing "make xpi" for both.

Seems like the em:updateURL isn't being replaced right, but strange thing is xpi-type is set correctly.. Both makefiles generate "weave-0.5pre2-dev.xpi"

constants.js.in missing is a good thing

binary diff is good too

I expanded the jars for the diff, and they don't report any differences, so it's just the jars themselves.

Comment 12

[Mike Connor [:mconnor]]

Gah, good catch, MAKECMDGOALS is in a different makefile from the xpi target, gah.  I'll fix that in the morning.  (the XPI name is set in the toplevel makefile, so it'll be fine).

Not sure why the jars would be different, if the contents are the same it doesn't seem like a major problem to me, but I'm not an expert on zip files...

Comment 13

[Mike Connor [:mconnor]]

Issue with chrome.manifest/install.rdf is fixed in the reorg repo.  Edward, can you review instead of Dan?  You seem to be better at generating sane diffs, and you seem to have more cycles to spend on this.

Comment 14

[Dan Mills [:thunder]]

Comment on attachment 388105 [details] [diff] [review]
single patch

>+ifeq ($(rebuild_crypto),)
>+  crypto_build_target :=
>+else
>+	crypto_build_target := rebuild_all
>+endif
>+

nit: spaces vs tabs above.

>-all: test
>+all: build

why change this?

I see in the latest changeset in your branch:

http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/rev/cd640f9843d0

that there's a chrometarget variable.  Why not just pass down MAKECMDGOALS instead?

Otherwise it's looking pretty good!

Comment 15

[Dan Mills [:thunder]]

Btw, I'm also not worried about the jars themselves are different, so long as the contents are correct.  Everything else looks fine, so with my comments from #14 addressed this is good to go imo.

Comment 16

[Mike Connor [:mconnor]]

(In reply to comment #14)
> (From update of attachment 388105 [details] [diff] [review])
> >+ifeq ($(rebuild_crypto),)
> >+  crypto_build_target :=
> >+else
> >+	crypto_build_target := rebuild_all
> >+endif
> >+
> 
> nit: spaces vs tabs above.

Gah, fixed.

> >-all: test
> >+all: build
> 
> why change this?

Because my goal is to let someone grab the source and build with a minimum of effort (setting up testing isn't easy enough, yet, but I'm going to take a stab at that), and because I want to be able to rebuild the flat extension with "make"

> I see in the latest changeset in your branch:
> 
> http://hg.mozilla.org/users/mconnor_mozilla.com/weave-codebase-reorg/rev/cd640f9843d0
> 
> that there's a chrometarget variable.  Why not just pass down MAKECMDGOALS
> instead?

Originally I did that, but then I realized that I only want to jar chrome if we're building an XPI, as long as we're supporting flat chrome, so calling make vs. make xpi has added significance.

Comment 17

[Mike Connor [:mconnor]]

Fixed.

Comment 18

[Dan Mills [:thunder]]

Unfortunately, the dist directory makes it so I have to run 'make' after every change now.

I don't mind having to run make after an hg pull/up, but being able to keep files open in emacs, make changes, and quickly test (with the extension developer's extension you don't even need to restart in some cases).

Maybe symlinks are an option, at least on unix-like platforms.  Symlinks are a mess in windows, so I don't know what we could do there.

Comment 19

[Mike Connor [:mconnor]]

So, the tricky part is that there's stuff like "use a manifest instead of a big glob" fixme comments, so a pure srcdir build kinda sucks.  I'm fine with a *nix-only symlink build option, bearing in mind that this only will work for files that we don't preprocess in any way.  Can you file a new bug on me for that?

Comment 20

[Dan Mills [:thunder]]

Filed: bug 504868