Closed
Bug 575894
Opened 14 years ago
Closed 12 years ago
Find a better way to deal with XPT linking
Categories
(Firefox Build System :: General, defect)
Tracking
(fennec-)
RESOLVED
INCOMPLETE
mozilla2.0b5
Tracking | Status | |
---|---|---|
fennec | - | --- |
People
(Reporter: khuey, Assigned: khuey)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
22.00 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
6.75 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
22.56 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
Our XPT linking story is a mess. The MSI installer generates its metadata from the package-manifest which is before XPT linking so the MSI installer will ship unlinked XPT files.
Comment 1•14 years ago
|
||
Perhaps you've looked, but Packager.pm's algorithm looks something like: 1) Find a section name (e.g. [browser]) 2) For every .xpt file listed in that section, link it to $sectionname.xpt
Comment 2•14 years ago
|
||
So yeah, inferring that the XPT files will be linked and listing just one is pretty easy. Note that I'm doing manifest-linking in bug 568691, also, so you'll need some logic for that.
Assignee | ||
Comment 3•14 years ago
|
||
So my thought on how to do this (for XPTs and manifests) was to set a flag in the makefile (say, IS_SHIPPED_COMPONENT = 1) which cause them to end up in the linked XPT/manifest.
Comment 4•14 years ago
|
||
I was actually thinking the opposite, and setting TEST_COMPONENT = 1 for non-shipped components, then we could eventually stick them all in $(DIST)/test instead of $(DIST)/bin, and maybe get rid of the packaging manifest at some point and just package everything in $(DIST)/bin.
Assignee | ||
Comment 5•14 years ago
|
||
Sure, reversing the polarity here is fine.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla2.0b5
Assignee | ||
Comment 6•14 years ago
|
||
There are some remaining issues with manifests, but this does XPTs flawlessly.
Assignee | ||
Comment 7•14 years ago
|
||
This patch moves XPT generation to the export phase, stores XPTs during the build in $(DIST)/xpt, and at the very end of the build links them together.
Attachment #468383 -
Attachment is obsolete: true
Attachment #474514 -
Flags: review?(mitchell.field)
Comment 8•14 years ago
|
||
Comment on attachment 474514 [details] [diff] [review] XPTs Looks good to me.
Attachment #474514 -
Flags: review?(mitchell.field) → review+
Comment 9•14 years ago
|
||
The downside here is that changing an interface and building in just one subdirectory will no longer work.
Assignee | ||
Comment 10•14 years ago
|
||
True. I'll see if I can find a way to fix that.
Assignee | ||
Comment 11•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #474514 -
Attachment is obsolete: true
Assignee | ||
Comment 12•14 years ago
|
||
Comment on attachment 476596 [details] [diff] [review] : Link XPTs in a sane way. This handles the "rebuild one" case by linking XPTs at the very end of the build (where .purgecaches is touched).
Attachment #476596 -
Flags: review+
Assignee | ||
Comment 13•14 years ago
|
||
Assignee | ||
Comment 14•14 years ago
|
||
Comment on attachment 476597 [details] [diff] [review] : Add a --first argument to buildlist.py. This adds a --first argument to buildlist.py so that the interface manifest can be listed first. If we list it later the browser explodes. I killed the Windows line endings while I was in the file.
Attachment #476597 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 15•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #476599 -
Flags: review?(bugspam.Callek)
Comment 16•14 years ago
|
||
Comment on attachment 476599 [details] [diff] [review] : comm-central changes. I'll get this review in the next day or so (I have some outstanding IRC Q's to khuey on buildlist.py changes). Either way this is happening, but requesting review from Standard8 as well, since TB will now package all .xpt's with this patch, and iirc they were explicitly not packaging some things. I do not feel we need to block on his review (unless he explicitly tells us to)
Attachment #476599 -
Flags: review?(bugzilla)
Comment 17•14 years ago
|
||
(In reply to comment #16) > Either way this is happening, but requesting review from Standard8 as well, > since TB will now package all .xpt's with this patch, and iirc they were > explicitly not packaging some things. If we were, I don't know why. The only things we wouldn't want to package nowadays would be test related xpts which this all seems to handle. The added bonus of this patch is also that we have less to maintain in package-manifest.in :-)
Comment 18•14 years ago
|
||
Comment on attachment 476599 [details] [diff] [review] : comm-central changes. I'm quite happy to let Justin review this.
Attachment #476599 -
Flags: review?(bugzilla)
Comment 19•14 years ago
|
||
Comment on attachment 476599 [details] [diff] [review] : comm-central changes. r- for something trivial but big. diff -r 95de0bfedba1 -r 04ffecbc295b suite/installer/package-manifest.in +@BINPATH@/components/suite.manifest ... +@BINPATH@/components/toolkitprofile.manifest Huh?? toolkitprofile.manifest comes from where? and I thought we needed suite.xpt not .manifest.
Attachment #476599 -
Flags: review?(bugspam.Callek) → review-
Comment 20•14 years ago
|
||
Comment on attachment 476599 [details] [diff] [review] : comm-central changes. Ok, I can relegate my last comment to make this a conditional review ;-) diff -r 95de0bfedba1 -r 04ffecbc295b config/config.mk + +default all:: if test -d $(DIST)/bin ; then touch $(DIST)/bin/.purgecaches ; fi + $(MAKE) xpt-link Nit: I'd like this before the .purgecaches line, but I want to match m-c for that, Mitch?
Attachment #476599 -
Flags: review- → review+
Assignee | ||
Comment 21•14 years ago
|
||
Looks like I just mangled toolkitprofile.manifest into toolkitplaces.manifest. Will fix. Suite.manifest is what I get for doing this late :-) As for the changing the order, I agree that xpt-link before touching .purgecaches is the way to go.
Updated•14 years ago
|
Version: unspecified → Trunk
Comment 22•14 years ago
|
||
I'm willing to believe we intentionally didn't package some xpt at some time, but looking at package-compare in a current tinderbox log, the only ones we aren't packaging now are things that appeared since the last time we looked.
Comment 23•14 years ago
|
||
Requesting blocking to get a decision on whether we want this fixed before fixing 526333 or bug 575751 .
tracking-fennec: --- → ?
Comment 24•14 years ago
|
||
(In reply to comment #23) > Requesting blocking to get a decision on whether we want this fixed before > fixing 526333 or bug 575751 . Do we need this to be fixed before fixing bug 526333?
Comment 25•14 years ago
|
||
What mfinkle is asking is if this is strictly necessary for bug 5263333. If not, what are the trade offs?
Comment 26•14 years ago
|
||
It's not necessary. khuey asked the review in bug 526333 to be held until this was fixed.
Assignee | ||
Comment 27•14 years ago
|
||
The other bug can go forward without this, that just doubles the work to be done there.
Comment 28•14 years ago
|
||
strictly speaking I don't think this bug should block. But if the implementation for bug 526333 is much easier if it depends on this bug then we can approve it for landing when the time comes to land 526333 (which does block fennec).
tracking-fennec: ? → 2.0-
Comment 29•14 years ago
|
||
Comment on attachment 476597 [details] [diff] [review] : Add a --first argument to buildlist.py. This is a slightly unfortunate change, since it means we have to rewrite the entire file every time now, instead of just appending new entries. >diff -r 2cf5863414e4 -r 8d76899a94f7 config/buildlist.py >--- a/config/buildlist.py Sat Sep 18 11:42:36 2010 -0400 >+++ b/config/buildlist.py Sat Sep 18 22:39:28 2010 -0400 >+'''A generic script to add entries to a file >+if the entry does not already exist. >+ >+Usage: buildlist.py <filename> <entry> [<entry> ...] >+''' Fix the comment here to mention the options. >+ >+import sys >+import os >+from utils import lockFile >+from optparse import OptionParser >+ >+def addEntriesToListFile(listFile, entries, first): >+ """Given a file |listFile| containing one entry per line, >+ add each entry in |entries| to the file, unless it is already >+ present.""" Fix the docstring comment to mention what "first" does. >+ lock = lockFile(listFile + ".lck") >+ try: >+ if os.path.exists(listFile): >+ f = open(listFile) >+ existing = [x.strip() for x in f.readlines()] >+ f.close() While you're blowing away all the blame here, how about you stick "from __future__ import with_statement" up top and refactor these file opens to use with statements? >+ else: >+ existing = [] >+ f = open(listFile, 'w') Just open the file after you put everything into entries, since you're not writing them one at a time. >+ for e in entries: >+ if e not in existing: >+ if first: >+ existing.insert(0, e) >+ else: >+ existing.append(e) >+ for e in existing: >+ f.write("%s\n" % e) You could just write them all in one shot: f.write("\n".join(entries) >+ f.close() >+ finally: >+ lock = None >+ >+if __name__ == '__main__': >+ if len(sys.argv) < 3: >+ print >>sys.stderr, "Usage: buildlist.py <list file> <entry> [<entry> ...]" >+ sys.exit(1) >+ parser = OptionParser() >+ parser.add_option("--first", dest="first", action="store_true") This could use a description of what it does. Also, I feel like maybe "--prepend" would make more sense here? (Nitpicky, I know.) Build the OptionParser before the len check above, and then check len(args) instead, and use parser.error for the error, ala: http://docs.python.org/library/optparse.html#generating-help http://docs.python.org/library/optparse.html#putting-it-all-together r=me with those fixed.
Attachment #476597 -
Flags: review?(ted.mielczarek) → review+
Comment 30•14 years ago
|
||
Heads up: I applied the first two patches and broke fennec. Apparently we have conflicting definitions of nsILoginManagerPrompter
Comment 31•14 years ago
|
||
Comment on attachment 476596 [details] [diff] [review] : Link XPTs in a sane way. >@@ -2357,5 +2344,8 @@ > libs export libs:: > $(CHECK_FROZEN_VARIABLES) > >+include $(topsrcdir)/toolkit/mozapps/installer/complink.mk >+ > default all:: > if test -d $(DIST)/bin ; then touch $(DIST)/bin/.purgecaches ; fi >+ $(MAKE) xpt-link This causes xpts to be linked right after packaging, which looks a bit odd.
Assignee | ||
Comment 32•14 years ago
|
||
(In reply to comment #30) > Heads up: I applied the first two patches and broke fennec. Apparently we have > conflicting definitions of nsILoginManagerPrompter If that's true, I'm kind of shocked that that works now.
Comment 33•14 years ago
|
||
(In reply to comment #32) > (In reply to comment #30) > > Heads up: I applied the first two patches and broke fennec. Apparently we have > > conflicting definitions of nsILoginManagerPrompter > > If that's true, I'm kind of shocked that that works now. It doesn't. Xptlinking fails with the same error. The packaging code continues anyway. For some reason, adding a packaging manifest to fennec magically makes the linking work, possibly by not copying the xpt we don't want.
Comment 34•14 years ago
|
||
is this ready to land if it has approval?
Comment 35•13 years ago
|
||
what's the status here? fennec supports package manifests, so we can remove the toolkit nsILoginManagerPrompter
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → INCOMPLETE
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•