471188 - stop calling "make install" for spidermonkey

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[(not currently active) Ted Mielczarek]

I think this might fix the underlying cause of bug 471097.

Comment 2

[(not currently active) Ted Mielczarek]

Also see bug 471097 comment 6

Comment 3

[Jim Blandy :jimb]

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

[Jim Blandy :jimb]

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?

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: don't call the install target in js/src

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.

Comment 6

[(not currently active) Ted Mielczarek]

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

[Jim Blandy :jimb]

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'.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: don't call the install target, remove unnecessary stuff

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.

Comment 9

[Jim Blandy :jimb]

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?

Comment 10

[(not currently active) Ted Mielczarek]

(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

[Jim Blandy :jimb]

(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.

Comment 12

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/3da64152f578

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Confirmed no more multiple attempts at linking js/src. Also, no more js3250.lib and js-config in dist/bin.

-V.FIXED

Comment 14

[(not currently active) Ted Mielczarek]

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+

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355784 [details] [diff] [review]
don't call the install target, remove unnecessary stuff

a191=beltzner

Comment 16

[(not currently active) Ted Mielczarek]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27a7287e4809

Comment 17

[[e-mail]]

(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

[Robert Sayre]

(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?

Comment 19

[(not currently active) Ted Mielczarek]

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)