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)

defect

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: gps, Assigned: bokeefe)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Blocks: 929147
Depends on: 948278
Attached patch Write interfaces.manifest during config.status (obsolete) (deleted) — Splinter Review
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)
Assignee: nobody → gps
Status: NEW → ASSIGNED
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.
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.
Raising priority since this decreases no-op build time by 20%.
Priority: -- → P2
Depends on: 969744
Attached patch Write interfaces.manifest during config.status (obsolete) (deleted) — Splinter Review
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)
Attachment #8372419 - Attachment is obsolete: true
Attachment #8372419 - Flags: feedback?(mh+mozilla)
Assignee: gps → nobody
Status: ASSIGNED → NEW
Attached patch Deal with interfaces.manifest from the backend (obsolete) (deleted) — Splinter Review
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)
Assignee: nobody → bokeefe
Status: NEW → ASSIGNED
Attachment #8596747 - Flags: review?(mshal)
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+
(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+
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
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)
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)
I wouldn't be surprised if not blowing up dist changes the build in subtle ways.
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.
(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 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+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: