Closed
Bug 924187
Opened 11 years ago
Closed 9 years ago
Write interfaces.manifest at config.status time
Categories
(Firefox Build System :: General, defect, P2)
Firefox Build System
General
Tracking
(firefox41 fixed)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: gps, Assigned: bokeefe)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
We currently use buildlist.py to write out interfaces.manifest. We should be able to write out interfaces.manifest at moz.build traversal time since it's simply derived from the set of active .xpt files.
This depends on having FINAL_TARGET available to moz.build files.
Reporter | ||
Comment 1•11 years ago
|
||
Brushing off the cobwebs on an old patch. This appears to shave 10-15s
off no-op builds because we eliminated a lot of buildlist invocations
during libs traversal! I went from ~1:17 no-op down to 1:02 on OS X.
The patch could use tests and needs some cleaning up.
Attachment #8372419 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → gps
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•11 years ago
|
||
I'm also not yet sure what the implication of adding chrome.manifest to the install manifest is. That might introduce a source of clobber needed because we'll fail to clean up entries from old builds. Will probably need to move chrome.manifest buildlist invocation into a Makefile.in.
Reporter | ||
Comment 3•11 years ago
|
||
Another idea is we can write out files with known manifest content at config.status time. At the top of the build, we just replace the existing manifests with the known content.
Reporter | ||
Comment 4•11 years ago
|
||
Raising priority since this decreases no-op build time by 20%.
Priority: -- → P2
Reporter | ||
Comment 5•11 years ago
|
||
This version improves on the component manifest handling. Python unit
tests are failing because a side-effect of this is adding FINAL_TARGET
to every sandbox. That may want to get addressed before final.
f? to get an opinion on the approach for dealing with manifests.
Attachment #8372769 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Updated•11 years ago
|
Attachment #8372419 -
Attachment is obsolete: true
Attachment #8372419 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Updated•10 years ago
|
Assignee: gps → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 6•10 years ago
|
||
I've had this sitting around for a while; time to do something about that. This is pretty similar to gps's patch, except for handling the chrome manifests.
I opted to leave the chrome manifests as a Makefile.in rule, although I moved it to the xpidl Makefile with the rest of those things. I had to use a FORCE dep to get them generated right, but that still ends up better than the current situation. Right now, they're processed once for every directory with an XPT_NAME, during the export tier. With this change, they're now processed just once per file, when the xpt files are written. As it turns out, there are only a handful that get written, so I wasn't too worried about that.
(this patch is on top of bug 1155816, but not for any reason other than that already being written)
Comment 7•9 years ago
|
||
Comment on attachment 8596747 [details] [diff] [review]
Deal with interfaces.manifest from the backend
>+ 'XPIDL_NO_MANIFEST': (bool, bool,
>+ """Indicate that the XPIDL module should not be added to a manifest.
>+
>+ This is a hacky flag that should not be used under ideal conditions.
>+ It exists to work around silliness related to netwerk/test/httpserver's
>+ packaging. Hacking around was determined to be easier than fixing
>+ packaging of that extension.
>+ """, None),
I realize this comment was from gps' patch, but why is XPIDL_NO_MANIFEST considered "hacky"? It looks like it's only used in the two test/example xpidl directories, not really as a hack.
Although the FORCE rule bugs me to no end, I realize that running buildlist ~90 times is definitely an improvement over the ~280 before this patch, and it seems to work the same otherwise as far as I can tell.
Attachment #8596747 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 8596747 [details] [diff] [review]
> Deal with interfaces.manifest from the backend
>
> >+ 'XPIDL_NO_MANIFEST': (bool, bool,
> >+ """Indicate that the XPIDL module should not be added to a manifest.
> >+
> >+ This is a hacky flag that should not be used under ideal conditions.
> >+ It exists to work around silliness related to netwerk/test/httpserver's
> >+ packaging. Hacking around was determined to be easier than fixing
> >+ packaging of that extension.
> >+ """, None),
>
> I realize this comment was from gps' patch, but why is XPIDL_NO_MANIFEST
> considered "hacky"? It looks like it's only used in the two test/example
> xpidl directories, not really as a hack.
I'm going to guess that's because all of netwerk/test/httpserver's packaging was a hack. In any case, I adjusted the explanation a bit.
> Although the FORCE rule bugs me to no end, I realize that running buildlist
> ~90 times is definitely an improvement over the ~280 before this patch, and
> it seems to work the same otherwise as far as I can tell.
Yeah, I wasn't a big fan of that either. There aren't all that many uses of buildlist left in makefiles, so maybe we can just get rid of them all. I'll see about doing that in another bug.
Attachment #8372769 -
Attachment is obsolete: true
Attachment #8596747 -
Attachment is obsolete: true
Attachment #8372769 -
Flags: feedback?(mh+mozilla)
Attachment #8598639 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
Try push from before I fixed the explanatory test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8148fcc121ec (this patch happens to have been the parent of that push; also, the orange was from an earlier version of bug 1155816).
Keywords: checkin-needed
Comment 10•9 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
This appears to have caused Linux PGO build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=9381821&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f39146b24ae
Flags: needinfo?(bokeefe)
Assignee | ||
Comment 12•9 years ago
|
||
I'm at a loss for how this broke only Linux PGO, but it's certainly guilty of that. It's failing generating the startup cache the second time through 'make package', so I'm assuming the maybe_clobber_profiledbuild step is deleting something it shouldn't be.
I'm poking at things to find out, but I don't expect it to be fast.
Flags: needinfo?(bokeefe)
Assignee | ||
Comment 13•9 years ago
|
||
It turns out that this breaks PGO because maybe_clobber_profiledbuild blows away the dist directory. If we stop doing that, Linux PGO builds work again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f795d13d4a
We apply the install manifests again when we build with the profile, so unknown files still get deleted. I'm pretty sure that doesn't break anything else, but an extra set of eyes couldn't hurt here.
Attachment #8598639 -
Attachment is obsolete: true
Attachment #8604854 -
Flags: review?(mshal)
Comment 14•9 years ago
|
||
I wouldn't be surprised if not blowing up dist changes the build in subtle ways.
Assignee | ||
Comment 15•9 years ago
|
||
I ended up comparing two objdirs (one that blows away dist, and one that doesn't) just to see what would happen. The dist dirs end up identical, except for:
- Not missing the interfaces.manifest files
- application.ini, because of the build ids
- The final binaries (firefox, plugin-container, xpcshell), presumably also because of build ids
Of course, that still doesn't preclude extra-subtle changes. I'll run a full set of tests on try, as an extra sanity check.
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Brian O'Keefe [:bokeefe] from comment #15)
> Of course, that still doesn't preclude extra-subtle changes. I'll run a full
> set of tests on try, as an extra sanity check.
Try looks happy with the results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3ae9d26ec50
Comment 17•9 years ago
|
||
Comment on attachment 8604854 [details] [diff] [review]
Deal with interfaces.manifest from the backend
I too am suspicious of the dist change breaking random things, but comparing your try results to some inbound PGO pushes doesn't show anything obviously different. I don't see any extra or missing files, and the file sizes are roughly the same. So hopefully we're luckier this time around...
Attachment #8604854 -
Flags: review?(mshal) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Keywords: checkin-needed
Comment 19•9 years ago
|
||
Comment 20•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•