With the patch from 433083 applied, the JavaScript shell built using the standard Mozilla build system (not Makefile.ref) doesn't provide line editing facilities (i.e. the stuff from js/src/editline). When building with Makefile.ref, the selected config/FOO.mk file indicates whether to build editline or not. In the main Mozilla build system, js/src/configure.in should make whatever tests are needed (termcap library, etc) and enable editline as appropriate. The editline library itself currently uses a Makefile.ref-style build system; it should use a standard Mozilla makefile.

Brian Crowder puts in a vote for a --enable-readline switch as well.

Note that this applies to the js/src/Makefile.ref build system, not the global Mozilla build system. We would like to use VPATH to find js.cpp once it moves into a 'shell' subdirectory. However, the rules changed by this patch use $* (the pattern stem) to generate their source filenames, not $<, which expands to the filename found in the VPATH.

Fixed with the tracemonkey merge to m-c I think?

Doesn't look like this was ever actually implemented. The patch here is not an actual fix. Am I missing something?

I keep trying ^P to edit the previous line and being sad... /be

I just went through all the bugs I merged in from T-M the other day, I assumed because the patch had landed that it was fixed, but that may well not be the case

Just some confusion, I think. Jim's patch here doesn't fix this bug.

That's right; that first patch is just a preliminary.

Not an enhancement, this is a regression whose fix would restore lost js shell interactive usability. Please fix, many will thank you. Can I help? /be

Ah, sorry. It is a regression. I thought this was a different bug.

robarnold reports that rlwrap is a usable workaround, and I agree! http://utopia.knoware.nl/~hlub/uck/rlwrap/ There's also a MacPorts port of it, and presumably at least a few distros have picked it up too.

Presumably jimb will not mind if I take this. Question: is it a problem if we just always link the js shell to readline if available, or build and link to editline otherwise?

This seems to do the trick. There's some ugliness in there, some of which will need to be resolved. Namely, js/src/Makefile.in sticks a lot of crap in EXTRA_LIBS, and then sticks all of EXTRA_LIBS into js-config for others to link to, which is sadness. I think all the junk that's currently being done with EXTRA_LIBS can be moved up into configure now, which will avoid this whole mess. This built fine on my mac, linux x86-64 box (without libreadline-dev installed), and on my windows box. Note that on Windows the whole library check section gets skipped, so we don't even try to build with editline support. (But MozillaBuild's bash seems to give me it for free anyway.)

This is less terrible. This patch creates js/src/shell and moves js.cpp there, but that makefile copies the js binary up one directory after building it. There's still a little bit of ugliness in here, namely how the shell finds the object files to link to, but that should be fixed in a different bug (perhaps bug 461996).

In order to preserve the location of the js executable regardless of build system (Makefile.ref vs. autoconf) I count on being able to create OBJ directories matching the old Makefile.ref directory names, and performing configure and make in the OBJ directories to place the obj and executable there. It seems that this patch will force configure and make to be performed in the js/src directory losing the freedom that configure gives you. Won't this also not allow you to build opt and debug versions of the shell at the same time?

This patch doesn't break configuring in a separate objdir, that would be unacceptable. All it does is build the shell in $(OBJDIR)/shell, and then copy it to $(OBJDIR).

(In reply to comment #12) > Question: is it a problem if we just always link the js shell to readline if > available, or build and link to editline otherwise? The default should probably be editline. Since readline is GPL, we can't ship binaries that are linked to it and it seems like we'd be making it too easy to accidentally build a GPL-tainted js shell.

Ok, so we should have an --enable-readline?

Yeah.

Then let it be so. One thing I have noticed about this patch is that it's going to require a clobber due to the awful $(wildcard ../*.o) bit. If there's a preexisting js.o in js/src (which there would be in a dep build), then you'll try to link them both and fail.

The try server didn't like this: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1228332930.1228337840.5956.gz Maybe we need to fix bug 461996 first. Or I can pull all the CPPSRCS out into a separate makefile and include it from both.

Comment on attachment 351227 [details] [diff] [review] with --enable-readline >+# People expect the js shell to wind up in the top-level JS dir. >+libs:: >+ cp $(PROGRAM) .. Please use $(INSTALL) $(IFLAGS2)

This depends on the patch that I'm going to attach to bug 467862.

Comment on attachment 352574 [details] [diff] [review] link to static lib This failed (again) on the Mac tryserver: /usr/bin/make -C ../../js/src install-runtime-libs libdir=../../dist/bin /builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/objdir/ppc/js/src/config/nsinstall -t -m 644 libjs_static.a ../../dist/bin /builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/objdir/ppc/js/src/config/nsinstall -t -m 755 libmozjs.dylib ../../dist/bin This is because the install-runtime-libs target in js/src/Makefile.in installs $(LIBRARY) if it's defined: http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#628 Jim: it feels like "install-runtime-libs" shouldn't be installing a static library. Can we tweak these rules to avoid this?

Oops, this was the actual error where it failed: /builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/build/macosx/universal/unify: copyIfIdentical: files differ: /builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/objdir/ppc/dist/firefox/Minefield.app/Contents/MacOS/libjs_static.a, /builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/objdir/i386/dist/firefox/Minefield.app/Contents/MacOS/libjs_static.a

Ok, I have an updated patch that works in Mac Universal builds, but now I'm failing on Windows. :-/ http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unix&logfile=1229288654.1229291015.9081.gz&buildtime=1229288654&buildname=Try%20server%20win32%20hg%20builder&fulltext=1

Ah, I was missing DEFINES+=-DEXPORT_JS_API from the Makefile. This builds on all three platforms on the try server.

I suspect this patch will break --enable-js-static-build, but looking at configure, I think it's probably already broken for non-win32 platforms, since we set: MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -lmozjs' unconditionally. bsmedberg: do you know if that build option actually still works? (Should I be concerned about keeping it working?)

The trick to avoid having install-runtime-libs install a static lib looks fine to me, but I think the comment should be kept close to the definition of install-runtime-libs. Is the below okay? diff --git a/js/src/Makefile.in b/js/src/Makefile.in --- a/js/src/Makefile.in +++ b/js/src/Makefile.in @@ -615,14 +615,16 @@ install:: $(INSTALLED_HEADERS) install:: $(SCRIPTS) $(PROGRAM) $(INSTALL) $(IFLAGS2) $^ $(bindir) +install:: $(LIBRARY) +ifneq (,$(LIBRARY)) + $(INSTALL) $(IFLAGS1) $(LIBRARY) $(libdir) +endif + # The Mozilla top-level makefiles use install-runtime-libs directly to # place an additional copy of the libraries in the 'dist/bin' # directory. install:: install-runtime-libs install-runtime-libs:: $(LIBRARY) $(SHARED_LIBRARY) $(IMPORT_LIBRARY) -ifneq (,$(LIBRARY)) - $(INSTALL) $(IFLAGS1) $(LIBRARY) $(libdir) -endif ifneq (,$(SHARED_LIBRARY)) $(INSTALL) $(IFLAGS2) $(SHARED_LIBRARY) $(libdir) endif

I believe that js-static-build is obsolete and no longer used... dougt, are you using it?

Jim: sounds reasonable, I'll fix that locally.

Actually, shouldn't IMPORT_LIBRARY fall in the install-only case, too? The .lib file doesn't belong in dist/bin. That is, install-runtime-libs should only deal with SHARED_LIBRARY.

Comment on attachment 353050 [details] [diff] [review] builds on the try server (In reply to comment #31) > I believe that js-static-build is obsolete and no longer used... dougt, are you > using it? The reality is that it looks broken anyway, and this patch enables a static JS library by default, so anyone relying on that code is no worse off than they used to be. I'll post an updated patch tomorrow that just removes js-static-build entirely, and fixes Jim's note from comment 33.

Ok, this just removes js-static-build entirely, and also fixes jimb's last note.

js/src/Makefile.in looks just right.

Pushed to m-c, you can pick this up in the next merge: http://hg.mozilla.org/mozilla-central/rev/55e23c647137 Note that you'll have to explicitly --enable-readline if you don't want to use editline.

This got backed out due to win32 build bustage.

This also breaks tracing print and other shell functions (see 471635).

This is updated to trunk, and also moves the defines of JS_TRACER and FEATURE_NANOJIT to configure from Makefile.in. This fixes bug 471635, and some of the other work done should fix the build bustage.

Relanded in m-c: http://hg.mozilla.org/mozilla-central/rev/19e319a0647b

Reopening: --enable-readline doesn't work on Linux: c++ -o js -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe -DDEBUG -D_DEBUG -DDEBUG_mrbkap -DTRACING -g -fno-inline js.o -lpthread -Wl,-rpath-link,/bin -Wl,-rpath-link,/lib -L../dist/bin -L../dist/lib -lreadline ../libjs_static.a -ldl -lm js.o: In function `GetLine': /home/mrbkap/work/js/mozilla/js/src/dbg-obj/shell/../../shell/js.cpp:153: undefined reference to `readline' /home/mrbkap/work/js/mozilla/js/src/dbg-obj/shell/../../shell/js.cpp:157: undefined reference to `add_history' /usr/bin/ld: js: hidden symbol `readline' isn't defined /usr/bin/ld: final link failed: Nonrepresentable section on output collect2: ld returned 1 exit status make[2]: *** [js] Error 1 I'm not sure why it doesn't work...

I bet that the readline header(s) need to be added to system-headers.

bsmedberg, this is a link error. We have the prototypes for the offending functions in js.cpp.

Figures. I tested --enable-readline on Mac, and I tested that it broke on Linux without readline installed, and I tested that editline worked on both, but I forgot to test --enable-readline on Linux. Anyway, trivial patch to fix visibility.

Pushed the followup to tracemonkey: http://hg.mozilla.org/tracemonkey/rev/a5dd39a8464a

Did all of this make it to 1.9.1, or just that followup patch?

oops, probably just that follow-up. Not a big deal, right? I don't think editline ever broke there.

(In reply to comment #49) > oops, probably just that follow-up. Not a big deal, right? I don't think > editline ever broke there. Diff js.cpp between trees for sanity? I'd be willing to read a diff -r of js/src in tm vs. 1.9.1, just for my sanity. /be

(In reply to comment #50) > (In reply to comment #49) > > oops, probably just that follow-up. Not a big deal, right? I don't think > > editline ever broke there. > > Diff js.cpp between trees for sanity? I'd be willing to read a diff -r of > js/src in tm vs. 1.9.1, just for my sanity. I did. It didn't have some Option and GC changes, which I think is fine, as Igor landed that later. I'll check it again, though.

sayrer: 1.9.1 has js-autoconf, which didn't provide editline at all.