574380 - make constants useful on trunk

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Philipp von Weitershausen [:philikon]]

While add-on specific, the WEAVE_* constants are exposed through the 'Weave' namespace even on trunk. Their values on trunk should match whatever version of the add-on we integrated into trunk.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

This would fill in the values to match the upcoming 1.4 release of the add-on.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

doesn't block landing (fennec has been fine since they turned sync on), but it would be nice to have.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 453764 [details] [diff] [review]
v1

We should talk through this, but I'd prefer to have a solution that leads to things being auto-updated when we merge from fx-sync, so that we can debug from logs better.

Comment 4

[Mike Connor [:mconnor]]

Let's figure out how to do this right, but we'll want to make sure this works right so bug 568156 doesn't show garbage.

Comment 7

[Philipp von Weitershausen [:philikon]]

This should block 2.0 final.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Marking softblocker as, aiui, everything works, this is just about simplifying log interpretation.

Comment 9

[[e-mail]]

This should be assigned to you, right? (Just getting it off the nobody radar!)

Comment 10

[Philipp von Weitershausen [:philikon]]

I'm swamped with the last hardblocker, moving this over to rnewman.

I think we want the stuff in constants.js to reflect values defined in a Makefile. In the add-on we already do that, but we use sed (ick). Since we already have the pre-processor set up in fx-sync now (for head_appinfo.js.in), we should just use that. In m-c we will then have to define those values somewhere in the Makefiles and make sure we update them when we merge a new version from fx-sync.

Comment 11

[Richard Newman [:rnewman]]

Got the preprocessor doing the right thing... just wrestling with getting all our files in the right place.

(nsinstall takes directories as arguments; Preprocessor.py does not; and Sync doesn't follow Mozilla's natural directory structure.)

Comment 12

[Richard Newman [:rnewman]]

Attachment: fx-sync part. v1

Here's an admittedly slightly janky version. Makefiles suck.

This is for fx-sync; m-c coming in the next attachment.

Explanation:

* Rename xpi_type to weave_channel, because that's what it is.

* Fetch weave_version from a text file in services/sync, where m-c also looks. A merge from fx-sync to m-c will include changes to this file, so the version goes in lockstep.

* Define vars for the preprocessor. We can't use the ACDEFINES that we might normally use, because with my m-c changes those include -Dweave_version! Counter-productive.

* Use Preprocessor.py with those definitions, instead of using SUBST.

* Add the filter directive to the top of constants.js so the preprocessor will chew on it.

Comment 13

[Richard Newman [:rnewman]]

Attachment: m-c part. v1

m-c part.

* Add weave_* definitions to configure.in. This'll pull the version out of the version.txt file, just as for the add-on. (Is this the right place to put this logic, though?)

For now the channel is hard-coded to rel, and weave_id is hardcoded, too.

* Replace the one-line (recursive) nsinstall call with a monstrous "find files in a relative manner, preprocessing each" chunk. It'd be easier if the preprocessor was able to handle directories, but we invert and rename directories, so blah.

I believe this chunk of code is portable (no use of pushd, complex find args, shell expansions, etc.), but a try build is a necessity.

* Again, replace xpi_type with weave_channel, define the version in a file, and add #filter substitution to constants.js.


Should we take this opportunity to s/weave/sync/?

Comment 14

[Richard Newman [:rnewman]]

Pause on this for a moment; a completely clean build just failed to copy. Checking.

Comment 15

[Richard Newman [:rnewman]]

OK, my final version didn't create directories as it should.

Fixed version coming up momentarily. Sure nice to see this:

2011-01-31 15:51:44	Service.Main         DEBUG	Weave Version: 1.7b1pre Local Storage: 5 Remote Storage: 5

instead of this:

2011-01-31 13:58:22	Service.Main         DEBUG	Weave Version: @weave_version@ Local Storage: 5 Remote Storage: 5

Comment 16

[Richard Newman [:rnewman]]

Attachment: m-c part. v2

Verified that this works from a cleaned build: it creates the necessary directories first.

I would like to solve this bug by either making Preprocessor.py able to handle directories, or nsinstall able to take a filter command. Either would return our lib:: task to a one-liner... but this'll do for now.

Comment 17

[Richard Newman [:rnewman]]

Looking green so far:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=aaba47d531a9

Comment 18

[Philipp von Weitershausen [:philikon]]

Please land the m-c bits on the places branch https://hg.mozilla.org/projects/places. I can also land it the next time I merge to places/m-c.

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 508594 [details] [diff] [review]
m-c part. v2

This makes changes to the top level configure.in, should probably ask Ted Mielczarek or Benjamin Smedberg for review here as well, no?

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 508594 [details] [diff] [review]
m-c part. v2

Requesting additional review for the configure.in change.

Comment 21

[Richard Newman [:rnewman]]

fx-sync part landed:

http://hg.mozilla.org/services/fx-sync/rev/5a01d066018e

Comment 22

[Benjamin Smedberg]

Comment on attachment 508594 [details] [diff] [review]
m-c part. v2

Man, this looks really awful, and would probably be a lot better with $(wildcard). I've also been trying to avoid putting buildconfig defines into AC_DEFINE_UNQUOTED, and AC_SUBST them and manually putting them in DEFINES where they are used, but that's a stylistic choice. I'm going to bump this to khuey since I'm swamped.

Comment 23

[Richard Newman [:rnewman]]

(In reply to comment #22)

> I'm going to bump this to
> khuey since I'm swamped.

Thanks!

> Man, this looks really awful, and would probably be a lot better with
> $(wildcard). 

Happy to have a better proposal; I'm a total Make n00b, so I couldn't easily find a way to phrase "copy this whole tree over here, filtering each file through this command".

The Make docs don't make any mention of wildcard being recursive, and it seems that established wisdom for working around that is "shell out to find", which is the solution I came up with.

> I've also been trying to avoid putting buildconfig defines into
> AC_DEFINE_UNQUOTED, and AC_SUBST them and manually putting them in DEFINES
> where they are used, but that's a stylistic choice. 

Yeah, I was just following the apparent style in that regard. It's always a fine line between a reviewer saying "don't trample these existing things!" and "why didn't you do it the way everything else does?". :)

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ugh, I'd much rather make Preprocessor.py "do the right thing" with a directory.  I'll poke at that tonight and see how much work that would be.

Comment 25

[Mike Connor [:mconnor]]

Can we do that as a followup?  I know this is ugly, and I'm happy to make it better, but this is the last blocker for us, and I'd like to be done here.  Changing the preprocessor is... not the most risk-averse thing we can do right now.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #25)
> Can we do that as a followup?  I know this is ugly, and I'm happy to make it
> better, but this is the last blocker for us, and I'd like to be done here. 
> Changing the preprocessor is... not the most risk-averse thing we can do right
> now.

Fair enough.

Is there a compelling reason to do this in configure?  I'd prefer to hardcode these constants in services/sync/Makefile.in so that changing them doesn't cause a complete rebuild of the entire browser.

>diff --git a/services/sync/Makefile.in b/services/sync/Makefile.in
>--- a/services/sync/Makefile.in
>+++ b/services/sync/Makefile.in
>@@ -46,16 +46,40 @@ DIRS = locales
> 
> EXTRA_COMPONENTS = \
>   SyncComponents.manifest \
>   Weave.js \
>   $(NULL)
> 
> PREF_JS_EXPORTS = $(srcdir)/services-sync.js
> 
>+NORMALIZE_PATH = python -c "import os,sys; print os.path.realpath(sys.argv[1])"

You don't ever use this?

>+# Preprocess constants (by preprocessing everything).
>+# The 'HERE' idiom avoids a dependency on pushd. We need to do this fiddling in
>+# order to get relative paths, so we can process services/sync/modules/* into
>+# modules/services-sync/*.
>+# 
>+# Note that we find candidates, make directories, then 'copy' files.
> libs::
>-	$(PYTHON) $(topsrcdir)/config/nsinstall.py $(srcdir)/modules/* $(FINAL_TARGET)/modules/services-sync
>+ifndef NO_DIST_INSTALL
>+	$(EXIT_ON_ERROR) \
>+	HERE=$$PWD; \

I think you can just use $(CURDIR) instead of $$PWD.

>+	cd $(srcdir)/modules; \
>+	dirs=$$(find * -type d); \
>+	files=$$(find * -type f); \
>+	cd $$HERE; \
>+	for d in $$dirs; do \
>+		$(PYTHON) $(topsrcdir)/config/nsinstall.py -D $(FINAL_TARGET)/modules/services-sync/$$d; \
>+	done; \
>+	for i in $$files; do \
>+		src=$(srcdir)/modules/$$i; \
>+		dest=$(FINAL_TARGET)/modules/services-sync/$$i; \
>+		$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $(XULPPFLAGS) $$src > $$dest ; \
>+	done
>+endif
>+

Like I said above, I'd like to hardcode the constants in this file and then have SYNC_DEFINES = -Dweave_client=$(WEAVE_CLIENT) ...

I'm not really thrilled that this will be rebuilt every time, but given that this whole thing is a big hack I won't complain much.  Can you file a followup bug on one (or both) of your ideas from comment 16.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 508594 [details] [diff] [review]
m-c part. v2

r-, but only because I want to see it again.

Comment 28

[Richard Newman [:rnewman]]

(In reply to comment #26)

> Is there a compelling reason to do this in configure?
> ...

Good points, Kyle, thanks. I was following some of the patterns set out for MOZILLA_VERSION, but of course that's grossly inappropriate (at least for the current usage of those constants).

I've made some changes locally, which reduce the scope (though I'm still not touchin' the preprocessor); if everything builds fine I'll post a new patch tonight.

Comment 29

[Richard Newman [:rnewman]]

Attachment: m-c part. v3

Green build-only try push:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=6991eff77a91

Grabbed the Win32 build and verified that omni.jar contains the correct services-sync files, and constants.js has the right substitutions.

This version doesn't touch configure.in, and is altogether more pleasing!

Kyle, disregarding the workaround for the non-recursive preprocessor, does this satisfy?

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 509696 [details] [diff] [review]
m-c part. v3

Looks good (well, as good as a hack can look ;-))

r=khuey.

Comment 31

[Philipp von Weitershausen [:philikon]]

Richard, please land the m-c part on https://hg.mozilla.org/projects/places first.

Comment 32

[Richard Newman [:rnewman]]

(In reply to comment #31)
> Richard, please land the m-c part on https://hg.mozilla.org/projects/places
> first.

Done:

http://hg.mozilla.org/projects/places/rev/bf09d7997084

Comment 33

[Philipp von Weitershausen [:philikon]]

Landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/aa9d8ceefe9a
http://hg.mozilla.org/mozilla-central/rev/98aa2eb5f212
http://hg.mozilla.org/mozilla-central/rev/bf09d7997084