Closed
Bug 471188
Opened 14 years ago
Closed 14 years ago
stop calling "make install" for spidermonkey
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 1 obsolete file)
6.52 KB,
patch
|
benjamin
:
review+
jimb
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
Since bug 467583 landed, I don't think there's any reason to call "make install" in js/src anymore. All the binaries and libs should wind up in the right spots by default since we instruct the spidermonkey build to use the top-level dist directory. We will have to make js/src/Makefile.in use EXPORTS to get its headers into dist/include/js, but I think that's the only real change needed.
Assignee | ||
Comment 1•14 years ago
|
||
I think this might fix the underlying cause of bug 471097.
Assignee | ||
Comment 2•14 years ago
|
||
Also see bug 471097 comment 6
Comment 3•14 years ago
|
||
I think this is right. My original idea had been to make js/src and self-contained GNU-style subdirectory, since those are rules that are easy to implement and use. But Mozilla has a lot of expectations about how dist behaves (rebuilds are visible immediately; PGO data) that GNU behavior won't support. So there's no way to avoid having js/src/Makefile.in have two modes. It seems to me Mozilla mode should be enabled by --with-dist-dir.
Comment 4•14 years ago
|
||
That is, NSDISTMODE in js/src/Makefile should be affected by --with-dist-dir: when that flag is specified, NSDISTMODE should be left unset, to get the symlink behavior (relative on Linux; absolute on Darwin). When it's omitted or given as --without-dist-dir, NSDISTMODE should be set to copy, to get GNU behavior. Then, config/js/Makefile.in can be simplified as appropriate. There shouldn't be any need to set JS_MOZ_INSTALL. Actually, if config/js/build.mk can list js/src simply as an ordinary directory for the js tier, not a staticdir, then could config/js/Makefile.in go away entirely?
Assignee | ||
Comment 5•14 years ago
|
||
This patch removes config/js/Makefile.in, has config/js/build.mk simply set |tier_js_dirs = js/src|, and then tweaks js/src/Makefile.in a bit such that a normal make export/libs there should get everything into the place that the mozilla build expects it. I've thrown it at the try server to verify that it doesn't break anything stupid.
Assignee | ||
Comment 6•14 years ago
|
||
Yeah, this fails on the try server. I added EXPORTS to js/src/Makefile.in, but nsinstall isn't actually built yet there (since it gets built in js/src/config).
Comment 7•14 years ago
|
||
For in-mozilla use, shouldn't js/src just use the top-level nsinstall the way it used to? By the way, we should rename --with-dist-dir to --with-mozilla-dist-dir or something like that, to better indicate that this is the flag that changes the mode of the makefile from 'stand-alone' to 'in-mozilla'.
Assignee | ||
Comment 8•14 years ago
|
||
This patch builds fine on Win32/Linux/Mac. I removed config/js/Makefile.in entirely, cleaned up the install target in js/src to use SYSINSTALL, and added a slight hack to ensure that nsinstall gets built before it's needed in the export phase.
Attachment #354717 -
Attachment is obsolete: true
Attachment #355784 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #355784 -
Flags: review?(jim)
Comment 9•14 years ago
|
||
Comment on attachment 355784 [details] [diff] [review] don't call the install target, remove unnecessary stuff This looks fine to me. I'm glad to see all that code go away. If the reason for changing the $(SDK_PUBLIC) $(PUBLIC) rule in rules.mk to a single-colon rule is something other than "This should never have been a double-colon rule to begin with", could we get a comment for that?
Attachment #355784 -
Flags: review?(jim) → review+
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9) > (From update of attachment 355784 [details] [diff] [review]) > This looks fine to me. I'm glad to see all that code go away. > > If the reason for changing the $(SDK_PUBLIC) $(PUBLIC) rule in rules.mk to a > single-colon rule is something other than "This should never have been a > double-colon rule to begin with", could we get a comment for that? It was just to make the dependency on nsinstall in the Makefile work. I didn't see any reason that should have been a double colon rule to begin with. Do you really think that justifies a comment?
Comment 11•14 years ago
|
||
(In reply to comment #10) > It was just to make the dependency on nsinstall in the Makefile work. I didn't > see any reason that should have been a double colon rule to begin with. Do you > really think that justifies a comment? In that case, no. It's only if there was some obvious (just not to me) reason for it to be :: and a non-obvious reason to make it :. But I don't see why it should have been :: either.
Updated•14 years ago
|
Attachment #355784 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/3da64152f578
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 13•14 years ago
|
||
Confirmed no more multiple attempts at linking js/src. Also, no more js3250.lib and js-config in dist/bin. -V.FIXED
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 14•14 years ago
|
||
Comment on attachment 355784 [details] [diff] [review] don't call the install target, remove unnecessary stuff This is needed to fix bug 467271, which is blocking1.9.1+
Attachment #355784 -
Flags: approval1.9.1?
Comment 15•14 years ago
|
||
Comment on attachment 355784 [details] [diff] [review] don't call the install target, remove unnecessary stuff a191=beltzner
Attachment #355784 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 16•14 years ago
|
||
Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27a7287e4809
Keywords: fixed1.9.1
Comment 17•14 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 355784 [details] [diff] [review] [details]) > > This looks fine to me. I'm glad to see all that code go away. > > > > If the reason for changing the $(SDK_PUBLIC) $(PUBLIC) rule in rules.mk to a > > single-colon rule is something other than "This should never have been a > > double-colon rule to begin with", could we get a comment for that? > > It was just to make the dependency on nsinstall in the Makefile work. I didn't > see any reason that should have been a double colon rule to begin with. Do you > really think that justifies a comment? Ironically this caused my comm-central build to fail. My directory structure is as follows: C:\build\comm - hg clone of comm-central C:\build\mozilla - srcdir build of the 1.8 branch (contains dist) C:\build\objdir - objdir for comm-central (contains mozilla/dist) The problem arises when trying to export headers in a comm-central subdirectory 4 levels deep. In this case, $(PUBLIC) equals ../../../../mozilla/dist/include/$(MODULE) which should be C:\build\objdir\mozilla\dist\include\$(MODULE) however gmake discovers that it can resolve it against the VPATH (from rules.mk) of ../../../../mozilla/dist/lib i.e. C:\build\objdir\mozilla\dist\lib to make ../../../../mozilla/dist/lib/../../../../mozilla/dist/include/$(MODULE) which points into my 1.8 branch build :-(
Comment 18•13 years ago
|
||
(In reply to comment #16) > Pushed to 1.9.1: > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27a7287e4809 This push didn't change the install:: targets like the push to m-c did. This conflicts with a later patch of brendan's I'm trying to merge: -install:: $(SCRIPTS) $(PROGRAM) +install:: $(SCRIPTS) $(SYSINSTALL) $^ $(bindir) on the branch, it looks like this: install:: $(SCRIPTS) $(PROGRAM) $(SYSINSTALL) $(IFLAGS2) $^ $(bindir) what should be done?
Assignee | ||
Comment 19•13 years ago
|
||
Probably some merge fail. I believe the latter version (with $(IFLAGS2)) is the more correct one. Just change trunk to match? (r=me if you want it)
Updated•4 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•