Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

[Stefan Monnier]

>     (package--autoloads-file-name)
>     (package--activate-autoloads-and-load-path): New function.
>     (package-activate-1): Delegate autoloading and load-path
>     configuration to `package--activate-autoloads-and-load-path'.
>     (package--compile): Before compilation, call
>     `package--activate-autoloads-and-load-path' instead of
>     `package-activate-1'.

Not sure I follow what this is doing, nor why.  Can you explain?


        Stefan

Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

[Artur Malabarba]

package-activate-1 was being called twice during installation. Once
before compilation and once after.
IIUC, there are only two things in package-activate-1 that affect
byte-compilation: setting the load-path and loading the autoloads
file. So I refactored those two functionalities into
`package--activate-autoloads-and-load-path'.

Now, instead of calling `package-activate-1' before compilation, we
call this new simpler function.
`package-activate-1' is still called after compilation, and it's
functionality has not changed (it still sets the load-path and loads
autoloads by calling this new function, in addition to the other stuff
that it does).

1) `package-activate-1' has a couple of line that shouldn't be called
twice, like (push name package-activated-list) and (push pkg-dir
Info-directory-list). It wasn't the end of the world, but it was wrong
(again, IIUC) and it could get worse if we extend this function in the
future.

2) It's just confusing to see a package being activated twice
throughout a single installation. I find it more informative to do
specifically what we want (i.e., load autloads and set load-path
before compilation).

3) The name package-activate-1 gives the impression that it's an
implementation function for package-activate, which made it confusing
that it was being called outside package-activate.


2015-09-08 2:33 GMT+01:00 Stefan Monnier <monnier@iro.umontreal.ca>:
>>     (package--autoloads-file-name)
>>     (package--activate-autoloads-and-load-path): New function.
>>     (package-activate-1): Delegate autoloading and load-path
>>     configuration to `package--activate-autoloads-and-load-path'.
>>     (package--compile): Before compilation, call
>>     `package--activate-autoloads-and-load-path' instead of
>>     `package-activate-1'.
>
> Not sure I follow what this is doing, nor why.  Can you explain?
>
>
>         Stefan

Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

[Stefan Monnier]

> IIUC, there are only two things in package-activate-1 that affect
> byte-compilation: setting the load-path and loading the autoloads
> file.

Hmm... I think that "Call `load' on all files in `pkg-dir' already present in
`load-history'" would also affect compilation, so it should be in
package--activate-autoloads-and-load-path.

> 1) `package-activate-1' has a couple of line that shouldn't be called
> twice, like (push name package-activated-list) and (push pkg-dir
> Info-directory-list).

They should be fixed to be idempotent since that can happen in any case
if the package was activated and then gets updated (and hence re-activated).

> 2) It's just confusing to see a package being activated twice
> throughout a single installation. I find it more informative to do
> specifically what we want (i.e., load autloads and set load-path
> before compilation).

That makes sense.  I think compilation is a separate step which should
be optional (and as mentioned elsewhere, we should have a command to
perform (re)compilation of a package separately afterwards).

So the presence of two "activation" calls is legitimate (the one for
compilation is needed if the package was not activated yet, and the
other is needed if compilation did not happen).  But I wonder why they
should be different.  Can't we just have the "non-compilation
activation" be performed before compilation takes place, and then change
compilation so that it only activates the package if that was not
done yet?


        Stefan

Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

[Artur Malabarba]

2015-09-08 14:46 GMT+01:00 Stefan Monnier <monnier@iro.umontreal.ca>:
>> IIUC, there are only two things in package-activate-1 that affect
>> byte-compilation: setting the load-path and loading the autoloads
>> file.
>
> Hmm... I think that "Call `load' on all files in `pkg-dir' already present in
> `load-history'" would also affect compilation, so it should be in
> package--activate-autoloads-and-load-path.

It can, but this wasn't done in the first activation anyway (because
the optional reload arg was nil) and I was trying to refactor without
changing any behavior. But I guess it would fix some issues if the
reloading happened before compilation as well.

>> 1) `package-activate-1' has a couple of line that shouldn't be called
>> twice, like (push name package-activated-list) and (push pkg-dir
>> Info-directory-list).
>
> They should be fixed to be idempotent since that can happen in any case
> if the package was activated and then gets updated (and hence re-activated).

OK

>> 2) It's just confusing to see a package being activated twice
>> throughout a single installation. I find it more informative to do
>> specifically what we want (i.e., load autloads and set load-path
>> before compilation).
>
> That makes sense.  I think compilation is a separate step which should
> be optional (and as mentioned elsewhere, we should have a command to
> perform (re)compilation of a package separately afterwards).
>
> So the presence of two "activation" calls is legitimate (the one for
> compilation is needed if the package was not activated yet, and the
> other is needed if compilation did not happen).  But I wonder why they
> should be different.  Can't we just have the "non-compilation
> activation" be performed before compilation takes place, and then change
> compilation so that it only activates the package if that was not
> done yet?

Yes, but we'll also have to add a third pseudo-activation step after
byte-compilation where we reload files already loaded. After all,
these files have now been compiled.

Re: [Emacs-diffs] master add49b6: * lisp/emacs-lisp/package.el: Reduce autoloading before compiling

[Stefan Monnier]

> Yes, but we'll also have to add a third pseudo-activation step after
> byte-compilation where we reload files already loaded. After all,
> these files have now been compiled.

Not sure it's important enough, but if you want to do it, that's OK.


        Stefan