"If i had more time i would have written a shorter commit message."

Jonas Bernoulli (2):
  Add fix from faabc69ba to compat--generate-verbose too
  Fix using compat libraries uncompiled

 compat-macs.el | 54 ++++++++++++++++++++++++++------------------------
 compat.el      | 30 ++++++++++++++++++----------
 2 files changed, 48 insertions(+), 36 deletions(-)

-- 
2.35.1

---
 compat-macs.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat-macs.el b/compat-macs.el
index 4f2d44e..fec167a 100644
--- a/compat-macs.el
+++ b/compat-macs.el
@@ -166,7 +166,7 @@ (defun compat--generate-verbose (name def-fn install-fn check-fn attr type)
                          ((string-match
                            "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'"
                            file)
-                          (match-string 1 file))
+                          (concat (match-string 1 file) ".1"))
                          ((error "No version number could be extracted"))))))
          (realname (or (plist-get attr :realname)
                        (intern (format "compat--%S" name))))
-- 
2.35.1

`compat-entwine' is called in two and only three distinct situations.

1. When "compat.el" is being loaded without having been compiled
   before.  This can happen while a third-party library is being
   compiled, which requires `compat'.

2. When "compat.el" is being evaluated, such as when using
   `eval-buffer'.

3. When "compat.el" is being compiled.

In all cases we need to detect where "compat.el" is located, because
that also tells us where the "compat-XY.el" libraries are located and
`compat-entwine' needs to read the contents of those files.  To detect
the location of "compat.el" we consult various variables, which we
expect to either be nil or "/path/to/compat.el".  We didn't use the
correct variables.

Some time ago Stefan added a call to `macro-file-name' to the front
of that search, which fixes the search for Emacs 28.1.  Older Emacs
releases lack that function, so for those we still ended up consulting
the wrong variables.  To fix that use `current-load-list' directly,
treating it the same way as `macro-file-name' does.

If that gives us a non-nil value, then we are dealing with one of the
first two cases and we are done.  Otherwise we are dealing with the
third case and `byte-compile-current-file' gives us the correct value.

One of these sources is always non-nil, so drop the variables we used
to fall back to.

Then we pass the version from `compat-entwine' to the
`compat--generate-function' function by binding the new variable
`compat--entwine-version'.  Previously this was done by binding
`byte-compile-current-file`.  The caller then had to extract the
version from the name of that file.

Binding `byte-compile-current-file` was also part of the bug that
prevented compilation of a third-party library, which depends on
"compat-XY.el", when compat's libraries have not been compiled yet.

To fix that we have to change the order of the possible sources of the
version string in `compat--generate-function' functions.  As before we
use the version explicitly specified for the function being processed,
if any.  Then we use value of the new `compat--entwine-version'
variable, if that is bound and non-nil, i.e., if the binding from
`compat-entwine' is in effect.

Previously we would have tried `byte-compile-current-file' at this
point instead, but while that might be non-nil because we used to bind
it in `compat--generate-function', it could also be non-nil because an
arbitrary third-party library is being compiled.

That is a problem if that third-party library depends on
"compat-XY.el" and the latter hasn't been compiled before: The
`compat--generate-function' function is called directly in that case,
without the binding from `compat-entwine' in effect.  We need
`byte-compile-current-file' to be some "compat-XY.el" library; not
some arbitrary third-party library.

Then we try `current-load-list' like in `compat-entwine', which here
is some "compat-XY.el" library when some third-party library is being
compiled and that requires that "compat-XY.el" library.

Finally, if we could not determine a version any other way, then we
know that some "compat-XY.el" library is being compiled.  In that
case, and only then, we can get the version string from
`byte-compile-current-file'.
---
 compat-macs.el | 54 ++++++++++++++++++++++++++------------------------
 compat.el      | 30 ++++++++++++++++++----------
 2 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/compat-macs.el b/compat-macs.el
index fec167a..85f31b8 100644
--- a/compat-macs.el
+++ b/compat-macs.el
@@ -82,19 +82,22 @@ (defun compat--generate-minimal (name def-fn install-fn check-fn attr type)
          (max-version (plist-get attr :max-version))
          (feature (plist-get attr :feature))
          (cond (plist-get attr :cond))
-         (version (or (plist-get attr :version)
-                      (let ((file (or (bound-and-true-p byte-compile-current-file)
-                                      load-file-name
-                                      (buffer-file-name))))
-                        ;; Guess the version from the file the macro is
-                        ;; being defined in.
-                        (cond
-                         ((not file) emacs-version)
-                         ((string-match
-                           "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'"
-                           file)
-                          (concat (match-string 1 file) ".1"))
-                         ((error "No version number could be extracted"))))))
+         (version ; If you edit this, also edit `compat--generate-verbose'.
+          (or (plist-get attr :version)
+              (bound-and-true-p compat--entwine-version)
+              (let* ((file (car (last current-load-list)))
+                     (file (if (stringp file)
+                               ;; Some library, which requires compat-XY.el,
+                               ;; is being compiled and compat-XY.el has not
+                               ;; been compiled yet.
+                               file
+                             ;; compat-XY.el is being compiled.
+                             (bound-and-true-p byte-compile-current-file))))
+                (if (and file
+                         (string-match
+                          "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'" file))
+                    (concat (match-string 1 file) ".1")
+                  (error "BUG: No version number could be extracted")))))
          (realname (or (plist-get attr :realname)
                        (intern (format "compat--%S" name))))
          (check (cond
@@ -155,19 +158,18 @@ (defun compat--generate-verbose (name def-fn install-fn check-fn attr type)
          (max-version (plist-get attr :max-version))
          (feature (plist-get attr :feature))
          (cond (plist-get attr :cond))
-         (version (or (plist-get attr :version)
-                      (let ((file (or (bound-and-true-p byte-compile-current-file)
-                                      load-file-name
-                                      (buffer-file-name))))
-                        ;; Guess the version from the file the macro is
-                        ;; being defined in.
-                        (cond
-                         ((not file) emacs-version)
-                         ((string-match
-                           "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'"
-                           file)
-                          (concat (match-string 1 file) ".1"))
-                         ((error "No version number could be extracted"))))))
+         (version ; If you edit this, also edit `compat--generate-minimal'.
+          (or (plist-get attr :version)
+              (bound-and-true-p compat--entwine-version)
+              (let* ((file (car (last current-load-list)))
+                     (file (if (stringp file)
+                               file
+                             (bound-and-true-p byte-compile-current-file))))
+                (if (and file
+                         (string-match
+                          "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'" file))
+                    (concat (match-string 1 file) ".1")
+                  (error "BUG: No version number could be extracted")))))
          (realname (or (plist-get attr :realname)
                        (intern (format "compat--%S" name))))
          (body `(progn
diff --git a/compat.el b/compat.el
index ed60ba5..a5d6346 100644
--- a/compat.el
+++ b/compat.el
@@ -49,6 +49,7 @@ (eval-when-compile (require 'compat-macs))
 ;; separately, by explicitly requiring the feature that defines them.
 (eval-when-compile
   (defvar compat--generate-function)
+  (defvar compat--entwine-version)
   (defmacro compat-entwine (version)
     (cond
      ((or (not (eq compat--generate-function 'compat--generate-minimal))
@@ -58,11 +59,24 @@ (eval-when-compile
              (file (expand-file-name
                     (format "compat-%d.el" version)
                     (file-name-directory
-                     (or (if (fboundp 'macroexp-file-name)
-                             (macroexp-file-name)
-                           (or (bound-and-true-p byte-compile-current-file)
-                               load-file-name))
-                         (buffer-file-name)))))
+                     (or
+                      ;; Some third-party library, which requires
+                      ;; compat.el, is being compiled, loaded or
+                      ;; evaluated, and compat.el hasn't been compiled
+                      ;; yet.
+                      ;;   cd compat && make clean && cd ../other && \
+                      ;;   make clean all
+                      ;;
+                      ;; Or compat.el is being evaluated.
+                      ;;   cd compat && make clean && emacs -Q -L . compat.el
+                      ;;   M-x eval-buffer
+                      ;;
+                      ;; (Like `macroexp-file-name' from Emacs 28.1.)
+                      (let ((file (car (last current-load-list))))
+                        (and (stringp file) file))
+                      ;; compat.el is being compiled.
+                      ;;   cd compat && make clean all
+                      (bound-and-true-p byte-compile-current-file)))))
              defs)
         (with-temp-buffer
           (insert-file-contents file)
@@ -70,11 +84,7 @@ (eval-when-compile
           (while (progn
                    (forward-comment 1)
                    (not (eobp)))
-            ;; We bind `byte-compile-current-file' before
-            ;; macro-expanding, so that `compat--generate-function'
-            ;; can correctly infer the compatibility version currently
-            ;; being processed.
-            (let ((byte-compile-current-file file)
+            (let ((compat--entwine-version (number-to-string version))
                   (form (read (current-buffer))))
               (cond
                ((memq (car-safe form)
-- 
2.35.1

Jonas Bernoulli <jonas@bernoul.li> writes:

> `compat-entwine' is called in two and only three distinct situations.

Please fix s/two/three/ before merging. :P

Of course also feel free to remove the comments, trim down the commit
message, or make other changes.

(Sorry for the late response, I had to find the time to try and
understand everything you were proposing here)

Jonas Bernoulli <jonas@bernoul.li> writes:

> "If i had more time i would have written a shorter commit message."
>
> Jonas Bernoulli (2):
>   Add fix from faabc69ba to compat--generate-verbose too
>   Fix using compat libraries uncompiled
>
>  compat-macs.el | 54 ++++++++++++++++++++++++++------------------------
>  compat.el      | 30 ++++++++++++++++++----------
>  2 files changed, 48 insertions(+), 36 deletions(-)

Jonas Bernoulli <jonas@bernoul.li> writes:

> ---
>  compat-macs.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat-macs.el b/compat-macs.el
> index 4f2d44e..fec167a 100644
> --- a/compat-macs.el
> +++ b/compat-macs.el
> @@ -166,7 +166,7 @@ (defun compat--generate-verbose (name def-fn install-fn check-fn attr type)
>                           ((string-match
>                             "compat-\\([[:digit:]]+\\)\\.\\(?:elc?\\)\\'"
>                             file)
> -                          (match-string 1 file))
> +                          (concat (match-string 1 file) ".1"))
>                           ((error "No version number could be extracted"))))))
>           (realname (or (plist-get attr :realname)
>                         (intern (format "compat--%S" name))))

Thank you for catching this, I had added it to the other function
because I was awaiting a response from another thread:

https://lists.sr.ht/~pkal/compat-devel/%3Cf8635d7d-e233-448f-b325-9e850363241c%40www.fastmail.com%3E

Jonas Bernoulli <jonas@bernoul.li> writes:

> `compat-entwine' is called in two and only three distinct situations.
>
> 1. When "compat.el" is being loaded without having been compiled
>    before.  This can happen while a third-party library is being
>    compiled, which requires `compat'.
>
> 2. When "compat.el" is being evaluated, such as when using
>    `eval-buffer'.
>
> 3. When "compat.el" is being compiled.
>

[...]

> Finally, if we could not determine a version any other way, then we
> know that some "compat-XY.el" library is being compiled.  In that
> case, and only then, we can get the version string from
> `byte-compile-current-file'.

Just to be sure: The issue we are dealing with here is that entwine
binds `byte-compile-current-file', and you are saying that the macro
generator functions assume the version is communicated using these
files, right?

What I don't understand is how the

--8<---------------cut here---------------start------------->8---
(or (bound-and-true-p byte-compile-current-file)
    load-file-name
    (buffer-file-name))
--8<---------------cut here---------------end--------------->8---

doesn't do the right thing?  If it is not being byte-compiled, shouldn't
`load-file-name' be bound to the right file (some "compat-XY.el")?

The other problem I have is that I am uncertain how to reproduce the
issue.  Even if I just load compat-XY.el in some older emacs -Q,
everything seems to work just fine, so I guess I am missing something (I
assume, as you appear to have put a lot of thought into this patch)?

Jonas Bernoulli <jonas@bernoul.li> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> `compat-entwine' is called in two and only three distinct situations.
>
> Please fix s/two/three/ before merging. :P

Will do.

> Of course also feel free to remove the comments, trim down the commit
> message, or make other changes.

I'd rather keep it the way it is, I just want to understand the changes.

Philip Kaludercic <philipk@posteo.net> writes:
> Just to be sure: The issue we are dealing with here is that entwine
> binds `byte-compile-current-file', and you are saying that the macro
> generator functions assume the version is communicated using these
> files, right?

Yes.

> What I don't understand is how the
>
> --8<---------------cut here---------------start------------->8---
> (or (bound-and-true-p byte-compile-current-file)
>     load-file-name
>     (buffer-file-name))
> --8<---------------cut here---------------end--------------->8---
>
> doesn't do the right thing?  If it is not being byte-compiled, shouldn't
> `load-file-name' be bound to the right file (some "compat-XY.el")?

Yes, but we try `byte-compile-current-file' first and that is bound and
non-nil too.

If "other.el" is being compiled, it requires compat-XY, and "compat*.el"
have not been compiled yet, then `byte-compile-current-file' is
"/path/to/other.el" and the `compat--generate-function' is evaluated
with that value in effect.

> The other problem I have is that I am uncertain how to reproduce the
> issue.  Even if I just load compat-XY.el in some older emacs -Q,
> everything seems to work just fine, so I guess I am missing something (I
> assume, as you appear to have put a lot of thought into this patch)?

$ cd compat
$ make clean
$ cd ../magit
$ make all
make[1]: Entering directory '/home/jonas/.emacs.d/lib/magit/lisp'
Compiling git-commit.el
Compiling magit-section.el
Eager macro-expansion failure: (error "No version number could be extracted")
Eager macro-expansion failure: (error "No version number could be extracted")

In toplevel form:
magit-section.el:43:2: Error: No version number could be extracted
make[1]: *** [Makefile:77: magit-section.elc] Error 1
make[1]: Leaving directory '/home/jonas/.emacs.d/lib/magit/lisp'
make: *** [Makefile:74: lisp] Error 2

Jonas Bernoulli <jonas@bernoul.li> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>> Just to be sure: The issue we are dealing with here is that entwine
>> binds `byte-compile-current-file', and you are saying that the macro
>> generator functions assume the version is communicated using these
>> files, right?
>
> Yes.
>
>> What I don't understand is how the
>>
>> --8<---------------cut here---------------start------------->8---
>> (or (bound-and-true-p byte-compile-current-file)
>>     load-file-name
>>     (buffer-file-name))
>> --8<---------------cut here---------------end--------------->8---
>>
>> doesn't do the right thing?  If it is not being byte-compiled, shouldn't
>> `load-file-name' be bound to the right file (some "compat-XY.el")?
>
> Yes, but we try `byte-compile-current-file' first and that is bound and
> non-nil too.
>
> If "other.el" is being compiled, it requires compat-XY, and "compat*.el"
> have not been compiled yet, then `byte-compile-current-file' is
> "/path/to/other.el" and the `compat--generate-function' is evaluated
> with that value in effect.

Ah, now I understand the issue, you are absolutely right.  I will push
the patches as is.  Thank you very much for spotting the bug and solving
it!

>> The other problem I have is that I am uncertain how to reproduce the
>> issue.  Even if I just load compat-XY.el in some older emacs -Q,
>> everything seems to work just fine, so I guess I am missing something (I
>> assume, as you appear to have put a lot of thought into this patch)?
>
> $ cd compat
> $ make clean
> $ cd ../magit
> $ make all
> make[1]: Entering directory '/home/jonas/.emacs.d/lib/magit/lisp'
> Compiling git-commit.el
> Compiling magit-section.el
> Eager macro-expansion failure: (error "No version number could be extracted")
> Eager macro-expansion failure: (error "No version number could be extracted")
>
> In toplevel form:
> magit-section.el:43:2: Error: No version number could be extracted
> make[1]: *** [Makefile:77: magit-section.elc] Error 1
> make[1]: Leaving directory '/home/jonas/.emacs.d/lib/magit/lisp'
> make: *** [Makefile:74: lisp] Error 2

Hi Jonas,

if you could find some time, could you check out this issue on the
compat issue tracker: https://todo.sr.ht/~pkal/compat/4.  I believe the
last message ~temporal might be related to these patches.

Philip Kaludercic <philipk@posteo.net> writes:

> Hi Jonas,
>
> if you could find some time, could you check out this issue on the
> compat issue tracker: https://todo.sr.ht/~pkal/compat/4.  I believe the
> last message ~temporal might be related to these patches.

I am taking a month or two off and I don't think it would be a good
idea for me to already make an exceptions only a few days in.  Sorry.

     Jonas

Jonas Bernoulli <jonas@bernoul.li> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Hi Jonas,
>>
>> if you could find some time, could you check out this issue on the
>> compat issue tracker: https://todo.sr.ht/~pkal/compat/4.  I believe the
>> last message ~temporal might be related to these patches.
>
> I am taking a month or two off and I don't think it would be a good
> idea for me to already make an exceptions only a few days in.  Sorry.

Noted, I totally understand it, no issue whatsoever.  I think the issue
has been resolved anyway.

Have a good time.

>      Jonas