jwiegley / emacs-async

Simple library for asynchronous processing in Emacs
GNU General Public License v3.0
832 stars 68 forks source link

Should the advice to package--compile be autoloaded? #46

Closed tarsius closed 9 years ago

tarsius commented 9 years ago

@thierryvolpiatto I don't understand how simply installing this package fixes any issues when installing Helm and other packages. Wouldn't the user have to explicitly require async-bytecomp? Wouldn't the advice to package--compile have to be autoloaded for this to just work? Do you explicitly require that library in Helm?

This was from the perspective of "having this just work". I actually have some doubts about whether even explicitly loading async-bytecomp should unconditionally advice package--compile. I would think that at the very least async-bytecomp should then be a separate package, so that users of the other async libraries are not affected without even being aware what is going on. This applies even more so if the advise is to be autoloaded in the future.

@purcell Any advice on this advice? Also, does anyone know the Emacs' maintainers' opinion on compiling in another instance out-of-the-box?

purcell commented 9 years ago

You know, I wondered why I was seeing messages about asynchronous compilation.

Turns out I've got the async package installed, presumably as a dependency from something else, but I've certainly never asked it to mess with my package compilation. :-/

I'd certainly agree that this should be a separate package.

purcell commented 9 years ago

Looking in my ~/.emacs.d/elpa, I find that helm-config requires async-bytecomp. That would be fine if it needs some code in there, but I certainly don't want helm to magically enable async byte compilation as a side effect.

thierryvolpiatto commented 9 years ago

Jonas Bernoulli notifications@github.com writes:

@thierryvolpiatto I don't understand how simply installing this package fixes any issues when installing Helm and other packages. Wouldn't the user have to explicitly require async-bytecomp?

async-bytecomp is required in helm-config.el.

Wouldn't the advice to package--compile have to be autoloaded for this to just work?

Once autoloaded, when installed from MELPA, it will load async-bytecomp unconditionally no ?

This was from the perspective of "having this just work". I actually have some doubts about whether even explicitly loading async-bytecomp should unconditionally advice package--compile.

Well yes, it does when you require async-bytecomp.

I would think that at the very least async-bytecomp should then be a separate package, so that users of the other async libraries are not affected without even being aware what is going on. This applies even more so if the advise is to be autoloaded in the future.

I can also provide the advice disabled and who need it can enable it.

@purcell Any advice on this advice? Also, does anyone know the Emacs' maintainers' opinion on compiling in another instance out-of-the-box?

There is a thread about this on emacs-dev or emacs-bug, can't remember, and compiling files outside of emacs was in discussion... But for the moment, as long as no body came up with a real fix I have enabled this which seems to work fine, at least with my helm users maintaining helm from MELPA (I have rarely bug report about this, and when I have it is generally a misusage of package.el).

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

marcbowes commented 9 years ago

If you can provide a link to that thread I'd really appreciate it.

Is this a bug since forever or was it introduced recently? I'm just trying to understand what has changed. I never experienced issues with package upgrades before but almost every package I install/upgrade these days blows up.

The only other workaround I'm aware of (I haven't tried this one) is to uninstall the package you intend to upgrade, kill emacs and then install it again next launch.

thierryvolpiatto commented 9 years ago

Steve Purcell notifications@github.com writes:

You know, I wondered why I was seeing messages about asynchronous compilation.

Turns out I've got the async package installed, presumably as a dependency from something else, but I've certainly never asked it to mess with my package compilation. :-/

Sorry for the inconvenience, but at least it have worked fine all this months :-)

I'd certainly agree that this should be a separate package.

If I make it a separate package, helm will use it as dependency and the problem will be the same, the advice will be enabled if you install helm. And I still want to use this, as it is a real improvement for package.el (I had tons of bugreports before enabling this).

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

marcbowes commented 9 years ago

Self-help: http://thread.gmane.org/gmane.emacs.devel/175464 has links to bug reports.

thierryvolpiatto commented 9 years ago

Marc Bowes notifications@github.com writes:

If you can provide a link to that thread I'd really appreciate it.

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18443

Is this a bug since forever or was it introduced recently?

Always been here, now emacs-25 is loading the new files which may improve the situation but it will fail to compile correctly with important changes like changing args of a macro or renaming it etc...

I'm just trying to understand what has changed. I never experienced issues with package upgrades before but almost every package I install/upgrade these days blows up.

Depends which packages, simple ones with single or few files and not actively maintained with no significant changes should not be affected.

The only other workaround I'm aware of (I haven't tried this one) is to uninstall the package you intend to upgrade, kill emacs and then install it again next launch.

async-bytecomp advice precisely fix this issue.

It is recommended though to restart emacs after upgrading a package. See:

https://github.com/emacs-helm/helm/wiki#ihaveupdatedfrommelpaandihaveerrors

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

purcell commented 9 years ago

I'd certainly agree that this should be a separate package. If I make it a separate package, helm will use it as dependency and the problem will be the same, the advice will be enabled if you install helm. And I still want to use this, as it is a real improvement for package.el (I had tons of bugreports before enabling this).

Understood. First step would be to not enable the advice by default. A global minor mode to control it would be nice. With this done, extracting it to a separate package would indeed be optional.

Ideally, helm would trigger asynchronous compilation only when helm requires it, and wouldn't change the global behaviour.

thierryvolpiatto commented 9 years ago

Steve Purcell notifications@github.com writes:

Understood. First step would be to not enable the advice by default. A global minor mode to control it would be nice. With this done, extracting it to a separate package would indeed be optional.

This is the way to do to activate/deactivate the advice and easy to do, this is not the problem.

Ideally, helm would trigger asynchronous compilation only when helm requires it, and wouldn't change the global behaviour.

This is the difficult part and I don't see right now how I can enable the advice just for helm.

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

dgutov commented 9 years ago

@thierryvolpiatto

Always been here, now emacs-25 is loading the new files which may improve the situation but it will fail to compile correctly with important changes like changing args of a macro or renaming it etc...

Where's the bug report on that? AFAIK, it should now work correctly in all significant cases.

thierryvolpiatto commented 9 years ago

Dmitry Gutov notifications@github.com writes:

@thierryvolpiatto

Always been here, now emacs-25 is loading the new files which may improve the situation but it will fail to compile correctly with important changes like changing
args of a macro or renaming it etc...

Where's the bug report on that?

There is no bug report, and I will not make any, I have not the time, feel free to fill one if you want.

AFAIK, it should now work correctly in all significant cases. ^^^ most

But not in ALL.

I have no examples in mind actually, but I have had situations with helm where just reloading the files and byte-compile would fail, e.g removing a macro foo called by a function bar without removing the function bar which doesn't use anymore the macro foo (I am not sure but it was something like this) and there is other situations with new args, vars, renaming etc...

Also your fix is provided only in emacs-25 and many users are not using it.

Anyway, starting from now, only the packages added to async-bytecomp-allowed-packages will compile async, by default helm and async.

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

dgutov commented 9 years ago

There is no bug report, and I will not make any, I have not the time, feel free to fill one if you want.

Then please do not spread misinformation. There are few things in open source I dislike more than non-specific complaints.

feel free to fill one if you want

I don't know what to file.

But not in ALL.

AFAIK, the only situations it should behave wrongly in are the false-positives. I.e. when the package wouldn't even compile in a new Emacs session (because the new package version mistakenly removed a macro, for instance), but it may still compile in the session where the previous version was loaded.

but I have had situations with helm where just reloading the files and byte-compile would fail

Just situations, in the past, unrelated to package-install or the current code in Emacs master?

Also your fix is provided only in emacs-25 and many users are not using it.

Sure, but the users of Emacs 25 shouldn't be forced to use the async byte-compilation. A version check somewhere in this package's code should take care of that.

thierryvolpiatto commented 9 years ago

Dmitry Gutov notifications@github.com writes:

Sure, but the users of Emacs 25 shouldn't be forced to use the async byte-compilation.

They are not forced.

I want my helm users to use this, and it will now happen only in helm and async, if they don't want this, up to them to setup this at their own risk, it is now customizable.

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