JavaScript shell should provide line editing facilities

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jimb, Assigned: ted)

Tracking

Trunk
mozilla1.9.2a1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

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.
Severity: normal → enhancement
Blocks: 462542
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.
Attachment #346526 - Flags: review?(ted.mielczarek)
Attachment #346526 - Flags: review?(ted.mielczarek) → review+
Fixed with the tracemonkey merge to m-c I think?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
That's right; that first patch is just a preliminary.
Assignee: general → jim
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
Severity: enhancement → major
Flags: wanted1.9.1?
Ah, sorry. It is a regression. I thought this was a different bug.
Flags: wanted1.9.1? → wanted1.9.1+
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?
Assignee: jim → ted.mielczarek
Posted patch a first pass (obsolete) — Splinter Review
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.)
Posted patch a less terrible patch (obsolete) — Splinter Review
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).
Attachment #351174 - Attachment is obsolete: true
Attachment #351191 - Flags: review?(benjamin)
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?
Posted patch with --enable-readline (obsolete) — Splinter Review
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.
Attachment #351191 - Attachment is obsolete: true
Attachment #351227 - Flags: review?(benjamin)
Attachment #351191 - Flags: review?(benjamin)
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)
Attachment #351227 - Flags: review?(benjamin) → review+
Depends on: 467862
Posted patch link to static lib (obsolete) — Splinter Review
This depends on the patch that I'm going to attach to bug 467862.
Attachment #351227 - Attachment is obsolete: true
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
Status: REOPENED → ASSIGNED
Posted patch builds on the try server (obsolete) — Splinter Review
Ah, I was missing DEFINES+=-DEXPORT_JS_API from the Makefile. This builds on all three platforms on the try server.
Attachment #353050 - Flags: review?(benjamin)
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.
Attachment #353050 - Flags: review?(benjamin)
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.
Attachment #353016 - Attachment is obsolete: true
Attachment #353050 - Attachment is obsolete: true
Attachment #354191 - Flags: review?(benjamin)
js/src/Makefile.in looks just right.
Attachment #354191 - Flags: review?(benjamin) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
This got backed out due to win32 build bustage.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This also breaks tracing print and other shell functions (see 471635).
Depends on: 471635
Posted patch updatedSplinter Review
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.
Attachment #354191 - Attachment is obsolete: true
Relanded in m-c:
http://hg.mozilla.org/mozilla-central/rev/19e319a0647b
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #357131 - Flags: review?(benjamin)
Attachment #357131 - Flags: review?(benjamin) → review+
Pushed the followup to tracemonkey:
http://hg.mozilla.org/tracemonkey/rev/a5dd39a8464a
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
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.
Keywords: fixed1.9.1
(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.
Depends on: 505771
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.