Closed Bug 605133 Opened 9 years ago Closed 9 years ago

Sync js/src/configure.in with configure.in

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: paul.biggar, Assigned: paul.biggar)

References

(Depends on 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 files, 4 obsolete files)

People forget to sync their configure.in changes to js/src/configure.in, and I'm having trouble making fixing bug 580409 as a result.

Once synced, it would be nice if we could prevent them from going out of sync. Suggestions?
(In reply to comment #0)
> 
> Once synced, it would be nice if we could prevent them from going out of sync.
> Suggestions?

Does configure support file inclusion?
(In reply to comment #1)
> Does configure support file inclusion?

Yes. configure scripts are just shell scripts, and autoconf has its own inclusion thing. 

I don't think we'll be able to do this very simply with includes. autoconf likes things in a certain order, and processes from top to bottom. We'd have to split into quite a few files to do source inclusion, but it would probably be better than the status quo.

We could split configure.in into macros (which ted wanted to do anyway), and just call the appropriate ones from js/src/configure.
configure.in is processed by M4, and the output is a shell script. We have a bunch of stuff in aclocal.m4 that gets included:
http://mxr.mozilla.org/mozilla-central/source/aclocal.m4

You can certainly add new m4 files in build/autoconf and add them to aclocal.m4. The canonical way to do this would be to define macros in there and then use them in configure.in. Like, if you wanted to move all the --enable-jemalloc logic there, you could define a macro called MOZ_CHECK_JEMALLOC or something, and simply put that in configure.in in place of all the existing logic, which you'd refactor into the macro definition.

The only downside is that it hurts discoverability slightly, since it looks like a black box in configure.in, but MXR makes these things pretty easy to find.
Ted and I got on the phone and merged this by hand. This is the diff, which is untested, and needs some minor cosmetic changes.
This syncs configure.in and js/src/configure.in. Ted and I sat down and did this last week. It needed a few minor tryserver fixes, but is otherwise blessed by ted too.

After this, the difference between the two configure.ins is pretty minimal, and patches to configure.in should be pretty easy to apply to js/src/configure.in.
Assignee: general → pbiggar
Attachment #485313 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #486119 - Flags: review?
Attachment #486119 - Flags: review? → review?(jimb)
Ted, I'm looking through the patch and it seems we removed one set of rules for the symbian target from js/src/configure.in. Did we say we were leaving them in, removing them, or porting them to configure.in?
I believe the plan was to leave the Symbian stuff in the JS configure and not to port it anywhere. Perhaps we messed up on a hunk?
This reinstates the Symbian stuff, which we accidentally removed. I didn't test this change (except for running autoconf213), but I don't think it needs more than that.
Attachment #486119 - Attachment is obsolete: true
Attachment #486126 - Flags: review?(jimb)
Attachment #486119 - Flags: review?(jimb)
As far as I can tell HAVE_LINUX_PERF_EVENT_H is not used, either inside or outside of SpiderMonkey.

HAVE_TM_ZONE_TM_GMTOFF is only used within SpiderMonkey; it doesn't need to be added to the top-level configure.in.

You've added the difference:
++AC_SUBST(MOZ_THUMB2)
Why?  As far as I can tell, it's not checked by any Makefile. AC_SUBST arranges for MOZ_THUMB2 to get set in generated Makefiles, but not in js-confdefs.h.

Also, the AC_DEFINE(MOZ_THUMB2) has gone away. This is what #defines the C++ preprocessor symbol. It's tested in js/src/methodjit/MethodJIT.cpp, but you'd need an AC_DEFINE for it to be seen there.
Has the Try server had a chance to look at this?
(In reply to comment #10)
> Has the Try server had a chance to look at this?

Yes, all green/intermittent-orange.
(In reply to comment #9)
> As far as I can tell HAVE_LINUX_PERF_EVENT_H is not used, either inside or
> outside of SpiderMonkey.

It's used in js/src/Makefile.in. It was added in bug 568863, and the changeset from that bug seems to be largely intact.

We chose to sync it because it was a configure check, and so was harmless to include, and led to less difference between the files. One of the aims was to reduce differences between the files without good reason, to simplify maintenance.

> HAVE_TM_ZONE_TM_GMTOFF is only used within SpiderMonkey; it doesn't need to be
> added to the top-level configure.in.

Ditto.


> You've added the difference:
> ++AC_SUBST(MOZ_THUMB2)
> Why?  As far as I can tell, it's not checked by any Makefile. AC_SUBST arranges
> for MOZ_THUMB2 to get set in generated Makefiles, but not in js-confdefs.h.

Ditto.

 
> Also, the AC_DEFINE(MOZ_THUMB2) has gone away. This is what #defines the C++
> preprocessor symbol. It's tested in js/src/methodjit/MethodJIT.cpp, but you'd
> need an AC_DEFINE for it to be seen there.

And ditto I think.
Any thoughts on this approach to keeping the files in sync?

Basically, this makes c.in a nearly-proper subset of j/s/c.in, with non-JS parts commented out with dnlJS at the start of the line, and a comment as to why.

An alternative would be to duplicate the c.in comments, with a note that this is not used in JS.

(I imagine the maintenance of this file will be done using a side-by-side diffing tool like meld, kdiff3, vimdiff, etc, and these will simplify maintenance in that case).
(In reply to comment #12)
> (In reply to comment #9)
> > As far as I can tell HAVE_LINUX_PERF_EVENT_H is not used, either inside or
> > outside of SpiderMonkey.
> 
> It's used in js/src/Makefile.in. It was added in bug 568863, and the changeset
> from that bug seems to be largely intact.

I got bamboozled: it's an AC_SUBST (which *is* checked by makefiles), not an AC_DEFINE (for use by C++ code).  Indeed, it must stay.

> > HAVE_TM_ZONE_TM_GMTOFF is only used within SpiderMonkey; it doesn't need to be
> > added to the top-level configure.in.
> 
> Ditto.

I don't think the two configure.in files should aspire to be the same. The clients of the top-level configure have way more stuff they need to know about than those of js/src/configure.in. When the browser/toolkit and SpiderMonkey have unique needs, those checks should remain distinct.

> > You've added the difference:
> > ++AC_SUBST(MOZ_THUMB2)
> > Why?  As far as I can tell, it's not checked by any Makefile. AC_SUBST arranges
> > for MOZ_THUMB2 to get set in generated Makefiles, but not in js-confdefs.h.
> 
> Ditto.

It should be removed from both: this AC_SUBST sets a 'make' variable that is never used --- unless my grep is missing something?

> > Also, the AC_DEFINE(MOZ_THUMB2) has gone away. This is what #defines the C++
> > preprocessor symbol. It's tested in js/src/methodjit/MethodJIT.cpp, but you'd
> > need an AC_DEFINE for it to be seen there.

No, this is not correct.  AC_SUBST sets makefile variables; AC_DEFINE defines C++ preprocessor symbols; they are not unified or tied. The only client of this test is the preprocessor conditional in js/src/methodjit/MethodJIT.cpp, and thus it should be a js/src/configure.in-only test that performs an AC_DEFINE.
(In reply to comment #13)
> Created attachment 486501 [details] [diff] [review]
> Possibly nicer way to sync files
> 
> Any thoughts on this approach to keeping the files in sync?

This seems must messier than simply moving common portions of code out into .m4 files in build/autoconf, and adding 'include' calls to aclocal.m4.
(In reply to comment #14)

> I don't think the two configure.in files should aspire to be the same. The
> clients of the top-level configure have way more stuff they need to know about
> than those of js/src/configure.in. When the browser/toolkit and SpiderMonkey
> have unique needs, those checks should remain distinct.

Sure, I'm not proposing to drag Cairo checks or something into j/s/c.in. I think the aspiration is to make them easy to diff.


> (In reply to comment #12)
> > (In reply to comment #9)
> > > As far as I can tell HAVE_LINUX_PERF_EVENT_H is not used, either inside or
> > > outside of SpiderMonkey.
> I got bamboozled: it's an AC_SUBST (which *is* checked by makefiles), not an
> AC_DEFINE (for use by C++ code).  Indeed, it must stay.

OK, have removed it from configure.in.


> > > HAVE_TM_ZONE_TM_GMTOFF is only used within SpiderMonkey; it doesn't need to be
> > > added to the top-level configure.in.

OK, gone from configure.in.
 
> > > You've added the difference:
> > > ++AC_SUBST(MOZ_THUMB2)
> > > Why?  As far as I can tell, it's not checked by any Makefile. AC_SUBST arranges
> > > for MOZ_THUMB2 to get set in generated Makefiles, but not in js-confdefs.h.
> > 
> > Ditto.
> 
> It should be removed from both: this AC_SUBST sets a 'make' variable that is
> never used --- unless my grep is missing something?


It's used in autconf.mk.in. But I don't find it in any makefiles, so I guess it can be removed from there too. Do I understand this correctly?


> > > Also, the AC_DEFINE(MOZ_THUMB2) has gone away. This is what #defines the C++
> > > preprocessor symbol. It's tested in js/src/methodjit/MethodJIT.cpp, but you'd
> > > need an AC_DEFINE for it to be seen there.
> 
> No, this is not correct.  AC_SUBST sets makefile variables; AC_DEFINE defines
> C++ preprocessor symbols; they are not unified or tied. The only client of this
> test is the preprocessor conditional in js/src/methodjit/MethodJIT.cpp, and
> thus it should be a js/src/configure.in-only test that performs an AC_DEFINE.

(I think both these comments say the same thing?)

Yes, AC_DEFINE is needed in j/s/c.in. I would argue to add it to c.in, but you've been against that so far so I've left it out.
Attachment #486126 - Attachment is obsolete: true
Attachment #486578 - Flags: review?(jimb)
Attachment #486126 - Flags: review?(jimb)
(In reply to comment #16)
> > > > You've added the difference:
> > > > ++AC_SUBST(MOZ_THUMB2)
> > > > Why?  As far as I can tell, it's not checked by any Makefile. AC_SUBST arranges
> > > > for MOZ_THUMB2 to get set in generated Makefiles, but not in js-confdefs.h.
> > > 
> > > Ditto.
> > 
> > It should be removed from both: this AC_SUBST sets a 'make' variable that is
> > never used --- unless my grep is missing something?
> 
> 
> It's used in autconf.mk.in. But I don't find it in any makefiles, so I guess it
> can be removed from there too. Do I understand this correctly?

Yes, configure.in produces autoconf.mk by substituting the AC_SUBST'd variables into autoconf.mk.in.  The Makefiles include autoconf.mk, so they can see the values configure established for those variables.  So if something in autoconf.mk.in is never used in the makefiles, it can go away.

But I owe you an apology: my grepping missed this:

./media/libtremor/lib/Makefile.in:68:ifeq (arm_1_, $(TARGET_CPU)_$(GNU_CC)_$(MOZ_THUMB2))

(That was just introduced this Monday; perhaps I was using an old tree.)

So it seems best to have configure.in and js/src/configure.in handle MOZ_THUMB2 identically. One only needs the preprocessor symbol, and one only needs the Makefile variable, but since there's no opportunity for real simplification (i.e. pitching it altogether), we might as well just avoid differences.

As penance for going back and forth on this, I'll revise the patch myself.
Revised to also unify the MOZ_THUMB2 handling.
Attachment #486705 - Flags: review?(pbiggar)
Comment on attachment 486578 [details] [diff] [review]
Sync configure.in to js/src/configure.in, both ways, with fixes.

Has this bug been marked blocking? Do we have approval to land?
Attachment #486578 - Flags: review?(jimb) → review+
(In reply to comment #19)
> Comment on attachment 486578 [details] [diff] [review]
> Sync configure.in to js/src/configure.in, both ways, with fixes.
> 
> Has this bug been marked blocking? Do we have approval to land?

Not a chance. I was going to put the j.s.c.in parts into the tracemonkey tree. Should I also put c.in changes there and let it go through with sayrer's merge, or wait for the tree to open and put it in myself?
Attachment #486705 - Flags: review?(pbiggar) → review+
http://hg.mozilla.org/tracemonkey/rev/66f4a212edeb
Whiteboard: [fixed-in-tracemonkey]
This (as determined by local bisect) seems to have broken my ability to build 32-bit shell builds on Mac:

js/src/opt-obj-32% rm -rf * && ( cd .. && autoconf213 ) && env CC="gcc-4.2 -arch i386" CXX="g++-4.2 -arch i386" AR=ar CROSS_COMPILE=1 ../configure --enable-optimize --disable-debug --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-shark
....
invoking make to create js-config script
../configure: line 16054: js-config: command not found
(In reply to comment #22)
> This (as determined by local bisect) seems to have broken my ability to build
> 32-bit shell builds on Mac:
> 
> js/src/opt-obj-32% rm -rf * && ( cd .. && autoconf213 ) && env CC="gcc-4.2
> -arch i386" CXX="g++-4.2 -arch i386" AR=ar CROSS_COMPILE=1 ../configure
> --enable-optimize --disable-debug --target=i386-apple-darwin9.2.0
> --enable-macos-target=10.5 --enable-shark
> ....
> invoking make to create js-config script
> ../configure: line 16054: js-config: command not found


Sorry bout this. Backed out http://hg.mozilla.org/tracemonkey/rev/a0fe78e5b8fe.
(In reply to comment #22)
> This (as determined by local bisect) seems to have broken my ability to build
> 32-bit shell builds on Mac:
> 
> js/src/opt-obj-32% rm -rf * && ( cd .. && autoconf213 ) && env CC="gcc-4.2
> -arch i386" CXX="g++-4.2 -arch i386" AR=ar CROSS_COMPILE=1 ../configure
> --enable-optimize --disable-debug --target=i386-apple-darwin9.2.0
> --enable-macos-target=10.5 --enable-shark
> ....
> invoking make to create js-config script
> ../configure: line 16054: js-config: command not found

I can't replicate this, can you attach the configure log, and also the configure file? My version of configure doesnt have 16054 lines.
Whiteboard: [fixed-in-tracemonkey]
This fixes the error which caused the tree to burn, which was that MAKE is now replaced by GMAKE, which we hadn't updated. This passed tryserver because tryserver doesn't clobber on a change to js/src/configure.in, which is now the subject of bug 608696.

I've reduced this to just the js/src/configure.in part, and I'll land the configure.in part to mozilla-central separately. Doing them together might have created a nasty merge back to MC otherwise.
Attachment #486578 - Attachment is obsolete: true
Attachment #487354 - Flags: review?(jimb)
Depends on: 608769
Tryserver is done, this works.
Comment on attachment 487354 [details] [diff] [review]
Sync configure.in to js/src/configure.in (both ways) (fixed)

The only difference is the $MAKE -> $GMAKE?  Looks good to me.
Attachment #487354 - Flags: review?(jimb) → review+
http://hg.mozilla.org/tracemonkey/rev/b1094f628602
Whiteboard: fixed-in-tracemonkey
Depends on: 612809
http://hg.mozilla.org/mozilla-central/rev/b1094f628602
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 643167
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.