Closed Bug 574380 Opened 11 years ago Closed 11 years ago
make constants useful on trunk
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.
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)
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-
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.
Priority: -- → P2
Target Milestone: 1.5 → 1.6
This should block 2.0 final.
blocking2.0: --- → ?
Marking softblocker as, aiui, everything works, this is just about simplifying log interpretation.
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.)
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.
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/?
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
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.
Looking green so far: http://tbpl.mozilla.org/?tree=MozillaTry&rev=aaba47d531a9
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)
fx-sync part landed: http://hg.mozilla.org/services/fx-sync/rev/5a01d066018e
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.
(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)" 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.
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?
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: 11 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][fixed in places] → [softblocker]
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.