Closed Bug 574380 Opened 9 years ago Closed 9 years ago

make constants useful on trunk

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: philikon, Assigned: rnewman)

References

Details

(Whiteboard: [softblocker][qa-])

Attachments

(2 files, 3 obsolete files)

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.
Depends on: 571902, 572970
Attached patch v1 (obsolete) — Splinter Review
This would fill in the values to match the upcoming 1.4 release of the add-on.
Assignee: nobody → philipp
Attachment #453764 - Flags: review?(mconnor)
Target Milestone: --- → 1.5
doesn't block landing (fennec has been fine since they turned sync on), but it would be nice to have.
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.
Attachment #453764 - Flags: review?(mconnor) → review-
Blocks: 592375
Flags: blocking-fx-sync1.5+
Summary: Hardcode constants on trunk → make constants useful on trunk
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.
Flags: blocking-fx-sync1.5+
Priority: -- → P2
Target Milestone: 1.5 → 1.6
Duplicate of this bug: 617488
Duplicate of this bug: 618404
This should block 2.0 final.
blocking2.0: --- → ?
blocking2.0: ? → final+
Assignee: philipp → nobody
Marking softblocker as, aiui, everything works, this is just about simplifying log interpretation.
Whiteboard: [softblocker]
This should be assigned to you, right? (Just getting it off the nobody radar!)
Assignee: nobody → philipp
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.
Assignee: philipp → 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.)
Attached patch fx-sync part. v1Splinter Review
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.
Attachment #453764 - Attachment is obsolete: true
Attachment #508496 - Flags: review?(mconnor)
Attached patch m-c part. v1 (obsolete) — Splinter Review
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/?
Attachment #508502 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Whiteboard: [softblocker] → [softblocker][has patch][needs review mconnor]
Target Milestone: 1.6 → 2.0
Pause on this for a moment; a completely clean build just failed to copy. Checking.
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
Attached patch m-c part. v2 (obsolete) — Splinter Review
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.
Attachment #508502 - Attachment is obsolete: true
Attachment #508594 - Flags: review?(mconnor)
Attachment #508502 - Flags: review?(mconnor)
Attachment #508496 - Flags: review?(mconnor) → review+
Attachment #508594 - Flags: review?(mconnor) → review+
Whiteboard: [softblocker][has patch][needs review mconnor] → [softblocker][has patch][has review]
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 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 on attachment 508594 [details] [diff] [review]
m-c part. v2

Requesting additional review for the configure.in change.
Attachment #508594 - Flags: review?(benjamin)
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.
Attachment #508594 - Flags: review?(benjamin) → review?(khuey)
(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?". :)
Whiteboard: [softblocker][has patch][has review] → [softblocker][has patch][needs review khuey]
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.
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.
Depends on: 631390
(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 on attachment 508594 [details] [diff] [review]
m-c part. v2

r-, but only because I want to see it again.
Attachment #508594 - Flags: review?(khuey) → review-
Whiteboard: [softblocker][has patch][needs review khuey] → [softblocker][needs new patch]
(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.
Attached patch m-c part. v3Splinter Review
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?
Attachment #508594 - Attachment is obsolete: true
Attachment #509696 - Flags: review?(khuey)
Whiteboard: [softblocker][needs new patch] → [softblocker][has patch][needs review khuey]
Comment on attachment 509696 [details] [diff] [review]
m-c part. v3

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

r=khuey.
Attachment #509696 - Flags: review?(khuey) → review+
Richard, please land the m-c part on https://hg.mozilla.org/projects/places first.
(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
Whiteboard: [softblocker][has patch][needs review khuey] → [softblocker][fixed in places]
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][fixed in places] → [softblocker]
Depends on: 633895
Whiteboard: [softblocker] → [softblocker][qa-]
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.