bug#30164: load-history and modules

bug#30164: load-history and modules

[Glenn Morris]

Package: emacs
Version: 26.0.91
Severity: minor

As pointed out in https://debbugs.gnu.org/30106#25,
Fmodule_load does not update load-history.

As a result

(require 'some-module)

gives a confusing error message if some-module.so fails to provide a feature.

Eg:

Comment out the line in data/emacs-module/mod-test.c:
  provide (env, "mod-test");

and rebuild mod-test.so. Then do:

./src/emacs -Q -L $PWD/test/data/emacs-module
(require 'mod-test)

-> error "Loading file [...]elec-pair.elc failed to provide feature 'mod-test'")

bug#30164: load-history and modules

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 192 bytes --]

Glenn Morris <rgm@gnu.org> schrieb am Do., 18. Jan. 2018 um 23:50 Uhr:

> As pointed out in https://debbugs.gnu.org/30106#25,
> Fmodule_load does not update load-history.
>

 Here is a patch.

[-- Attachment #1.2: Type: text/html, Size: 541 bytes --]

[-- Attachment #2: 0001-Properly-integrate-modules-into-the-loading-process-Bu.txt --]
[-- Type: text/plain, Size: 6491 bytes --]

From 6814b3f3a1c4f9f1c7cc3b7f91fa398a262cdabb Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sun, 28 Jan 2018 21:36:03 +0100
Subject: [PATCH] Properly integrate modules into the loading process
 (Bug#30164).

* src/lread.c (Fload): Don't defer to module-load immediately when
encountering a module, but use the normal loading machinery to
properly set up load-history, check for recursive loads, print
messages, etc.

* test/src/emacs-module-tests.el (module/load-history): New test.
---
 src/lread.c                    | 82 ++++++++++++++++++++++++++++++------------
 test/src/emacs-module-tests.el | 11 ++++++
 2 files changed, 70 insertions(+), 23 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index 3b0a17c90b..1221dc9a05 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -164,6 +164,8 @@ static int read_emacs_mule_char (int, int (*) (int, Lisp_Object),
 static void readevalloop (Lisp_Object, struct infile *, Lisp_Object, bool,
                           Lisp_Object, Lisp_Object,
                           Lisp_Object, Lisp_Object);
+
+static void build_load_history (Lisp_Object, bool);
 \f
 /* Functions that read one byte from the current source READCHARFUN
    or unreads one byte.  If the integer argument C is -1, it returns
@@ -1246,8 +1248,9 @@ Return t if the file exists and loads successfully.  */)
     }
 
 #ifdef HAVE_MODULES
-  if (suffix_p (found, MODULES_SUFFIX))
-    return unbind_to (count, Fmodule_load (found));
+  bool is_module = suffix_p (found, MODULES_SUFFIX);
+#else
+  bool is_module = false;
 #endif
 
   /* Check if we're stuck in a recursive load cycle.
@@ -1348,7 +1351,7 @@ Return t if the file exists and loads successfully.  */)
             } /* !load_prefer_newer */
 	}
     }
-  else
+  else if (!is_module)
     {
       /* We are loading a source file (*.el).  */
       if (!NILP (Vload_source_file_function))
@@ -1375,7 +1378,7 @@ Return t if the file exists and loads successfully.  */)
       stream = NULL;
       errno = EINVAL;
     }
-  else
+  else if (!is_module)
     {
 #ifdef WINDOWSNT
       emacs_close (fd);
@@ -1386,9 +1389,23 @@ Return t if the file exists and loads successfully.  */)
       stream = fdopen (fd, fmode);
 #endif
     }
-  if (! stream)
-    report_file_error ("Opening stdio stream", file);
-  set_unwind_protect_ptr (fd_index, close_infile_unwind, stream);
+
+  if (is_module)
+    {
+      /* `module-load' uses the file name, so we can close the stream
+         now.  */
+      if (fd >= 0)
+        {
+          emacs_close (fd);
+          clear_unwind_protect (fd_index);
+        }
+    }
+  else
+    {
+      if (! stream)
+        report_file_error ("Opening stdio stream", file);
+      set_unwind_protect_ptr (fd_index, close_infile_unwind, stream);
+    }
 
   if (! NILP (Vpurify_flag))
     Vpreloaded_file_list = Fcons (Fpurecopy (file), Vpreloaded_file_list);
@@ -1398,6 +1415,8 @@ Return t if the file exists and loads successfully.  */)
       if (!safe_p)
 	message_with_string ("Loading %s (compiled; note unsafe, not compiled in Emacs)...",
 		 file, 1);
+      else if (is_module)
+        message_with_string ("Loading %s (module)...", file, 1);
       else if (!compiled)
 	message_with_string ("Loading %s (source)...", file, 1);
       else if (newer)
@@ -1411,24 +1430,39 @@ Return t if the file exists and loads successfully.  */)
   specbind (Qinhibit_file_name_operation, Qnil);
   specbind (Qload_in_progress, Qt);
 
-  struct infile input;
-  input.stream = stream;
-  input.lookahead = 0;
-  infile = &input;
-
-  if (lisp_file_lexically_bound_p (Qget_file_char))
-    Fset (Qlexical_binding, Qt);
-
-  if (! version || version >= 22)
-    readevalloop (Qget_file_char, &input, hist_file_name,
-		  0, Qnil, Qnil, Qnil, Qnil);
+  if (is_module)
+    {
+#ifdef HAVE_MODULES
+      specbind (Qcurrent_load_list, Qnil);
+      LOADHIST_ATTACH (found);
+      Fmodule_load (found);
+      build_load_history (found, true);
+#else
+      /* This cannot happen.  */
+      emacs_abort ();
+#endif
+    }
   else
     {
-      /* We can't handle a file which was compiled with
-	 byte-compile-dynamic by older version of Emacs.  */
-      specbind (Qload_force_doc_strings, Qt);
-      readevalloop (Qget_emacs_mule_file_char, &input, hist_file_name,
-		    0, Qnil, Qnil, Qnil, Qnil);
+      struct infile input;
+      input.stream = stream;
+      input.lookahead = 0;
+      infile = &input;
+
+      if (lisp_file_lexically_bound_p (Qget_file_char))
+        Fset (Qlexical_binding, Qt);
+
+      if (! version || version >= 22)
+        readevalloop (Qget_file_char, &input, hist_file_name,
+                      0, Qnil, Qnil, Qnil, Qnil);
+      else
+        {
+          /* We can't handle a file which was compiled with
+             byte-compile-dynamic by older version of Emacs.  */
+          specbind (Qload_force_doc_strings, Qt);
+          readevalloop (Qget_emacs_mule_file_char, &input, hist_file_name,
+                        0, Qnil, Qnil, Qnil, Qnil);
+        }
     }
   unbind_to (count, Qnil);
 
@@ -1449,6 +1483,8 @@ Return t if the file exists and loads successfully.  */)
       if (!safe_p)
 	message_with_string ("Loading %s (compiled; note unsafe, not compiled in Emacs)...done",
 		 file, 1);
+      else if (is_module)
+        message_with_string ("Loading %s (module)...done", file, 1);
       else if (!compiled)
 	message_with_string ("Loading %s (source)...done", file, 1);
       else if (newer)
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index a6407524ad..a0c5f66b57 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -17,6 +17,7 @@
 ;; You should have received a copy of the GNU General Public License
 ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
+(require 'cl-lib)
 (require 'ert)
 
 (defconst mod-test-emacs
@@ -261,4 +262,14 @@ module--test-assertion
       (rx "Module function called during garbage collection\n")
     (mod-test-invalid-finalizer)))
 
+(ert-deftest module/load-history ()
+  "Check that Bug#30164 is fixed."
+  (load mod-test-file)
+  (cl-destructuring-bind (file &rest entries) (car load-history)
+    (should (equal (file-name-sans-extension file) mod-test-file))
+    (should (member '(provide . mod-test) entries))
+    ;; FIXME: The following should be true.
+    ;; (should (member '(defun . mod-test-sum) entries))
+    ))
+
 ;;; emacs-module-tests.el ends here
-- 
2.15.1

bug#30164: load-history and modules

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 28 Jan 2018 20:38:12 +0000
> Cc: 30164@debbugs.gnu.org
> 
>  Here is a patch.

Thanks, this LGTM, but I think it needs a NEWS entry.  Also, please
see if something we say about 'load' and load-history needs updating
due to this changeset.

bug#30164: load-history and modules

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 2. Feb. 2018 um 16:24 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 28 Jan 2018 20:38:12 +0000
> > Cc: 30164@debbugs.gnu.org
> >
> >  Here is a patch.
>
> Thanks, this LGTM, but I think it needs a NEWS entry.  Also, please
> see if something we say about 'load' and load-history needs updating
> due to this changeset.
>

Thanks, pushed to master as  0443411f5c.
The manual didn't say anything in particular about differences in behavior
for loading modules (which makes sense, the previous behavior was clearly
buggy), so no change to the manual was necessary.

[-- Attachment #2: Type: text/html, Size: 1087 bytes --]