Closed
Bug 1255485
Opened 8 years ago
Closed 6 years ago
Build binaries directly in their FINAL_TARGET location
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: gw280, Assigned: ted)
References
(Blocks 2 open bugs)
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chmanchester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
I had a weird bug that I was dealing with where I had added a new GeckoProgram target in a directory where Makefile.in didn't state NSDISTMODE=copy. As a result, I got a bunch of really weird XPCOM initialisation failures when trying to launch the resulting binary. Apparently this is a known issue when an executable is symlinked rather than copied? I think that GeckoProgram should imply copying rather than symlinking.
Reporter | ||
Updated•8 years ago
|
Component: mach → Build Config
Assignee | ||
Comment 1•8 years ago
|
||
This would let us remove all the hardcoded `NSDISTMODE = copy` bits in Makefile.in.
Comment 2•8 years ago
|
||
I think the right thing to do is to link the programs in their final destination directly instead of linking them in the objdir and then copying/symlinking them. The only thing of note is that on windows, that means the pdb would go alongside, but there's a flag to make the linker put the pdb elsewhere.
Comment 3•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2) > I think the right thing to do is to link the programs in their final > destination directly instead of linking them in the objdir and then > copying/symlinking them. The only thing of note is that on windows, that > means the pdb would go alongside, but there's a flag to make the linker put > the pdb elsewhere. I agree. I actually thought about this as I was working on bug 1253436. The fact we can produce JSON describing binaries means the backend has enough knowledge to do this today.
Updated•7 years ago
|
Blocks: nomakefiles, buildtup
Assignee | ||
Comment 4•7 years ago
|
||
I was worried that this would be hard because we used to clobber dist/bin at the beginning of the build, but I guess we don't do that anymore, so this should be very doable.
Assignee: nobody → ted
Assignee | ||
Updated•7 years ago
|
Summary: GeckoProgram should always imply NSDISTMODE = copy → Build binaries directly in their FINAL_TARGET location
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d2bb9f378757eec2682cdc9e9f7f449d3585462
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review207560 You're right, that was surprisingly pleasant. ::: config/rules.mk:815 (Diff revision 1) > ifeq (,$(filter 1,$(MOZ_AUTOMATION_BUILD_SYMBOLS))) > DUMP_SYMS_TARGETS := > endif > endif > > -$(foreach file,$(DUMP_SYMS_TARGETS),$(eval $(call syms_template,$(file),$(file)_syms.track))) > +$(foreach file,$(DUMP_SYMS_TARGETS),$(eval $(call syms_template,$(file),$(notdir $(file))_syms.track))) Consider pushing this stripping up to the definition of DUMP_SYMS_TARGETS, since it's not 100% clear that it'll do the right thing for the other components.
Attachment #8931097 -
Flags: review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8931098 [details] bug 1255485 - Remove NSDISTMODE=copy from Makefiles. https://reviewboard.mozilla.org/r/202180/#review207564 Stamp!
Attachment #8931098 -
Flags: review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8931099 [details] bug 1255485 - Remove NSDISTMODE=copy support from config.mk. https://reviewboard.mozilla.org/r/202182/#review207566 Stamp!
Attachment #8931099 -
Flags: review+
Updated•6 years ago
|
Attachment #8931097 -
Flags: review?(core-build-config-reviews)
Attachment #8931098 -
Flags: review?(core-build-config-reviews)
Attachment #8931099 -
Flags: review?(core-build-config-reviews)
Comment 12•6 years ago
|
||
Doesn't this result in the pdbs being created in dist/bin too? I remember that was one of the problems I had when I tried this a while ago.
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12) > Doesn't this result in the pdbs being created in dist/bin too? I remember > that was one of the problems I had when I tried this a while ago. It might, I'll have to check. That's fixable.
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9d2bb9f378757eec2682cdc9e9f7f449d3585462 I apparently chose a bad inbound changeset for this, because all my Windows builds are busted. I rebased it and it builds fine on Windows locally. (In reply to Mike Hommey [:glandium] from comment #12) > Doesn't this result in the pdbs being created in dist/bin too? I remember > that was one of the problems I had when I tried this a while ago. No, it turns out we already set LINK_PDBFILE to be just the basename of the file, so that works OK: https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/config/rules.mk#166 https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/config/rules.mk#557 However, after doing a local Windows build I notice that I do wind up with .ilk and .lib files in dist/bin, so I'm going to see if it's possible to make those go elsewhere. (I'm actually sorta confused as to why we wind up with .lib files for binaries, TBH!)
Assignee | ||
Comment 15•6 years ago
|
||
We apparently get .lib and .exp files generated because we must have functions that are __declspec(dllexport) that wind up in firefox.exe. (I don't know if that's intentional or not.) I should be able to make those not wind up in dist/bin. Unfortunately there's no way to control the location of the .ilk files, which are used for incremental linking, so we're going to be stuck with those. They won't be produced in most automation builds, since we use compiler options that disable incremental linking, so it hopefully shouldn't cause any issues even if we want to make packaging better in the future.
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review207560 > Consider pushing this stripping up to the definition of DUMP_SYMS_TARGETS, since it's not 100% clear that it'll do the right thing for the other components. I looked into doing this but I can't because `syms_template` does need the full path to the .exe file so it can pass that to dump_syms.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
One thing I still need to fix--on Windows this breaks symbol dumping for programs because the exe and pdb files are no longer in the same directory. This should be fixable, the full path to the pdb file is in fact in the exe headers.
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8932082 [details] bug 1255485 - force import libraries to be generated in objdir, not dist/bin. https://reviewboard.mozilla.org/r/203138/#review211156
Attachment #8932082 -
Flags: review+
Updated•6 years ago
|
Attachment #8932082 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 23•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adda21323a1ee77e8dc062e6dccffbd2c23625bd
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21) > One thing I still need to fix--on Windows this breaks symbol dumping for > programs because the exe and pdb files are no longer in the same directory. > This should be fixable, the full path to the pdb file is in fact in the exe > headers. The latest try push contains a simple patch atop the existing patches here that should fix this issue. If that works then it should be easy to get that reviewed and land all of these patches!
Assignee | ||
Comment 25•6 years ago
|
||
OK that try push exposed two more issues: 1) I failed to fix the integration test in unit-symbolstore.py to match the change to locate pdb files, so that test failed. 2) On Mac builds, some programs wind up using DIST_SUBDIR to get installed inside an app bundle, but nothing was ensuring that that subdirectory was created before linking happened (I added it as a prerequisite to the PROGRAM rule in rules.mk): https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/toolkit/crashreporter/minidump-analyzer/moz.build#28
Assignee | ||
Comment 26•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a66832e441ca76bfb2b9d3eb9ae814423e42d44
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
At least for building programs into dist/bin, it would be splendid if there were some tests for this new behavior.
Assignee | ||
Comment 32•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88d3b062723ea33c9476fdb0ce01cbba8b8540f6
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #31) > At least for building programs into dist/bin, it would be splendid if there > were some tests for this new behavior. Good call. I wrote a frontend test and a recursive make backend test to check the various things that can impact the program destination. I pushed to try again to make sure the tests pass on all platforms.
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review220286 OK, I re-reviewed this. It looks fine to me, and my questions are truly questions and not blockers. Green try is good to go! ::: browser/app/Makefile.in:49 (Diff revision 3) > > ifneq (,$(filter-out WINNT,$(OS_ARCH))) > > ifdef COMPILE_ENVIRONMENT > libs:: > - cp -p $(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX) > + cp -p $(DIST)/bin/$(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX) Huh. Isn't this saying that we're _not_ actually installing into the correct place? Can we not fold this into the program declaration and avoid this copy? (This could be follow-up.) ::: config/rules.mk:555 (Diff revision 3) > > # > # PROGRAM = Foo > # creates OBJS, links with LIBS to create Foo > # > -$(PROGRAM): $(PROGOBJS) $(STATIC_LIBS_DEPS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) > +$(PROGRAM): $(PROGOBJS) $(STATIC_LIBS_DEPS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $(call mkdir_deps,$(FINAL_TARGET)) I know you noted why you're doing this, but it's special and opaque. Can we not determine the directory from `$(PROGRAM)`? We definitely produce some executables that aren't in `dist/bin`, at least on macOS (crashreporter?)
Attachment #8931097 -
Flags: review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8943303 [details] bug 1255485 - add some tests for building programs in dist/bin. https://reviewboard.mozilla.org/r/213608/#review220292 Looks great.
Attachment #8943303 -
Flags: review+
Assignee | ||
Comment 37•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review220286 > Huh. Isn't this saying that we're _not_ actually installing into the correct place? Can we not fold this into the program declaration and avoid this copy? > > (This could be follow-up.) This is the horrible "we used to ship `firefox-bin` as the binary, and `firefox` as a shell script, then we made `firefox` the binary, but lots of people still expect to invoke `firefox-bin` so we just ship a copy named that" thing. It would be nice to get rid of this but it doesn't feel very high priority to me. > I know you noted why you're doing this, but it's special and opaque. Can we not determine the directory from `$(PROGRAM)`? We definitely produce some executables that aren't in `dist/bin`, at least on macOS (crashreporter?) We could, sure, but for programs that don't get dist installed at all it'd just be `.`. For things like that, the `FINAL_TARGET` is correctly set to that path: https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/toolkit/crashreporter/minidump-analyzer/moz.build#28 (this was what broke and the reason I need this `mkdir_deps`). Surprisingly the crashreporter client doesn't set `FINAL_TARGET`, it just uses the same ugly Makefile rules that browser/app does to build its app bundle (bummer).
Assignee | ||
Updated•6 years ago
|
Attachment #8931097 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Updated•6 years ago
|
Attachment #8943303 -
Flags: review?(core-build-config-reviews)
Comment 38•6 years ago
|
||
Pushed by tmielczarek@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/51af06b6123c build PROGRAMs directly in dist/bin instead of copying them. r=nalexander https://hg.mozilla.org/integration/autoland/rev/0ba36c0feddb Remove NSDISTMODE=copy from Makefiles. r=nalexander https://hg.mozilla.org/integration/autoland/rev/ed6dd4aefadb Remove NSDISTMODE=copy support from config.mk. r=nalexander https://hg.mozilla.org/integration/autoland/rev/5672cf8d324b force import libraries to be generated in objdir, not dist/bin. r=chmanchester https://hg.mozilla.org/integration/autoland/rev/90b7449882b6 add some tests for building programs in dist/bin. r=nalexander
Comment 39•6 years ago
|
||
Backed out 5 changesets (bug 1255485) for mochitest failures on test/mochitest/test_hangui.xul Backout: https://hg.mozilla.org/integration/autoland/rev/336ec3ad9df4be2aab93580018a50270173439dc Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=90b7449882b65a5f16e75c58106f6cf153785f2f Log: https://treeherder.mozilla.org/logviewer.html#?job_id=157844683&repo=autoland&lineNumber=3566 23:42:56 INFO - Buffered messages finished 23:42:56 ERROR - 297 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_hangui.xul | Test timed out. 23:42:56 INFO - reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
Flags: needinfo?(ted)
Updated•6 years ago
|
Product: Core → Firefox Build System
Comment 40•6 years ago
|
||
I stumbled on a fix for this that looks good on try, will upload shortly.
Updated•6 years ago
|
Flags: needinfo?(ted)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8959720 [details] Bug 1255485 - Don't assume target path is srcdir relative when locating a program's manifest on windows. https://reviewboard.mozilla.org/r/228476/#review234380
Attachment #8959720 -
Flags: review+
Assignee | ||
Comment 43•6 years ago
|
||
Comment on attachment 8959720 [details] Bug 1255485 - Don't assume target path is srcdir relative when locating a program's manifest on windows. Thanks for fixing this! I keep meaning to file a bug, we should really rip out most of that manifest-embedding code and simplify it.
Attachment #8959720 -
Flags: review?(core-build-config-reviews)
Comment 44•6 years ago
|
||
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad09b92fb875 build PROGRAMs directly in dist/bin instead of copying them. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/6fffc6fbb8c9 Remove NSDISTMODE=copy from Makefiles. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/5888578c22f8 Remove NSDISTMODE=copy support from config.mk. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/465358fef0d2 force import libraries to be generated in objdir, not dist/bin. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/02e71426ed3e add some tests for building programs in dist/bin. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/7862033a42ab Don't assume target path is srcdir relative when locating a program's manifest on windows. r=ted
Comment 45•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad09b92fb875 https://hg.mozilla.org/mozilla-central/rev/6fffc6fbb8c9 https://hg.mozilla.org/mozilla-central/rev/5888578c22f8 https://hg.mozilla.org/mozilla-central/rev/465358fef0d2 https://hg.mozilla.org/mozilla-central/rev/02e71426ed3e https://hg.mozilla.org/mozilla-central/rev/7862033a42ab
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•