Re: master 5183123: Add Tramp recompilation

Re: master 5183123: Add Tramp recompilation

[Basil L. Contovounesios]

Michael.Albinus@gmx.de (Michael Albinus) writes:

> diff --git a/lisp/net/tramp-cmds.el b/lisp/net/tramp-cmds.el
> index d208f0e..6342cf5 100644
> --- a/lisp/net/tramp-cmds.el
> +++ b/lisp/net/tramp-cmds.el
> @@ -472,6 +472,48 @@ For details, see `tramp-rename-files'."
>  (function-put
>   #'tramp-rename-these-files 'completion-predicate #'tramp-command-completion-p)
>  
> +;; This function takes action since Emacs 28.1, when
> +;; `read-extended-command-predicate' is set to
> +;; `command-completion-default-include-p'.
> +;;;###tramp-autoload
> +(defun tramp-recompile-elpa-command-completion-p (_symbol _buffer)
> +  "A predicate for `tramp-recompile-elpa'.
> +It is completed by \"M-x TAB\" only if package.el is loaded, and
> +Tramp is an installed ELPA package."
> +  ;; We cannot apply `package-installed-p', this would also return the
> +  ;; builtin package.
> +  (tramp-compat-funcall 'package--user-installed-p 'tramp))

It looks like package--user-installed-p isn't currently designed to be
used in this way:

0. emacs -Q
1. (progn
     (require 'package)
     (require 'tramp)
     (setq read-extended-command-predicate
           #'command-completion-default-include-p)
     (read-extended-command))
2. C-j C-i

Debugger entered--Lisp error: (wrong-type-argument package-desc nil)
  signal(wrong-type-argument (package-desc nil))
  package--user-installed-p(tramp)
  tramp-recompile-elpa-command-completion-p(tramp-recompile-elpa
                                            #<buffer *scratch*>)
  command-completion-default-include-p(tramp-recompile-elpa
                                       #<buffer *scratch*>)

Should package--user-installed-p simply guard against a nil package
descriptor?

Thanks,

-- 
Basil

Re: master 5183123: Add Tramp recompilation

[Stefan Monnier]

>> +;; This function takes action since Emacs 28.1, when
>> +;; `read-extended-command-predicate' is set to
>> +;; `command-completion-default-include-p'.
>> +;;;###tramp-autoload
>> +(defun tramp-recompile-elpa-command-completion-p (_symbol _buffer)
>> +  "A predicate for `tramp-recompile-elpa'.

Who would need to run `tramp-recompile-elpa` via `M-x`, really?
My vote is to just make it non-interactive and move on.
[ Also, I don't see why we need `tramp-recompile-elpa`.
  I do see a need for `M-x package-recompile`, OTOH.  ]


        Stefan

Re: master 5183123: Add Tramp recompilation

[Michael Albinus]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

Hi Basil,

> It looks like package--user-installed-p isn't currently designed to be
> used in this way:
>
> 0. emacs -Q
> 1. (progn
>      (require 'package)
>      (require 'tramp)
>      (setq read-extended-command-predicate
>            #'command-completion-default-include-p)
>      (read-extended-command))
> 2. C-j C-i
>
> Debugger entered--Lisp error: (wrong-type-argument package-desc nil)
>   signal(wrong-type-argument (package-desc nil))
>   package--user-installed-p(tramp)
>   tramp-recompile-elpa-command-completion-p(tramp-recompile-elpa
>                                             #<buffer *scratch*>)
>   command-completion-default-include-p(tramp-recompile-elpa
>                                        #<buffer *scratch*>)
>
> Should package--user-installed-p simply guard against a nil package
> descriptor?

That would be fine. But since Tramp must be backward compatible, I have
added a check in tramp-recompile-elpa-command-completion-p.

> Thanks,

Best regards, Michael.

Re: master 5183123: Add Tramp recompilation

[Michael Albinus]

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

Hi Stefan,

>>> +;; This function takes action since Emacs 28.1, when
>>> +;; `read-extended-command-predicate' is set to
>>> +;; `command-completion-default-include-p'.
>>> +;;;###tramp-autoload
>>> +(defun tramp-recompile-elpa-command-completion-p (_symbol _buffer)
>>> +  "A predicate for `tramp-recompile-elpa'.
>
> Who would need to run `tramp-recompile-elpa` via `M-x`, really?

People who install Tramp's GNU ELPA package.

> My vote is to just make it non-interactive and move on.

This command is triggered by error report(s) on the Tramp ML. Scenario:

- Run an older Emacs version, say 27.2.
- Load tramp.el (this happens often, implicitly)
- Install Tramp 2.5.0.3 from GNU ELPA

There is a compilation error, because the already loaded tramp-compat.el
does not fit the expectations of Tramp 2.5.0.3.

> [ Also, I don't see why we need `tramp-recompile-elpa`.
>   I do see a need for `M-x package-recompile`, OTOH.  ]

D'accord.

>         Stefan

Best regards, Michael.

Re: master 5183123: Add Tramp recompilation

[Stefan Monnier]

>> My vote is to just make it non-interactive and move on.
> This command is triggered by error report(s) on the Tramp ML. Scenario:
> - Run an older Emacs version, say 27.2.
> - Load tramp.el (this happens often, implicitly)
> - Install Tramp 2.5.0.3 from GNU ELPA
>
> There is a compilation error, because the already loaded tramp-compat.el
> does not fit the expectations of Tramp 2.5.0.3.

I think we should treat this is a bug in package.el: we already have
code to try and avoid such problems, so apparently it doesn't do its job
in this case.

>> [ Also, I don't see why we need `tramp-recompile-elpa`.
>>   I do see a need for `M-x package-recompile`, OTOH.  ]
> D'accord.

Patches warmly welcome,


        Stefan

Re: master 5183123: Add Tramp recompilation

[Michael Albinus]

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

Hi Stefan,

>> This command is triggered by error report(s) on the Tramp ML. Scenario:
>> - Run an older Emacs version, say 27.2.
>> - Load tramp.el (this happens often, implicitly)
>> - Install Tramp 2.5.0.3 from GNU ELPA
>>
>> There is a compilation error, because the already loaded tramp-compat.el
>> does not fit the expectations of Tramp 2.5.0.3.
>
> I think we should treat this is a bug in package.el: we already have
> code to try and avoid such problems, so apparently it doesn't do its job
> in this case.

The point is that simply unloading Tramp didn't work. That's why I start
a new Emacs instance via "emacs -Q" for recompilation. Maybe we could
use this approach in package.el?

I'll happily remove tramp-recompile-elpa from Tramp once recompilation
is fixed in package.el. But since Tramp is backwards compatible (down to
Emacs 25.1 ATM), this will take years.

>>> [ Also, I don't see why we need `tramp-recompile-elpa`.
>>>   I do see a need for `M-x package-recompile`, OTOH.  ]
>> D'accord.
>
> Patches warmly welcome,

I'll have a look on it. No promise for a working patch, of course.

>         Stefan

Best regards, Michael.

Re: master 5183123: Add Tramp recompilation

[Stefan Monnier]

>> I think we should treat this is a bug in package.el: we already have
>> code to try and avoid such problems, so apparently it doesn't do its job
>> in this case.
> The point is that simply unloading Tramp didn't work.

The way package.el tries to deal with it currently is not by unloading
the package (which we consider as too risky) but by force-loading the
new files over the old ones.  E.g. we load ~/.emacs.d/elpa/.../tramp-compat.el
before starting the compilation if we see that `tramp-compat` is in the
load-history.

I recommend you open a bug report for the current problem, and we can
try and figure out what's the root cause of the miscompilation.
That will help us figure out how to better avoid such problems.

> That's why I start a new Emacs instance via "emacs -Q" for
> recompilation. Maybe we could use this approach in package.el?

The `async` package provides such a solution indeed.  It'd be good to
integrate it into Emacs proper.

> I'll happily remove tramp-recompile-elpa from Tramp once recompilation
> is fixed in package.el. But since Tramp is backwards compatible (down to
> Emacs 25.1 ATM), this will take years.

I'm not bothered by the existence of `tramp-recompile-elpa` (interactive
or not).  I just saw that "a lot of effort" was expanded trying to make
it work smoothly whereas I thought we should spend more time making
it unnecessary.

>>>> [ Also, I don't see why we need `tramp-recompile-elpa`.
>>>>   I do see a need for `M-x package-recompile`, OTOH.  ]
>>> D'accord.
>> Patches warmly welcome,
> I'll have a look at it. No promise for a working patch, of course.

If you take the first step, I can probably help take the second.


        Stefan

Re: master 5183123: Add Tramp recompilation

[Michael Albinus]

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

> The way package.el tries to deal with it currently is not by unloading
> the package (which we consider as too risky) but by force-loading the
> new files over the old ones.  E.g. we load ~/.emacs.d/elpa/.../tramp-compat.el
> before starting the compilation if we see that `tramp-compat` is in the
> load-history.
>
> I recommend you open a bug report for the current problem, and we can
> try and figure out what's the root cause of the miscompilation.
> That will help us figure out how to better avoid such problems.

bug#48015

>         Stefan

Best regards, Michael.