Suggestion: `:update-query` property

[tkf]

When Emacs library changes slots in defstruct or defclass, reloading the library causes problem because accessing the old object using new accessor raises an error. If the library offers a function to close its resources (defstruct or defclass objects), possibly asking user whether or not to save the resources, I think calling that function before updating the library is a simple to solve this problem. To make it possible, I suggest adding :update-query (like kill-buffer-query-functions) property in the recipe property. What do you think?

[jd]

Looks like an idea, but I doubt it would be enough. It's clear that Emacs has not be designed to allow the upgrade of various code at runtime (but almost no software has been designed to do so anyway :-). So while this might solve some issues, I'm not sure el-get will do enough to solve ALL upgrade related issues.

In the end, I think going this way will lead users to think el-get supports the dynamic upgrade of elisp code, whereas it doesn't because Emacs does not. And that may lead to disastrous thing in the end. :(

[tkf]

Yes, I agree that rebooting Emacs is the best way to reload upgraded library. But killing Emacs after upgrade still has some problem if the library uses, for example kill-emacs-query-functions. What library can do is to close (or even better, query user for saving) resource before updating the library which makes Emacs shutdown safe.

At least, if upgrading library is not safe for the library, recipe author can prevent disaster by doing something like this:

(:name some-library ...
       :update-query
       (if (and (fboundp 'some-library-active-p)
                (some-library-active-p))
           (progn
             (y-or-n-p "Some-library does not support update when it is active.  \
Please restart Emacs.")
             nil)
         t))

[jd]

Yeah, you're right @tkf.

In the end, I wonder if el-get-upgrade should reload code by default. I think it may be done via C-u el-get-upgrade, but not "by default". And some library could have a flag in the recipe indicating it's too dangerous and not supported to reload them.

(I saw @dimitri doing an el-get-upgrade of Gnus a few weeks ago and that's convincing me even more about how a bad idea it can be. :)

[tkf]

Ah, I didn't think about not reloading by el-get-update! Reloading is definitely useful for most cases, but adding a flag in the recipe to inhibit reload is probably better solution than the one I suggested. Anyway, :reload-query is better name than :update-query because it should work for el-get-reload also.

[DarwinAwardWinner]

I'm the one who originally wrote the el-get-reload function. It is most certainly a "best effort" function, with no guarantee of actually working. Perhaps we need a :reloadable property that defaults to nil, effectively a reloadability whitelist? el-get-update would only reload a package if the recipe sets this property to t, and when using el-get-reload directly on any package without this property set, it would prompt with something like "Package X is not known to be safely reloadable. Attempting to reload it might cause problems. Would you like to try anyway?".

Certainly the documentation should reflect that el-get-reload is not guaranteed to work.

[tkf]

It would be nice if :reloadable is a list form, not just a boolean. That way you can put not only nil and t but also a function call.

[DarwinAwardWinner]

So anything other than nil or t would be code to eval to help the package reload properly? I guess this could be supported, but I don't know how easy it would be to write such code.

[tkf]

nil and t are also valid lisp form, right? Evaluating them just yields nil and t. One thing we should be careful is absence of this property. It should be interpreted as t instead of nil if we want backward compatible behavior.

[DarwinAwardWinner]

Yes, you could treat t as just another form to eval, but that is an implementation detail. Semantically, t would mean that the package is safe to reload without modification, while any other code means that the package requires help to reload.

As for backward-compatibility, I have no problem changing the default behavior to be a message telling people to restart emacs.

[tkf]

What I meant by lisp form was code without side effect. Just check if it is possible to reload the package. I prefer something like :pre-reload for anything with side effect. In that case raising error can be interpreted as unreloadable. But I have no strong opinion on this. Probably we can even allow query for user in the :reloadable lisp form.

Re: default :reloadable.
I am OK for breaking backward compatibility in this case. But I guess many libraries in el-get/recipes are relodable, except for several complex libraries. Also, I guess recipe author won't put :reloadable t if they need to know about what library does. On the other hand, I can see people sending PR for adding :reloadable nil if it is not default, after they get injured by reloading problem. So, if we want :reloadable to be actually useful, I think nil by default won't work. What we can do is to provide a variable el-get-default-reloadable which is nil by default so that those who are lazy or want to test poison can set t to el-get-default-reloadable.

[DarwinAwardWinner]

Ah, I see. Can you think of any case where you wouldn't just put either t or nil? When would a package be only sometimes reloadable?

[tkf]

If you cache any complex object which has destructor then it's not reloadable. You can reload this library if you clear the cache. So, I would put (not (my-has-cache-p)) in the reloadable property.

[dimitri]

I think having pre- and post- hooks for updating packages is a good idea, and a new boolean property :reload defaults true is a good idea too. I don't much like designs in between.

We should error out when trying to reload a package that has :reload set to nil so that users see what happens and cam decide for themselves whether to restart their Emacs now or later: all we can do here is make it so that they are not surprised when they see things are broken.

Packages that want to support full online reload would have to provide pre and post update hooks implementing the internal state and cache migration to the new formats the code is now expecting, which is a rather hard proposal. I guess in some cases something simple can be devised, so we should still provide recipe authors with the feature.

@jd you might want to see http://www.erlang.org/doc/man/gen_server.html#Module:code_change-3 for an example of a whole programming language facility allowing application programmers to easily cope with that :)

[tkf]

:reload instead of :reloadable? I am ok with both but I prefer :reloadable.

[dimitri]

Well I prefer :reload, but he who does the work gets to choose :)