Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

[Stefan Monnier]

Hi Lars,

> --- a/lisp/emacs-lisp/bytecomp.el
> +++ b/lisp/emacs-lisp/bytecomp.el
> @@ -124,7 +124,7 @@
>  (require 'backquote)
>  (require 'macroexp)
>  (require 'cconv)
> -(eval-when-compile (require 'compile))
> +(require 'compile)

This is a bad change.

It brings in many more packages, and every additional package brought
in reduces the quality of our warnings (e.g. if any of the packages
(transitively) loaded load cl-lib, then we fail to signal when
a package uses cl-lib functionality without requiring cl-lib).

We should try and find some other solution.


        Stefan

Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

[Lars Ingebrigtsen]

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

>> -(eval-when-compile (require 'compile))
>> +(require 'compile)
>
> This is a bad change.
>
> It brings in many more packages, and every additional package brought
> in reduces the quality of our warnings (e.g. if any of the packages
> (transitively) loaded load cl-lib, then we fail to signal when
> a package uses cl-lib functionality without requiring cl-lib).

I'm not quite sure I followed the last bit -- if something transitively
required cl-lib, then there's no point for the "top" file to require
cl-lib, surely?  Or is this a style issue.

But I agree that requiring compile.el runtime is probably not good, but:

> We should try and find some other solution.

No other solution I thought of at the top of my head seemed good.
Moving either of those keymaps to a different place seemed logical,
either...

The problem is that the mode now inherits from the parent defined in
compile.el:

(defvar emacs-lisp-compilation-mode-map
  (let ((map (make-sparse-keymap)))
    (set-keymap-parent map compilation-minor-mode-map)
    (define-key map "g" 'emacs-lisp-compilation-recompile)
    map))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

[Stefan Monnier]

> I'm not quite sure I followed the last bit -- if something transitively
> required cl-lib, then there's no point for the "top" file to require
> cl-lib, surely?  Or is this a style issue.

If a file (transitively) required by bytecomp.el requires cl-lib,
then when compiling *another* file any missing (require 'cl-lib) in that
other file won't be noticed.

> (defvar emacs-lisp-compilation-mode-map
>   (let ((map (make-sparse-keymap)))
>     (set-keymap-parent map compilation-minor-mode-map)
>     (define-key map "g" 'emacs-lisp-compilation-recompile)
>     map))

Do we need this set-keymap-parent?  define-derived-mode should do it for
us (when the mode is called).


        Stefan

Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

[Lars Ingebrigtsen]

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

>> I'm not quite sure I followed the last bit -- if something transitively
>> required cl-lib, then there's no point for the "top" file to require
>> cl-lib, surely?  Or is this a style issue.
>
> If a file (transitively) required by bytecomp.el requires cl-lib,
> then when compiling *another* file any missing (require 'cl-lib) in that
> other file won't be noticed.

Oh, this is just special for bytecomp.el; I get it.

>> (defvar emacs-lisp-compilation-mode-map
>>   (let ((map (make-sparse-keymap)))
>>     (set-keymap-parent map compilation-minor-mode-map)
>>     (define-key map "g" 'emacs-lisp-compilation-recompile)
>>     map))
>
> Do we need this set-keymap-parent?  define-derived-mode should do it for
> us (when the mode is called).

I didn't know that.  I've now made the change (and reverted the
`require'), and this do indeed work as they should.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [Emacs-diffs] master 28b7dd4 2/2: Fix build error in bytecomp.el from previous change

[Sven Joachim]

On 2019-10-01 11:08 -0400, Stefan Monnier wrote:

> Hi Lars,
>
>> --- a/lisp/emacs-lisp/bytecomp.el
>> +++ b/lisp/emacs-lisp/bytecomp.el
>> @@ -124,7 +124,7 @@
>>  (require 'backquote)
>>  (require 'macroexp)
>>  (require 'cconv)
>> -(eval-when-compile (require 'compile))
>> +(require 'compile)
>
> This is a bad change.
>
> It brings in many more packages, and every additional package brought
> in reduces the quality of our warnings (e.g. if any of the packages
> (transitively) loaded load cl-lib, then we fail to signal when
> a package uses cl-lib functionality without requiring cl-lib).

Indeed, this is bug #30635, mentioned in the comments you had written
just around Lars' changes:

,----
| ;; Refrain from using cl-lib at run-time here, since it otherwise prevents
| ;; us from emitting warnings when compiling files which use cl-lib without
| ;; requiring it! (bug#30635)
`----

> We should try and find some other solution.

Cheers,
       Sven