bug#18835: load-from-path is inconsistent when looking for a compiled version of the source file

bug#18835: load-from-path is inconsistent when looking for a compiled version of the source file

[Geert Janssens]

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

$ guile --version
guile (GNU Guile) 2.0.11

$ config.guess 
x86_64-unknown-linux-gnu

$ rpm -qa | grep guile
compat-guile18-1.8.8-7.fc20.x86_64
guile-debuginfo-2.0.11-1.fc20.x86_64
guile-2.0.11-1.fc20.x86_64
guile-devel-2.0.11-1.fc20.x86_64

These steps illustrate the issue:

1. Download the attached file and save it as $HOME/guile/test.scm
Note: this is just an example file printing one line to indicate is has
successfully been called. Any other scm file will do to illustrate this bug.

2. Run these commands:
$ mkdir -p $HOME/guile/ccache
$ cd $HOME/guile/ccache
$ guild compile -o test.go ../test.scm
$ guild compile -o test.scm.go ../test.scm

This will generate two compiled versions of the test file, each with a different extension.

Now run these commands and observe what happens:
$ cd $HOME/guile
$ GUILE_LOAD_PATH=$HOME/guile \
  GUILE_LOAD_COMPILED_PATH=$HOME/guile/ccache \
  guile -c '(load-from-path "test")'

=> This command will use the file $HOME/guile/ccache/test.go. That is, no auto-compilation 
is triggered.

$ GUILE_LOAD_PATH=$HOME/guile \
  GUILE_LOAD_COMPILED_PATH=$HOME/guile/ccache \
  guile -c '(load-from-path "test.scm")'

=> This command will *not* use either of $HOME/guile/ccache/test.go or 
$HOME/guile/ccache/test.scm.go. Instead it will autocompile test.scm into <default-cache-
dir>/test.scm.go and use that one.

So there is no way to have the second command use your self-compiled files.

From Andy Wingo's comments on irc, it is expected that the second case would have used the 
self-compiled $HOME/guile/ccache/test.scm.go. I'm fine with that although using 
$HOME/guile/ccache/test.go would have made more sense from an outsider's point of view. At 
least it should be possible to make load-from-path use a self-compiled .go file regardless of 
whether the argument ends in .scm or not.

If more information is needed, feel free to ask.

Regards,

Geert

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

[-- Attachment #2: test.scm --]
[-- Type: text/x-scheme, Size: 38 bytes --]

(display "Testfile is read")(newline)

bug#18835: load-from-path is inconsistent when looking for a compiled version of the source file

[Andy Wingo]

On Sat 25 Oct 2014 19:09, Geert Janssens <info@kobaltwit.be> writes:

> $ guild compile -o test.scm.go ../test.scm
>
> $ GUILE_LOAD_PATH=$HOME/guile \
>
> GUILE_LOAD_COMPILED_PATH=$HOME/guile/ccache \
>
> guile -c '(load-from-path "test.scm")'
>
> => This command will *not* use either of $HOME/guile/ccache/test.go or
> $HOME/guile/ccache/test.scm.go. Instead it will autocompile test.scm
> into <default-cache-dir>/test.scm.go and use that one.

Weird.  The logic in load.c is that we only add on .go if the file
doesn't already have an extension.  If the file has an extension and
it's not .go, then we don't grovel in the path at all.  I guess this is
the wrong thing?

I am not sure if we can change this in 2.0 or not.  I guess we can.

Andy

bug#18835: load-from-path is inconsistent when looking for a compiled version of the source file

[Taylan Kammer]

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

In 2016, Andy wrote:

> The logic in load.c is that we only add on .go if the file
> doesn't already have an extension.  If the file has an extension and
> it's not .go, then we don't grovel in the path at all.  I guess this is
> the wrong thing?
> 
> I am not sure if we can change this in 2.0 or not.  I guess we can.

Well that ship has sailed anyway. ;-)

IMO we should change the behavior to always try to add extensions, after
first trying without an extension.

It's feasible that one might choose to name one's files like foo.bar.scm
and the like, in which case trying to load foo.bar should work.  I might
name a number of files foo.v1.scm, foo.v2.scm, and so on.  Alternatively,
consider the already popular foo.upstream.scm.

Further, I don't see any possible confusion arising from adding the
extensions.  No one would name their files foo.go.scm or foo.scm.scm and
then try to load "foo.go" or "foo.scm" and expect the one with an extra
.scm to be loaded.

I thought a bit whether there might be security implications, like when
dropping files into a directory where every user can create files, and
someone could maliciously put a foo.scm.scm to take precedence over your
foo.scm, but that's already a problem as they could drop in a foo.go, so
the solution is not to try to load files from directories you can't trust,
like /tmp.

The only compatibility issue I can think of: maybe some people put both a
file foo and a file foo.scm in the same directory, and expect (load "foo")
to *not* try to load the one without an extension.

Thoughts?


Attached is a patch that would implement the suggestion of trying without
an extension first, then with an extension, without checking whether the
original filename does or doesn't have an extension.

-- 
Taylan

[-- Attachment #2: 0001-Make-load-try-to-add-an-extension-to-files-that-alre.patch --]
[-- Type: text/plain, Size: 4640 bytes --]

From 2ed80f831006815e37374a74218af4572703229f Mon Sep 17 00:00:00 2001
From: Taylan Kammer <taylan.kammer@gmail.com>
Date: Mon, 17 May 2021 19:31:40 +0200
Subject: [PATCH] Make load try to add an extension to files that already have
 one.

* libguile/load.c (load_thunk_from_path, search_path): Remove the code
that checks whether the filename already has an extension.  Add the
empty string to the head of the extensions list.
---
 libguile/load.c | 67 +++++--------------------------------------------
 1 file changed, 6 insertions(+), 61 deletions(-)

diff --git a/libguile/load.c b/libguile/load.c
index e95c36db1..0c198f165 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -649,7 +649,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
   struct stringbuf buf;
   struct stat stat_buf;
   char *filename_chars;
-  size_t filename_len;
   SCM path, extensions;
   SCM result = SCM_BOOL_F;
   char initial_buffer[256];
@@ -667,7 +666,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
   scm_dynwind_begin (0);
 
   filename_chars = scm_to_locale_string (filename);
-  filename_len = strlen (filename_chars);
   scm_dynwind_free (filename_chars);
 
   /* If FILENAME is absolute and is still valid, return it unchanged.  */
@@ -680,34 +678,6 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
       goto end;
     }
 
-  /* If FILENAME has an extension, don't try to add EXTENSIONS to it.  */
-  {
-    char *endp;
-
-    for (endp = filename_chars + filename_len - 1;
-	 endp >= filename_chars;
-	 endp--)
-      {
-	if (*endp == '.')
-	  {
-            if (!string_has_an_ext (filename, extensions))
-              {
-                /* This filename has an extension, but not one of the right
-                   ones... */
-                goto end;
-              }
-	    /* This filename already has an extension, so cancel the
-               list of extensions.  */
-	    extensions = SCM_EOL;
-	    break;
-	  }
-	else if (is_file_name_separator (SCM_MAKE_CHAR (*endp)))
-	  /* This filename has no extension, so keep the current list
-             of extensions.  */
-	  break;
-      }
-  }
-
   /* This simplifies the loop below a bit.
    */
   if (scm_is_null (extensions))
@@ -736,6 +706,9 @@ load_thunk_from_path (SCM filename, SCM source_file_name,
       stringbuf_cat (&buf, filename_chars);
       sans_ext_len = buf.ptr - buf.buf;
 
+      /* Add the empty string as the first "extension." */
+      extensions = scm_cons (scm_nullstr, extensions);
+
       /* Try every extension. */
       for (exts = extensions; scm_is_pair (exts); exts = SCM_CDR (exts))
 	{
@@ -805,7 +778,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
 {
   struct stringbuf buf;
   char *filename_chars;
-  size_t filename_len;
   SCM result = SCM_BOOL_F;
   char initial_buffer[256];
 
@@ -819,7 +791,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
   scm_dynwind_begin (0);
 
   filename_chars = scm_to_locale_string (filename);
-  filename_len = strlen (filename_chars);
   scm_dynwind_free (filename_chars);
 
   /* If FILENAME is absolute and is still valid, return it unchanged.  */
@@ -833,35 +804,6 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
       goto end;
     }
 
-  /* If FILENAME has an extension, don't try to add EXTENSIONS to it.  */
-  {
-    char *endp;
-
-    for (endp = filename_chars + filename_len - 1;
-	 endp >= filename_chars;
-	 endp--)
-      {
-	if (*endp == '.')
-	  {
-            if (scm_is_true (require_exts) &&
-                !string_has_an_ext (filename, extensions))
-              {
-                /* This filename has an extension, but not one of the right
-                   ones... */
-                goto end;
-              }
-	    /* This filename already has an extension, so cancel the
-               list of extensions.  */
-	    extensions = SCM_EOL;
-	    break;
-	  }
-	else if (is_file_name_separator (SCM_MAKE_CHAR (*endp)))
-	  /* This filename has no extension, so keep the current list
-             of extensions.  */
-	  break;
-      }
-  }
-
   /* This simplifies the loop below a bit.
    */
   if (scm_is_null (extensions))
@@ -890,6 +832,9 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
       stringbuf_cat (&buf, filename_chars);
       sans_ext_len = buf.ptr - buf.buf;
 
+      /* Add the empty string as the first "extension." */
+      extensions = scm_cons (scm_nullstr, extensions);
+
       /* Try every extension. */
       for (exts = extensions; scm_is_pair (exts); exts = SCM_CDR (exts))
 	{
-- 
2.30.2