Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

>     * lisp/files.el (locate-file): Return the .elc file (if it exists)
>     in nativecomp, too, to mimic the behaviour from non-nativecomp
>     builds (bug#51308).

Huh?  `locate-file` should have nothing to do with the mapping between
.elc and .eln files.  It's used for all kinds of files not just for
files containing ELisp code.

IOW the patch above makes things closer to what they should be, but
we should simply not need that because locate-file-internal should not
magically replace `.elc` files with `.eln` files.


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> IOW the patch above makes things closer to what they should be, but
> we should simply not need that because locate-file-internal should not
> magically replace `.elc` files with `.eln` files.

Indeed.  Patches welcome.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

Lars Ingebrigtsen [2022-03-12 22:51:01] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> IOW the patch above makes things closer to what they should be, but
>> we should simply not need that because locate-file-internal should not
>> magically replace `.elc` files with `.eln` files.
> Indeed.  Patches welcome.

How 'bout


        Stefan


diff --git a/lisp/files.el b/lisp/files.el
index 7be93662b1c..eca8cba93f2 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -987,20 +987,7 @@ locate-file
 	  (logior (if (memq 'executable predicate) 1 0)
 		  (if (memq 'writable predicate) 2 0)
 		  (if (memq 'readable predicate) 4 0))))
-  (let ((file (locate-file-internal filename path suffixes predicate)))
-    (if (and file (string-match "\\.eln\\'" file))
-        ;; This is all a bit of a mess.  We pass in a list of suffixes
-        ;; that doesn't include .eln, but with a nativecomp emacs, we
-        ;; get the .eln file back.  We then map that to the .el file.
-        ;; But `load-history' has the .elc file, so that's the file we
-        ;; return here (if it exists).
-        (let* ((el (gethash (file-name-nondirectory file) comp-eln-to-el-h))
-               (elc (replace-regexp-in-string "\\.el\\'" ".elc" el)))
-          (if (and (member ".elc" suffixes)
-                   (file-exists-p elc))
-              elc
-            el))
-      file)))
+  (locate-file-internal filename path suffixes predicate))
 
 (defun locate-file-completion-table (dirs suffixes string pred action)
   "Do completion for file names passed to `locate-file'."
diff --git a/src/lread.c b/src/lread.c
index 0486a98883c..d7b56c5087e 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1661,7 +1661,7 @@ DEFUN ("locate-file-internal", Flocate_file_internal, Slocate_file_internal, 2,
   (Lisp_Object filename, Lisp_Object path, Lisp_Object suffixes, Lisp_Object predicate)
 {
   Lisp_Object file;
-  int fd = openp (path, filename, suffixes, &file, predicate, false, false);
+  int fd = openp (path, filename, suffixes, &file, predicate, false, true);
   if (NILP (predicate) && fd >= 0)
     emacs_close (fd);
   return file;

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> How 'bout

Well, that looks much better.  😅

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

Lars Ingebrigtsen [2022-03-13 00:40:56] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> How 'bout
> Well, that looks much better.  😅

It seems to pass the tests here, so I pushed it,


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Sat, 12 Mar 2022 23:51:50 -0500
> 
> Lars Ingebrigtsen [2022-03-13 00:40:56] wrote:
> > Stefan Monnier <monnier@iro.umontreal.ca> writes:
> >> How 'bout
> > Well, that looks much better.  😅
> 
> It seems to pass the tests here, so I pushed it,

I object to such backward-incompatible change in a public API.  Please
revert that change.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I object to such backward-incompatible change in a public API.  Please
> revert that change.

Stefan's patch does not change the behaviour of locate-file (after my
patch).  It does change the behaviour of locate-file-internal, but
that's not a public function (and it's only used in locate-file).  So
I'm not sure what you mean here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

>> I object to such backward-incompatible change in a public API.  Please
>> revert that change.
> Stefan's patch does not change the behaviour of locate-file (after my
> patch).  It does change the behaviour of locate-file-internal, but
> that's not a public function (and it's only used in locate-file).  So
> I'm not sure what you mean here.

AFAIK it reverts the behavior of `locate-file-internal` to that of
Emacs-27, and I think the change in Emacs-28 is just a bug (an
oversight, I suspect), so I'd even suggest we install my patch in
`emacs-28`.


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> AFAIK it reverts the behavior of `locate-file-internal` to that of
> Emacs-27, and I think the change in Emacs-28 is just a bug (an
> oversight, I suspect), so I'd even suggest we install my patch in
> `emacs-28`.

That is probably the right thing to do, but I'd prefer to test it on
master for a while first, because it's not obvious what the
repercussions might be.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Sun, 13 Mar 2022 15:16:46 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I object to such backward-incompatible change in a public API.  Please
> > revert that change.
> 
> Stefan's patch does not change the behaviour of locate-file (after my
> patch).  It does change the behaviour of locate-file-internal, but
> that's not a public function (and it's only used in locate-file).  So
> I'm not sure what you mean here.

I mean locate-file-internal.  It behaved like that since the merge of
native-compilation, which happened more than a year ago.  Emacs 28
will be released very soon with that behavior, which means that
behavior will be out there until Emacs 29.1 is released, at least two
years from now?  And all that just because some test failed?  Sorry, I
cannot agree to that.

It is very easy, albeit less elegant, to add a new optional argument;
then we can have the cake and eat it, too (provided that this solution
is at all correct and doesn't cause regressions, which I'm not sure is
true).

More generally, I'm not at all convinced that the problem, as
presented in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51308#14,
indeed needs a solution that makes locate-file behave the same as it
did in Emacs 27, because the *.eln files introduce new aspects into
the stuff that 'load' and 'load-history' intend to support.  We didn't
discuss this enough, and I'm not even sure we have a common POV on
this.

Bottom line: this needs further discussion.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Sun, 13 Mar 2022 10:26:48 -0400
> 
> >> I object to such backward-incompatible change in a public API.  Please
> >> revert that change.
> > Stefan's patch does not change the behaviour of locate-file (after my
> > patch).  It does change the behaviour of locate-file-internal, but
> > that's not a public function (and it's only used in locate-file).  So
> > I'm not sure what you mean here.
> 
> AFAIK it reverts the behavior of `locate-file-internal` to that of
> Emacs-27

Emacs 27 behavior cannot guide us here, because the *.eln files and
the aspects they bring into the issue are new.

> and I think the change in Emacs-28 is just a bug (an oversight, I
> suspect)

How can you think that when this "bug" endured more than a year of
testing since the nativecomp branch was merged, including the pretest
until now?  The only place where this issue happens is in a test, and
we can easily change the test to adapt it to the actual behavior,
should we decide that it's the correct behavior--and I'm not yet
saying it has to be, but I'm not yet convinced it's wrong, either.
The whole issue is somewhat non-trivial, because load-history doesn't
(AFAIK) record the *.eln files when they are loaded, but instead
"lies" to us by mentioning only the *.elc files.  I'm not sure we
shouldn't change that, for example.

> so I'd even suggest we install my patch in `emacs-28`.

Do we want to release Emacs 28 any time soon, or do we want to
continue dragging our feet by making destabilizing changes in it?  The
behavior of locate-file on emacs-28 doesn't cause any problems that we
know about, so what would be our reason for making such a problematic
change and delaying the release?

Look, the native-compilation stuff introduces complexity into Emacs
whose implications we don't yet understand well enough.  It will take
time to collect enough experience and see what else should be done and
changed as result.  I submit that your opinion that
"locate-file-internal should not magically replace `.elc` files with
`.eln` files" is not based on enough data for us to change the
behavior that worked well for the last year.

Bottom line: I see no reason to rush in this matter, especially since
the "problem" so far exists only in a test.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > I object to such backward-incompatible change in a public API.  Please
>> > revert that change.

[...]

> I mean locate-file-internal.

I don't think the -internal subrs can be counted as part of the public
API.  Like -- functions, they're internal.

> And all that just because some test failed?

It's a real bug (breaking loadhist functionality).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

>> AFAIK it reverts the behavior of `locate-file-internal` to that of
>> Emacs-27
> Emacs 27 behavior cannot guide us here, because the *.eln files and
> the aspects they bring into the issue are new.

`locate-file` is a function looks for files with a given set of
extensions through path.  That's it.  Nothing specific to ELisp files.
E.g. it's used in `executable-find`.

The existence of `.eln` should have no bearing on this.

It can affect the ELisp-specific code (such as `load` or
`find-library`), which may indeed use `locate-file` internally, but it
should not affect `locate-file` itself.

>> and I think the change in Emacs-28 is just a bug (an oversight, I
>> suspect)
> How can you think that when this "bug" endured more than a year of
> testing since the nativecomp branch was merged, including the pretest
> until now?

That's just what my understanding of the code tells me.

>> so I'd even suggest we install my patch in `emacs-28`.
> Do we want to release Emacs 28 any time soon, or do we want to
> continue dragging our feet by making destabilizing changes in it?

Notice that I did not install it on `emacs-28` nor asked to do that.
I only mention it as a way to say that I consider it fixes a regression
in Emacs-28, rather than being a "backward-incompatible change".


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

> More generally, I'm not at all convinced that the problem, as
> presented in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51308#14,
> indeed needs a solution that makes locate-file behave the same as it
> did in Emacs 27, because the *.eln files introduce new aspects into
> the stuff that 'load' and 'load-history' intend to support.

`load` does not use `locate-file-internal`, so it's not affected.


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sun, 13 Mar 2022 23:16:08 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > I object to such backward-incompatible change in a public API.  Please
> >> > revert that change.
> 
> [...]
> 
> > I mean locate-file-internal.
> 
> I don't think the -internal subrs can be counted as part of the public
> API.  Like -- functions, they're internal.

It breaks us.

> > And all that just because some test failed?
> 
> It's a real bug (breaking loadhist functionality).

We never discussed nor decided what should be the behavior of loadhist
when *.eln files are loaded.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  emacs-devel@gnu.org
> Date: Sun, 13 Mar 2022 22:57:48 -0400
> 
> > More generally, I'm not at all convinced that the problem, as
> > presented in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51308#14,
> > indeed needs a solution that makes locate-file behave the same as it
> > did in Emacs 27, because the *.eln files introduce new aspects into
> > the stuff that 'load' and 'load-history' intend to support.
> 
> `load` does not use `locate-file-internal`, so it's not affected.

I don't see how this answers the concerns, sorry.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

Eli Zaretskii [2022-03-14 05:38:42] wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  emacs-devel@gnu.org
>> Date: Sun, 13 Mar 2022 22:57:48 -0400
>> > More generally, I'm not at all convinced that the problem, as
>> > presented in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51308#14,
>> > indeed needs a solution that makes locate-file behave the same as it
>> > did in Emacs 27, because the *.eln files introduce new aspects into
>> > the stuff that 'load' and 'load-history' intend to support.
>> `load` does not use `locate-file-internal`, so it's not affected.
> I don't see how this answers the concerns, sorry.

My point is that whatever we may want to do with `.el`, `.eln`, and
`.elc` files should be done without changing what `locate-file` does,
because `locate-file` is a generic function that looks for files through
a path, not a function that looks for ELisp-containing files.

And AFAICT all uses of `locate-file-internal` share that same desire
(more specifically I could only find 2 such uses: the one in
`locate-file` and one in `with-editor.el` which looks weird because it
seems it could just as well use `locate-file`).


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Sun, 13 Mar 2022 20:20:38 -0400
> 
> >> AFAIK it reverts the behavior of `locate-file-internal` to that of
> >> Emacs-27
> > Emacs 27 behavior cannot guide us here, because the *.eln files and
> > the aspects they bring into the issue are new.
> 
> `locate-file` is a function looks for files with a given set of
> extensions through path.  That's it.  Nothing specific to ELisp files.
> E.g. it's used in `executable-find`.
> 
> The existence of `.eln` should have no bearing on this.

Are you sure?  We never discussed this seriously.

E.g., is the result below expected, or is it a bug?

  (locate-file "simple" native-comp-eln-load-path '(".eln"))
    => nil

Or what about load-history -- why does it only show the *.elc files
loaded by the session, but not the *.eln files?  Should it?  (This
discussion was triggered by a conflict between load-history and
locate-file, so load-history is definitely relevant to the issue at
hand.)

> It can affect the ELisp-specific code (such as `load` or
> `find-library`), which may indeed use `locate-file` internally, but it
> should not affect `locate-file` itself.

It didn't until now.  But one can argue that it was only so because
the .el and its .elc file usually live in the same directory and have
the same basename.  That is no longer true with *.eln files, so we
should perhaps stop and think about this a bit harder.  E.g., maybe a
simpleton that locate-file is no longer satisfies us for looking up
Lisp files?

> >> so I'd even suggest we install my patch in `emacs-28`.
> > Do we want to release Emacs 28 any time soon, or do we want to
> > continue dragging our feet by making destabilizing changes in it?
> 
> Notice that I did not install it on `emacs-28` nor asked to do that.

Is there such a big difference between "ask" (which you didn't) and
"would suggest" (which you did)?

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

> E.g., is the result below expected, or is it a bug?
>
>   (locate-file "simple" native-comp-eln-load-path '(".eln"))
>     => nil

I assume you don't have a `simple.eln` file in your
`native-comp-eln-load-path`, so nil seems correct.

> Or what about load-history -- why does it only show the *.elc files
> loaded by the session, but not the *.eln files?

To better preserve compatibility.

> (This discussion was triggered by a conflict between load-history and
> locate-file, so load-history is definitely relevant to the issue
> at hand.)

The problem can't be "a conflict between load-history and locate-file".
There needs to be some code that links the two and tweaking
`locate-file` so it does what we happen to need can't the answer,
instead it has to be in the code that links the two.
Again `locate-file` is just a generic function, like
`locate-dominating-file`, or `file-attributes`.  If it doesn't do quite
what a specific function needs (e.g. `load` or `locate-library`), then
either that function needs to supplement `locate-file` with what it
needs, or it needs to use some other function.

You're right that the `openp` code probably needs further fixes to
better separate the `load` case from the other cases.  The way the
redirection from `.elc` to `.eln` was added to it is rather messy for
those uses that aren't `load`.  It happens to work OK largely because
executables tend not to use a `.elc` extension.

> should perhaps stop and think about this a bit harder.  E.g., maybe a
> simpleton that locate-file is no longer satisfies us for looking up
> Lisp files?

That's already the case: `load` supplements the code of `locate-file`
with some extra processing to look for a `.eln` after it found a `.elc`.

But in theory this replaces a call to `locate-file` with two calls to
that function (one to look for a file through `load-path` and then
another to look for a .eln version of the result through the
`native-comp-eln-load-path`), so it's not really an argument that
`locate-file` no longer satisfies us.
[ In practice it's implemented in a much more intertwined way in
  `openp`, and the second loop (through `native-comp-eln-load-path`
  doesn't reuse the code of locate-file/openp).  ]

> Is there such a big difference between "ask" (which you didn't) and
> "would suggest" (which you did)?

All I can say is that when I wrote that sentence I did not think it was
appropriate to put it right away into `emacs-28` (instead I thought,
like Lars, that it was better to see how it fares in `master` first).


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Mon, 14 Mar 2022 12:57:45 -0400
> 
> > E.g., is the result below expected, or is it a bug?
> >
> >   (locate-file "simple" native-comp-eln-load-path '(".eln"))
> >     => nil
> 
> I assume you don't have a `simple.eln` file in your
> `native-comp-eln-load-path`, so nil seems correct.

So you are saying that finding the .eln file produced from simple.el
is not something we should support, like we do with the *.elc files?
And/or that the tools which we use since day one for *.elc files
should not be extended to support the *.eln files?

> > Or what about load-history -- why does it only show the *.elc files
> > loaded by the session, but not the *.eln files?
> 
> To better preserve compatibility.

Compatibility with what?  The native-compilation feature explicitly
_breaks_ compatibility, in that it introduces a new kind of "compiled
Lisp" files, new places to look for them, and many other exciting
incompatibilities.  You cannot be compatible when such changes happen.

Anyway, I will now bow out of this discussion, since it's abundantly
clear we deeply disagree about quite a few of the involved aspects,
starting with what is the role and the purpose of locate-file.

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

>> I assume you don't have a `simple.eln` file in your
>> `native-comp-eln-load-path`, so nil seems correct.
> So you are saying that finding the .eln file produced from simple.el
> is not something we should support, like we do with the *.elc files?

`locate-file` is a function that doesn't rely on external
config variables.  It should do its job based exclusively on the
arguments it takes and the state of the file-system.
If you're looking for a file named `simple-<HASH>.eln` then you will
have to pass that <HASH> either in the first or the third arg.

It can definitely be a useful building block to find a `.eln` file, but
its job is not to find `.eln` files.

Are you confusing it with `locate-library` maybe?

>> > Or what about load-history -- why does it only show the *.elc files
>> > loaded by the session, but not the *.eln files?
>> To better preserve compatibility.
> Compatibility with what?

I can't remember but I remember that Andrea first had the `.eln` file in
there and then changed it to the `.elc` to fix some compatibility problems.


        Stefan

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Corwin Brust]

Hi Stefan & everyone,
I hope my questions aren't out of place (and that my message finds
everyone well :)

On Mon, Mar 14, 2022 at 1:12 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> I assume you don't have a `simple.eln` file in your
> >> `native-comp-eln-load-path`, so nil seems correct.
> > So you are saying that finding the .eln file produced from simple.el
> > is not something we should support, like we do with the *.elc files?
>
> `locate-file` is a function that doesn't rely on external
> config variables.  It should do its job based exclusively on the
> arguments it takes and the state of the file-system.

Are there cases where we can "lie to it", for example giving "false"
information about the state of the file-system by let-binding a
variable or something?

> If you're looking for a file named `simple-<HASH>.eln` then you will
> have to pass that <HASH> either in the first or the third arg.
>
> It can definitely be a useful building block to find a `.eln` file, but
> its job is not to find `.eln` files.

Are you suggesting a way forward could be to pass a pre-fabricated
collection of (e.g.) ELC to ELN mappings, eg. as an optional param to
locate-file?  Would it be equally acceptable to have that list in a
def{{var custom} that locate-file knows to look?  Can you share more
on why one or the other of those is "obviously worse/better" than the
other (if either is)?

Please do feel welcome to disregard if these questions are unhelpful
to moving the conversation forward.

FWIW, I think it would be nice to "easily discover" all of the
relevant source file locations for any natively compiled sources I may
have loaded.  That said, I do not seem to be sitting on any sort of
code toward solving for this, nor perhaps am I properly understanding
the problem space.

Appreciate all your guidance,
Corwin

Re: master f8bb6cca33: Return the same file from locate-file in nativecomp and non

[Stefan Monnier]

> Are there cases where we can "lie to it", for example giving "false"
> information about the state of the file-system by let-binding a
> variable or something?

No.

>> It can definitely be a useful building block to find a `.eln` file, but
>> its job is not to find `.eln` files.
> Are you suggesting a way forward could be to pass a pre-fabricated
> collection of (e.g.) ELC to ELN mappings, eg. as an optional param to
> locate-file?

No.  Why would you want that?
You can just as easily use such a mapping after calling the function.

> FWIW, I think it would be nice to "easily discover" all of the
> relevant source file locations for any natively compiled sources I may
> have loaded.

It should be easy to do that (tho it's not clear exactly what you mean
by that), but it's not the job of `locate-file`.


        Stefan