package update trouble

[michael-heerdegen]

Hi,

I must admit that it can be kind of surprising for users that updating helm via package manager can lead to a broken installation. The behavior is something one might not expect from sanity of reason, or from experience with other package managers. So I can a bit understand that some people are not aware of that problem, although it's well documented. I too use bash and apt and didn't read all of it's manual.

I wonder it we can improve the situation - sorry if this was discussed before.

What's the underlying problem? I guess the problem is that require doesn't reload the new versions of libraries, so that macros are expanded based on outdated code while compiling, which is fatal.

Would it make help to add something like

(eval-when-compile (load "..."))

to force an explicit reload when compiling?

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Hi,

I must admit that it can be kind of surprising for users that updating
helm via package manager can lead to a broken installation. The
behavior is something one might not expect from sanity of reason, or
from experience with other package managers. So I can a bit understand
that some people are not aware of that problem, although it's well
documented. I too use bash and apt and didn't read all of it's manual.

I wonder it we can improve the situation - sorry if this was discussed
before.

What's the underlying problem? I guess the problem is that require
doesn't reload the new versions of libraries, so that macros are
expanded based on outdated code while compiling, which is fatal.

This is indeed the problem.

Would it make help to add something like

(eval-when-compile (load "..."))

to force an explicit reload when compiling?

Yes, in many places probably.

(eval-when-compile (require '...))

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

I think require is not sufficient, since in the bad case, the feature is already loaded, and require won't do anything. That's why I suggested load.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

I think require is not sufficient, since in the bad case, the feature
is already loaded, and require won't do anything. That's why I
suggested load.

I didn't dig fully into that, but I think we will break autoloads and
endup with a helm package very long (slow) to load.
Note that not only helm suffer of this problem, I see the same problem
with magit and probably others, so the right fix would be at the
package.el level like described in FAQ with a message like:
"Please restart emacs to make changes take effect" (Not sure if this is
correct english though).

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

I didn't dig fully into that, but I think we will break autoloads and
endup with a helm package very long (slow) to load.

Why do you think so? We would do this only when compiling. Compiling
may take longer due to reloading. But the compiled files would not look
different than now, except for that they would have been built with up
to date macros.

autoloads are IMHO not related.

Note that not only helm suffer of this problem, I see the same problem
with magit and probably others, so the right fix would be at the
package.el level like described in FAQ with a message like: "Please
restart emacs to make changes take effect" (Not sure if this is
correct english though).

The problem does also appear without using the package manager when
recompiling files you've updated while old versions are loaded. But I
guess package.el could be smart enough to do the right thing.

I could be quite easy: just reload every library of the package that
already had been loaded (load-history!) before compiling.

Better would probably to recompile in a sane environment (using a
separate emacs instance).

But yes, it's not cool to have to fix this per package.

I have the feeling that this already has been discussed at emacs.devel
or so.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

autoloads are IMHO not related.

Yes you are right this happen only at compile time.

The problem does also appear without using the package manager when
recompiling files you've updated while old versions are loaded.

Of course.

But I guess package.el could be smart enough to do the right thing.

I could be quite easy: just reload every library of the package that
already had been loaded (load-history!) before compiling.

No I don't want to enter into such complications.

Better would probably to recompile in a sane environment (using a
separate emacs instance).

It is what the defadvice in the FAQ does.

But yes, it's not cool to have to fix this per package.

Yes, it's a pain to fix this per package.

I have the feeling that this already has been discussed at emacs.devel
or so.

Dunno.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

Better would probably to recompile in a sane environment (using a
separate emacs instance).

It is what the defadvice in the FAQ does.

I meant package.el, not Helm ;-)

I have the feeling that this already has been discussed at
emacs.devel
or so.

Dunno.

I'll try to find it.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

I meant package.el, not Helm ;-)

The defadvice is affecting package.el not helm.

I have the feeling that this already has been discussed at
emacs.devel
or so.

Dunno.

I'll try to find it.

Thanks.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

This issue and related problems were discussed broadly in bug#10125:

https://lists.gnu.org/archive/html/emacs-orgmode/2011-11/msg00844.html

Summary: everybody knows about the problems, but it's tricky to fix this satisfactorily.

I don't expect that this will be fixed in Emacs soon. I don't think it has a high priority, so this may take years.

[michael-heerdegen]

I had a look how other large projects handle this problem.

org doesn't treat the problem at all AFAICT.

Icicles does, I remember that I was involved in working on that issue. We had the advantage that all macros were located in one file ("icicles-mac"), and we decided to force a reload when compiling. We ended up with this:

(eval-when-compile
 (or (condition-case nil
         ;; Use load-library to ensure latest .elc.
         (load-library "icicles-mac")
       (error nil))
     ;; Require, so can load separately if not on `load-path'.
     (require 'icicles-mac)))

That at least shows that it is possible to treat this problem from out of the package.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

I had a look how other large projects handle this problem.

org doesn't treat the problem at all AFAICT.

Icicles does,

We have actually 44 files and I don't want to enter in these troubles.

That at least shows that it is possible to treat this problem from out
of the package.

We can do much better.

1. Compile all files outside of main Emacs.

2. Back to main Emacs and reload all files (or ask user to restart emacs).

See our FAQ (It seems you didn't look at the actual fix).

Actually helm is using emacs-async to copy/rename etc... files from
`helm-find-files', but it is enabled only if dired-async is found.
We could add this dependency to emacs-async in the melpa rules for helm
and I can provide the code proposed in our FAQ (I will improve the error
handling) in emacs-async.
This would fix also the problem for all packages for people using
emacs-async.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

Actually helm is using emacs-async to copy/rename etc... files from
`helm-find-files', but it is enabled only if dired-async is found. We
could add this dependency to emacs-async in the melpa rules for helm

I tried it only quickly. But if it is stable and reliable enough, and
we know that it will continue working in the future, why not?

and I can provide the code proposed in our FAQ (I will improve the
error handling) in emacs-async. This would fix also the problem for
all packages for people using emacs-async.

This sounds like a really good idea!

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

I tried it only quickly. But if it is stable and reliable enough, and
we know that it will continue working in the future, why not?

I am one of the maintainer of emacs-async, so I can ensure this code
will be maintained in the future.

and I can provide the code proposed in our FAQ (I will improve the
error handling) in emacs-async. This would fix also the problem for
all packages for people using emacs-async.

This sounds like a really good idea!

I have improved the code to handle better errors, you can try it if you
want, it is here:

https://github.com/thierryvolpiatto/emacs-tv-config/blob/master/tv-utils.el#L816

You can try it like this:

(async-byte-recompile-directory "~/tmp/helm" 0 t)

Add error to a helm file (e.g remove some parens)

and async byte recompile directory.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

At first, I noticed that after compiling tv-utils.el, the compiled code includes a hardcoded absolute path. It was put there by async-start. See jwiegley/emacs-async#33.

[michael-heerdegen]

Have tried it now. Works like a charm!

[thierryvolpiatto]

Now I have repaired async.el and async-bytecomp.el, I think we can use this as dependency.

[thierryvolpiatto]

BTW hope I did the right thing to add dependency for async in helm-pkg.el

[thierryvolpiatto]

Looks it is working fine now helm have async as dependency, we will see in the future how helm and other packages are upgrading when using async-bytecomp.

[michael-heerdegen]

Fine, thanks so far.

[thierryvolpiatto]

Since we have no more bug reports about this since we switch to async, I am closing this.