bug#35022: 26.1; Load order of custom-autoload, custom-load-symbol, custom-loads is backward?

bug#35022: 26.1; Load order of custom-autoload, custom-load-symbol, custom-loads is backward?

[Allen Li]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

I just spent some time tracking down an unexpected behavior in custom
autoloads.

The custom-autoload function is used in autoloads/loaddef files to set
up custom variables.  custom-autoload adds a load path to the
custom-loads property of a variable.  This property is then used in
custom-load-symbol to load each of the load paths.

The problem is that custom-autoload adds load paths to the front of
custom-loads, and custom-load-symbol loads the paths in forward order.

This is unexpected because if the site distributes a package foo and the
user installs their own version of foo, the site custom-autoload will be
called first:

;; Called from site-start
(custom-autoload 'foo-some-variable "path/to/site/foo" nil)
;; Called from init.el/package-initialize
(custom-autoload 'foo-some-variable "path/to/user/foo" nil)

In this case the custom-loads property will be

("path/to/user/foo" "path/to/site/foo")

The user version will be loaded first, and the site version will be
loaded later, overriding the user version.

This behavior is unexpected; the user would expect that their version of
foo overrides the site version of foo.  Furthermore, this behavior is
somewhat non-deterministic; site overrides foo *only if* foo is loaded
via setting foo-some-variable with customize.  If foo is loaded normally
(e.g. with require), then the user version of foo will be loaded in
preference to the site verison of foo.

I have attached a patch fixing this.

Note that this is technically a breaking change; however I really doubt
any one is intentionally relying on this behavior.  I consider this a
bugfix and I think there are probably users who don't realize that they
are suffering from this bug because it's so subtle.

In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.22.24), modified by Debian
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux rodete (upgraded from: Ubuntu 14.04 LTS)

[-- Attachment #2: 0001-Load-custom-variable-dependencies-in-FIFO-order.patch --]
[-- Type: text/x-patch, Size: 2250 bytes --]

From 719573c43ecb6e37e2f7c813dfcab09c8707d59d Mon Sep 17 00:00:00 2001
From: Allen Li <ayatane@google.com>
Date: Wed, 27 Mar 2019 12:39:28 -0700
Subject: [PATCH] Load custom variable dependencies in FIFO order

The custom-autoload function is used in autoloads/loaddef files to set
up custom variables.  custom-autoload adds a load path to the
custom-loads property of a variable.  This property is then used in
custom-load-symbol to load each of the load paths.

The problem is that custom-autoload adds load paths to the front of
custom-loads, and custom-load-symbol loads the paths in forward order.

This is unexpected because if the site distributes a package foo and the
user installs their own version of foo, the site custom-autoload will be
called first:

;; Called from site-start
(custom-autoload 'foo-some-variable "path/to/site/foo" nil)
;; Called from init.el/package-initialize
(custom-autoload 'foo-some-variable "path/to/user/foo" nil)

In this case the custom-loads property will be

("path/to/user/foo" "path/to/site/foo")

The user version will be loaded first, and the site version will be
loaded later, overriding the user version.

This behavior is unexpected; the user would expect that their version of
foo overrides the site version of foo.  Furthermore, this behavior is
somewhat non-deterministic; site overrides foo *only if* foo is loaded
via setting foo-some-variable with customize.  If foo is loaded normally
(e.g. with require), then the user version of foo will be loaded in
preference to the site verison of foo.

* lisp/custom.el (custom-load-symbol): Load variable deps in reverse order.
---
 lisp/custom.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index f0125742d1..da407bee83 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -639,7 +639,7 @@ custom-load-symbol
       (condition-case nil
 	  (require 'cus-start)
 	(error nil))
-      (dolist (load (get symbol 'custom-loads))
+      (dolist (load (reverse (get symbol 'custom-loads)))
 	(cond ((symbolp load) (condition-case nil (require load) (error nil)))
 	      ;; This is subsumed by the test below, but it's much faster.
 	      ((assoc load load-history))
-- 
2.21.0.392.gf8f6787159e-goog

bug#35022: 26.1; Load order of custom-autoload, custom-load-symbol, custom-loads is backward?

[Allen Li]

Please ignore my patch.  In my haste, I missed that custom-load-symbol
already checks whether a dependency is already loaded, because that
logic wasn't firing for my case (where the load path is an actual path
rather than a package name).

To clarify, the existing code works for this:

;; Called from site-start
(custom-autoload 'foo-some-variable "foo" nil)
;; Called from init.el/package-initialize
(custom-autoload 'foo-some-variable "foo" nil)

But not for this:

;; Called from site-start
(custom-autoload 'foo-some-variable "some/path/to/foo" nil)
;; Called from init.el/package-initialize
(custom-autoload 'foo-some-variable "foo" nil)

I'm not sure yet what the proper approach for my problem is.

bug#35022: 26.1; Load order of custom-autoload, custom-load-symbol, custom-loads is backward?

[Noam Postavsky]

retitle 35022 custom-add-load, custom-load-symbol don't support filenames with directory components
severity 35022 wishlist
quit

Allen Li <darkfeline@felesatra.moe> writes:

> But not for this:
>
> ;; Called from site-start
> (custom-autoload 'foo-some-variable "some/path/to/foo" nil)
> ;; Called from init.el/package-initialize
> (custom-autoload 'foo-some-variable "foo" nil)
>
> I'm not sure yet what the proper approach for my problem is.

Would it be too impertinent to suggest the proper aproach is this:

    ;; Called from site-start
    (add-to-list 'load-path (expand-file-name "some/path/to"))
    (custom-autoload 'foo-some-variable "foo" nil)
    ;; Called from init.el/package-initialize
    (custom-autoload 'foo-some-variable "foo" nil)

Otherwise, I guess extending custom-add-load and custom-load-symbol to
handle entries of the form (LIBRARY . RELATIVE-NAME) could make sense.

bug#35022: 26.1; Load order of custom-autoload, custom-load-symbol, custom-loads is backward?

[Allen Li]

On Thu, Apr 11, 2019 at 1:54 AM Noam Postavsky <npostavs@gmail.com> wrote:
>
> Allen Li <darkfeline@felesatra.moe> writes:
>
> > But not for this:
> >
> > ;; Called from site-start
> > (custom-autoload 'foo-some-variable "some/path/to/foo" nil)
> > ;; Called from init.el/package-initialize
> > (custom-autoload 'foo-some-variable "foo" nil)
> >
> > I'm not sure yet what the proper approach for my problem is.
>
> Would it be too impertinent to suggest the proper aproach is this:
>
>     ;; Called from site-start
>     (add-to-list 'load-path (expand-file-name "some/path/to"))
>     (custom-autoload 'foo-some-variable "foo" nil)
>     ;; Called from init.el/package-initialize
>     (custom-autoload 'foo-some-variable "foo" nil)

The site-start is shipped by the distro which I don't have control over.

>
> Otherwise, I guess extending custom-add-load and custom-load-symbol to
> handle entries of the form (LIBRARY . RELATIVE-NAME) could make sense.

After some deliberation, I think custom-add-load's current behavior is correct,
so I'll take my issue downstream.

Thanks