bug#59707: 29.0.50; Seeking a more robust `package-quickstart'

[Matt Armstrong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> With this, emacs fails to launch, instead printing:
>>
>>     Symbol’s value as variable is void: geiser-active-implementations
>
> Did `--debug-init` provide a backtrace?

--debug-init did nothing insofar as I could tell.  Emacs behavior did
not observably change.  It prints the following to stderr and exits with
255:

    Symbol’s value as variable is void: geiser-active-implementations

In gdb I end up in a call to Fcommand_error_default_function with
SELECTED_FRAME()->glyphs_initialized_p false, so it takes the
Fkill_emacs(-1) path.  The trace below is easy for me to repro so feel
free to ask me to root around.

#0  __GI_exit (status=-1) at ./stdlib/exit.c:143
#1  0x000055555567ffab in Fkill_emacs (arg=Python Exception <class 
'gdb.error'>: value has been optimized out
, arg@entry=make_fixnum(-1), restart=XIL(0)) at emacs.c:2916
#2  0x00005555556884f1 in Fcommand_error_default_function (data=Python 
Exception <class 'gdb.error'>: value has been optimized out
, context=Python Exception <class 'gdb.error'>: value has been optimized out
, signal=XIL(0x2aaa9b653e40)) at /home/matt/git/e/emacs/src/lisp.h:758
#3  0x0000555555719217 in funcall_subr
    (subr=0x555555dcf620 <Scommand_error_default_function>, 
numargs=numargs@entry=3, args=args@entry=0x7ffff0dff050) at eval.c:3026
#4  0x0000555555756f03 in exec_byte_code (fun=Python Exception <class 
'gdb.error'>: value has been optimized out
, fun@entry=XIL(0x7ffff18fdfc5), args_template=args_template@entry=771, 
nargs=<optimized out>, nargs@entry=3, args=<optimized out>, 
    args@entry=0x7fffffffda18) at bytecode.c:809
#5  0x0000555555718aa6 in fetch_and_exec_byte_code
    (fun=fun@entry=XIL(0x7ffff18fdfc5), args_template=771, nargs=nargs@entry=3, 
args=args@entry=0x7fffffffda18) at eval.c:3069
#6  0x000055555571a165 in funcall_lambda
    (fun=fun@entry=XIL(0x7ffff18fdfc5), nargs=nargs@entry=3, 
arg_vector=arg_vector@entry=0x7fffffffda18) at eval.c:3141
#7  0x000055555571a593 in funcall_general (fun=XIL(0x7ffff18fdfc5), 
numargs=numargs@entry=3, args=args@entry=0x7fffffffda18) at eval.c:2933
#8  0x000055555571707c in Ffuncall (nargs=nargs@entry=4, 
args=args@entry=0x7fffffffda10) at eval.c:2983
#9  0x0000555555682726 in call3 (fn=Python Exception <class 'gdb.error'>: value 
has been optimized out
, arg1=Python Exception <class 'gdb.error'>: value has been optimized out
, arg1@entry=XIL(0x555555f74ef3), arg2=Python Exception <class 'gdb.error'>: 
value has been optimized out
, arg3=Python Exception <class 'gdb.error'>: value has been optimized out
) at /home/matt/git/e/emacs/src/lisp.h:3256
#10 0x0000555555689328 in cmd_error_internal 
(data=data@entry=XIL(0x555555f74ef3), context=context@entry=0x7fffffffda60 "") 
at keyboard.c:1005
#11 0x000055555568952e in cmd_error (data=XIL(0x555555f74ef3)) at keyboard.c:973
#12 0x0000555555715df3 in internal_condition_case 
(bfun=bfun@entry=0x5555556826c1 <top_level_2>, handlers=Python Exception <class 
'gdb.error'>: value has been optimized out
, hfun=hfun@entry=0x555555689363 <cmd_error>) at eval.c:1470
#13 0x00005555556825a1 in top_level_1 (ignore=Python Exception <class 
'gdb.error'>: value has been optimized out
, ignore@entry=XIL(0)) at keyboard.c:1142
#14 0x0000555555715d43 in internal_catch (tag=Python Exception <class 
'gdb.error'>: value has been optimized out
, func=func@entry=0x55555568256b <top_level_1>, arg=Python Exception <class 
'gdb.error'>: value has been optimized out
, arg@entry=XIL(0)) at eval.c:1197
#15 0x00005555556824d7 in command_loop () at keyboard.c:1102
#16 0x0000555555688e10 in recursive_edit_1 () at keyboard.c:712
#17 0x000055555568925c in Frecursive_edit () at keyboard.c:795
#18 0x0000555555681255 in main (argc=4, argv=<optimized out>) at emacs.c:2517

Lisp Backtrace:
"command-error-default-function" (0xf0dff050)
"help-command-error-confusable-suggestions" (0xffffda18)


>> The code for `geiser-activate-implementation' was originally:
>>
>>     (defsubst geiser-activate-implementation (impl)
>>       (add-to-list 'geiser-active-implementations impl))
>>
>> So the `(add-to-list 'geiser-active-implementations 'guile)' ended up
>> inlined into "package-quickstart.elc", which executed directly, without
>> first (auto) loading 'geiser-impl' as was intended.  Thus, the
>> `geiser-active-implementations' variable did not exist, yet was
>> referenced, by inlining, but did not appear textually in
>> package-quickstart.el.
>>
>> Once understood obvious fix to make geiser-activate-implementation a
>> defun instead of a defsubst.
>
> FWIW, that's a rather poor fix, since it causes `geiser-impl.el` (and
> the files it requires) to be unconditionally loaded during
> Emacs startup.  The better fix would be to make sure the
> `geiser-active-implementations` itself gets initialized before the calls
> to `geiser-activate-implementation`.

Yes, I agree that the fix is suboptimal -- it was the smallest diff that
made the code do what it clearly intended to do.

I'm not sure I like your suggestion to keep using the
`geiser-activeate-implementation' function, because I don't think it is
because calling functions from "foo" without loading "foo" first is
confusing, and works only if they are defsubst (or macros).  Is this
kind of thing commonly done in autoloads?  Seems quite subtle.

I'd rather suggest that the gesier package autoloads simply not use the
helper function at all.  Instead, just have the autoloads defvar
`geiser-active-implementations' and call `add-to-list' on it explicitly.
Seems simpler and unsurprising.


>> Ideas for improvement (also seeking ideas from others)
>> ========================================
>>
>> 0) Make `package-quickstart t' the default, because it is great, and
>> that way people tend to see the same kinds of problems.  :-)
>
> I think it's a good idea to do that on `master`, indeed.

Ok.


>> 1) Harden Emacs such that signaled errors from "package-quickstart.elc"
>> don't prevent startup (but are somehow saved and logged, maybe as
>> warnings?).
>
> Agreed.  I suspect it should also do things like delete the `.elc` file
> (and/or the `.el` file), or at least suggest doing it, so as to help
> diagnose/circumvent the problem.

Ok, let's keep it on the list of possibilities.


>> 2) Make --debug-init functional for errors thrown by
>> "package-quickstart.elc".  Currently, package-quickstart is loaded
>> before the first frame is initialized, so Emacs takes the "print the
>> error and exit" route for any signaled errors, instead of, for example,
>> ending up in the elisp debugger.
>
> In the case of `--debug-init` it could at least print the backtrace on
> `stderr`.  Or store the backtrace and display it later (even though the
> debugger wouldn't be active, it would still be nicer to manipulate than
> when sent to stderr).

Yes, though see the backtrace above.  It isn't very helpful, at least
for me, since the bottom of the lisp stack is the current
`command-error-function'.

Is this because the stack has already been unwound somehow?


>> 3) Perhaps package-quickstart.elc could be loaded later in the
>> initialization phase, after the first frame is created?
>
> I'm not in favor of such a change, no.
>
>> 5) Change "package-quickstart.el" code gen to provide more context for
>> errors, perhaps with `condition-case-unless-debug'.  The jwigley's
>> use-package package does something similar in its macro expansions and
>> it is nice.  In the specific case with the geiser packages, because
>> Emacs exited immediately, it would have been useful to know that the
>> error came from the "geiser-guile-autloads" portion of
>> "package-quickstart.elc".
>
> We could probably use `(setq load-file-name ...)` between file chunks
> (instead of `let` binding the var) and wrap the whole file with
> a `condition-case` which then prints the "current" `load-file-name`.

...adding to the list.  :)


>> 6) Print a warning when a `defsubst' function is autoloaded, as the
>> autoload won't work from byte compiled code.
>
> It works.  It just works differently (it copies the whole `defsubst`
> instead of placing an `autoload` statement).  AFAIK byte-compiled or
> not doesn't make much difference in this respect.

Ah, ok.  It looks like geiser's autoloads are hand written (and might
predate package.el), so an actual autoload for the defsubst ends up in
package-quickstart.el.  See
https://gitlab.com/emacs-geiser/geiser/-/blob/master/elisp/geiser.el#L104

So, there is an extra level of indirection there compared to what
presumably a "modern" emacs package would do.

Yet, geiser wants to support use directly from downloaded source.  What
is our current suggestion for packages that want to ship their own
pre-canned autoloads?  Do you know of a package supporting this without
resorting to hand edited autoloads?  I assume there is some canonical
'emacs -f batch_blah_blah' but, maybe not?