bug#72045: Emacs graft lookup still fails

bug#72045: Emacs graft lookup still fails

[Liliana Marie Prikler]

Hi Guix,

this got reported in the XMPP chat already, but the basic gist is this:
with the grafting of Emacs 29.3 to 29.4, we see that Emacs itself is
still correctly loaded, but Emacs libraries (e.g. dash) aren't.

(comp-el-to-eln-filename (expand-file-name "…/dash.el"))
=> $HOME/.config/emacs/eln-cache/29.4-46e5bcbe/dash-2.19.1/dash.eln

find $(guix build emacs-dash --with-input=…) -name 'dash.eln'
=> $PREFIX/lib/emacs/native-site-lisp/29.3-62809b9a/dash.eln

It seems that we might have to rebuild emacs native-compiled packages
even if emacs itself is grafted.  Do we have any pointers on how to
correctly process this graft?

Cheers

bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and native version dir without version.

[Liliana Marie Prikler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

* gnu/packages/patches/emacs-native-comp-fix-filenames.patch: Drop
emacs_version from Vcomp_abi_hash.
Make Vcomp_native_version_dir equal to Vcomp_abi_hash.
---
Hi Guix,

this is a somewhat experimental patch, which reduces Vcomp_native_version_dir
to simply Vcomp_abi_hash.  Note, that this is not enough to address the
issues currently noticed with Emacs native compilation, as Vcomp_abi_hash
is unstable between 29.3 and 29.4.  These hashes are probably unlikely
to stay the same between minor releases even when dropping the version.

Cheers

 .../emacs-native-comp-fix-filenames.patch     | 27 ++++++++++++++++---
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/gnu/packages/patches/emacs-native-comp-fix-filenames.patch b/gnu/packages/patches/emacs-native-comp-fix-filenames.patch
index 169323f290..6c81d7c28c 100644
--- a/gnu/packages/patches/emacs-native-comp-fix-filenames.patch
+++ b/gnu/packages/patches/emacs-native-comp-fix-filenames.patch
@@ -12,11 +12,30 @@ way into the actual variable despite attempts to remove it by calling
 The user-visible procedure ‘startup-redirect-eln-cache’ is kept, as
 packages may require it, but only pushes the new value now.
 
-Index: emacs-29.2/src/comp.c
+Index: emacs-29.3/src/comp.c
 ===================================================================
---- emacs-29.2.orig/src/comp.c
-+++ emacs-29.2/src/comp.c
-@@ -4396,26 +4396,17 @@ DEFUN ("comp-el-to-eln-rel-filename", Fc
+--- emacs-29.3.orig/src/comp.c
++++ emacs-29.3/src/comp.c
+@@ -805,7 +805,7 @@ hash_native_abi (void)
+   Vcomp_abi_hash =
+     comp_hash_string (
+       concat3 (build_string (ABI_VERSION),
+-	       concat3 (Vemacs_version, Vsystem_configuration,
++	       concat2 (Vsystem_configuration,
+ 			Vsystem_configuration_options),
+ 	       Fmapconcat (intern_c_string ("comp--subr-signature"),
+ 			   Vcomp_subr_list, build_string (""))));
+@@ -835,8 +835,7 @@ hash_native_abi (void)
+     }
+ #endif
+ 
+-  Vcomp_native_version_dir =
+-    concat3 (version, build_string ("-"), Vcomp_abi_hash);
++  Vcomp_native_version_dir = Vcomp_abi_hash;
+ }
+ 
+ static void
+@@ -4396,26 +4395,17 @@ DEFUN ("comp-el-to-eln-rel-filename", Fc
         Scomp_el_to_eln_rel_filename, 1, 1, 0,
         doc: /* Return the relative name of the .eln file for FILENAME.
  FILENAME must exist, and if it's a symlink, the target must exist.

base-commit: a1e6ac72fd88faf20c26e235f5c8222881b2b450
-- 
2.45.2

bug#72045: [PATCH 2/2] gnu: emacs-minimal: Ungraft.

[Liliana Marie Prikler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

The current graft breaks native compilation and would do so even if reduced to
an ABI hash.  Thus remove it, and rebuild all Emacsen.

* gnu/packages/emacs.scm (emacs-minimal): Update to 29.4.
[replacement]: Remove.  Add note for future replacements.
(emacs-minimal/fixed): Remove variable.

Fixes: Emacs native compilation across grafts <https://bugs.gnu.org/72045>
---
 gnu/packages/emacs.scm | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index ed186d221c..33bb0dd542 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -100,15 +100,16 @@ (define (%emacs-modules build-system)
 (define-public emacs-minimal
   (package
     (name "emacs-minimal")
-    (version "29.3")
-    (replacement emacs-minimal/fixed)
+    (version "29.4")
+    ;; Note: When using (replacement …), ensure that comp-native-version-dir
+    ;; stays the same across grafts.
     (source (origin
               (method url-fetch)
               (uri (string-append "mirror://gnu/emacs/emacs-"
                                   version ".tar.xz"))
               (sha256
                (base32
-                "1822swrk4ifmkd4h9l0h37zifcpa1w3sy3vsgyffsrp6mk9hak63"))
+                "0dd2mh6maa7dc5f49qdzj7bi4hda4wfm1cvvgq560djcz537k2ds"))
               (patches (search-patches "emacs-disable-jit-compilation.patch"
                                        "emacs-exec-path.patch"
                                        "emacs-fix-scheme-indent-function.patch"
@@ -335,18 +336,6 @@ (define-public emacs-minimal
             (files '("lib/tree-sitter")))))
     (properties `((upstream-name . "emacs")))))
 
-(define emacs-minimal/fixed
-  (package
-    (inherit emacs-minimal)
-    (version "29.4")
-    (source
-     (origin (inherit (package-source emacs-minimal))
-             (uri (string-append "mirror://gnu/emacs/emacs-"
-                                 version ".tar.xz"))
-             (sha256
-              (base32
-               "0dd2mh6maa7dc5f49qdzj7bi4hda4wfm1cvvgq560djcz537k2ds"))))))
-
 (define-public emacs-no-x
   (package/inherit emacs-minimal
     (name "emacs-no-x")
-- 
2.45.2

bug#72045: [PATCH 2/2] gnu: emacs-minimal: Ungraft.

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> +    ;; Note: When using (replacement …), ensure that comp-native-version-dir
> +    ;; stays the same across grafts.

Perhaps an acknowledgment that we don't yet know how to accomplish that
would be helpful?  Unless I have misunderstood the current state and am
mistaken.

-- 
Suhail

bug#72045: [PATCH 2/2] gnu: emacs-minimal: Ungraft.

[Liliana Marie Prikler]

Am Samstag, dem 13.07.2024 um 12:14 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > +    ;; Note: When using (replacement …), ensure that comp-native-
> > version-dir
> > +    ;; stays the same across grafts.
> 
> Perhaps an acknowledgment that we don't yet know how to accomplish
> that would be helpful?  Unless I have misunderstood the current state
> and am mistaken.
I think as long as the package version and function signatures remain
unchanged, we can graft Emacs itself.  But it is limited to minor
patches; no version bumps.

Cheers

bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and native version dir without version.

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> this is a somewhat experimental patch, which reduces Vcomp_native_version_dir
> to simply Vcomp_abi_hash.  Note, that this is not enough to address the
> issues currently noticed with Emacs native compilation, as Vcomp_abi_hash
> is unstable between 29.3 and 29.4.  These hashes are probably unlikely
> to stay the same between minor releases even when dropping the version.

I am confused as to what this change is intended to accomplish in that
case.  Specifically, while this patch may be _necessary_ to allow both
grafts and native-compilation to work since it isn't _sufficient_ to
accomplish that, it may be better to withhold the patch till a later
time when an appropriate fix for dealing with grafts has been found.

-- 
Suhail

bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and native version dir without version.

[Liliana Marie Prikler]

Am Samstag, dem 13.07.2024 um 12:08 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > this is a somewhat experimental patch, which reduces
> > Vcomp_native_version_dir to simply Vcomp_abi_hash.  Note, that this
> > is not enough to address the issues currently noticed with Emacs
> > native compilation, as Vcomp_abi_hash is unstable between 29.3 and
> > 29.4.  These hashes are probably unlikely to stay the same between
> > minor releases even when dropping the version.
> 
> I am confused as to what this change is intended to accomplish in
> that case.  Specifically, while this patch may be _necessary_ to
> allow both grafts and native-compilation to work since it isn't
> _sufficient_ to accomplish that, it may be better to withhold the
> patch till a later time when an appropriate fix for dealing with
> grafts has been found.
It could possibly allow us some flexibility in future changes, but at
the very least it doesn't work right now.  Hence why it's RFC while the
other patch isn't.

Cheers

bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and native version dir without version.

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> It could possibly allow us some flexibility in future changes, but at
> the very least it doesn't work right now.  Hence why it's RFC while
> the other patch isn't.

Thank you for clarifying.  It is clear that different versions of Emacs
store the natively compiled files in different locations.  However, it's
less clear what happens during grafting and what is desired for grafting
and native compilation to work.

What is needed for grafting to work with native compilation?  Is the
intent to be able to reuse the natively compiled files from the original
version when grafting installs the replacement version?

What happens during grafting today?  Is the location where the original
version kept its natively compiled files kept around, or does it get
deleted when the replacement is grafted?

-- 
Suhail

bug#72045: Emacs graft lookup still fails

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> with the grafting of Emacs 29.3 to 29.4, we see that Emacs itself is
> still correctly loaded, but Emacs libraries (e.g. dash) aren't.
>
> (comp-el-to-eln-filename (expand-file-name "…/dash.el"))
> => $HOME/.config/emacs/eln-cache/29.4-46e5bcbe/dash-2.19.1/dash.eln
>
> find $(guix build emacs-dash --with-input=…) -name 'dash.eln'
> => $PREFIX/lib/emacs/native-site-lisp/29.3-62809b9a/dash.eln
>
> It seems that we might have to rebuild emacs native-compiled packages
> even if emacs itself is grafted.

I had missed this message, previously.

IIUC, the issue is that replacement packages are grafted post-build.
This means that when emacs-dash is built, its AOT native-compilation
happens with Emacs 29.3.  However, at run-time Emacs 29.4 gets grafted
in.

There are at least two possible ways (ignoring feasibility) to resolve
this:

1. When emacs-dash etc. is being built we use Emacs 29.4 for native
   compilation.

2. When emacs-dash etc. is being built we use Emacs 29.3 for native
   compilation, but ensure that said files are transferred to a location
   where Emacs 29.4 is able to find them.

Which do we desire?  My belief is that 1 is what we need, and that doing
2 may be inadequate for ensuring that appropriate security fixes are
deployed (consider the case where the bug is in a macro in Emacs core).

If my analysis above is correct, then the question (which I don't know
the answer to) is can 1 be accomplished with grafts?

--
Suhail

bug#72045: Emacs graft lookup still fails

[Liliana Marie Prikler]

Am Samstag, dem 13.07.2024 um 19:22 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > with the grafting of Emacs 29.3 to 29.4, we see that Emacs itself
> > is still correctly loaded, but Emacs libraries (e.g. dash) aren't.
> > 
> > (comp-el-to-eln-filename (expand-file-name "…/dash.el"))
> > => $HOME/.config/emacs/eln-cache/29.4-46e5bcbe/dash-2.19.1/dash.eln
> > 
> > find $(guix build emacs-dash --with-input=…) -name 'dash.eln'
> > => $PREFIX/lib/emacs/native-site-lisp/29.3-62809b9a/dash.eln
> > 
> > It seems that we might have to rebuild emacs native-compiled
> > packages even if emacs itself is grafted.
> 
> I had missed this message, previously.
> 
> IIUC, the issue is that replacement packages are grafted post-build.
> This means that when emacs-dash is built, its AOT native-compilation
> happens with Emacs 29.3.  However, at run-time Emacs 29.4 gets
> grafted in.
Nitpick: Emacs 29.4 gets grafted in at profile-building time.

> There are at least two possible ways (ignoring feasibility) to
> resolve this:
> 
> 1. When emacs-dash etc. is being built we use Emacs 29.4 for native
>    compilation.
That kinda defeats the point of grafting, though.  At this point,
rebuilding with newer Emacs makes more sense.

> 2. When emacs-dash etc. is being built we use Emacs 29.3 for native
>    compilation, but ensure that said files are transferred to a
>    location where Emacs 29.4 is able to find them.
Given that the ABI hash is used to guard against loading outdated
libraries like this, I'm not sure whether this makes too much sense.  I
think what we would need is something like 

3. Accurately capture the compatibility between Emacs-used-to-compile
   and Emacs-used-to-run.  I.e. find a way to enable Emacs cross
   compilation.

Perhaps upstream already has some ideas on this, perhaps not.

> Which do we desire?  My belief is that 1 is what we need, and that
> doing 2 may be inadequate for ensuring that appropriate security
> fixes are deployed (consider the case where the bug is in a macro in
> Emacs core).
I think 1 could be accomplished with a build system hack, but see
above.

Cheers

bug#72045: Emacs graft lookup still fails

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

>> IIUC, the issue is that replacement packages are grafted post-build.
>> This means that when emacs-dash is built, its AOT native-compilation
>> happens with Emacs 29.3.  However, at run-time Emacs 29.4 gets
>> grafted in.
> Nitpick: Emacs 29.4 gets grafted in at profile-building time.

Agreed; thanks for the correction.

>> There are at least two possible ways (ignoring feasibility) to
>> resolve this:
>> 
>> 1. When emacs-dash etc. is being built we use Emacs 29.4 for native
>>    compilation.
> That kinda defeats the point of grafting, though.  At this point,
> rebuilding with newer Emacs makes more sense.

I agree, and that is what I am leaning towards.  The main concern I have
is that it's not directly apparent based on the package version whether
the ABI_VERSION has been bumped or not.  As such, any time a Guix
packager proposes a replacement, the patch reviewer has to manually
review the Emacs source to ensure that the ABI_VERSION has not been
bumped.  Unless there is an automated way to ensure that, this would
increase the maintenance overhead in Guix (as compared to a comment
noting that grafts for Emacs aren't recommended).

However, perhaps there is a way to ensure that the proposed replacement
doesn't have a different ABI_VERSION.  Could this be caught by a test or
"sanity checker" of some kind?

>> 2. When emacs-dash etc. is being built we use Emacs 29.3 for native
>>    compilation, but ensure that said files are transferred to a
>>    location where Emacs 29.4 is able to find them.
> Given that the ABI hash is used to guard against loading outdated
> libraries like this, I'm not sure whether this makes too much sense.  I
> think what we would need is something like 
>
> 3. Accurately capture the compatibility between Emacs-used-to-compile
>    and Emacs-used-to-run.  I.e. find a way to enable Emacs cross
>    compilation.

I see.  Now your RFC patch regd. dropping the version prefix from the
.eln path makes sense.  The intent being to allow grafting to work with
AOT native compilation as long as ABI_VERSION remains the same.

> Perhaps upstream already has some ideas on this, perhaps not.

Hopefully upstream also has some thoughts as to where assertions
regd. the validity of package replacements could be tested.

>> Which do we desire?  My belief is that 1 is what we need, and that
>> doing 2 may be inadequate for ensuring that appropriate security
>> fixes are deployed (consider the case where the bug is in a macro in
>> Emacs core).
> I think 1 could be accomplished with a build system hack, but see
> above.

Noted, thank you for the elaboration.

-- 
Suhail

bug#72045: Emacs graft lookup still fails

[Liliana Marie Prikler]

Am Sonntag, dem 14.07.2024 um 12:27 -0400 schrieb Suhail Singh:
> However, perhaps there is a way to ensure that the proposed
> replacement doesn't have a different ABI_VERSION.  Could this be
> caught by a test or "sanity checker" of some kind?
You could just print the value of comp-native-version-dir.  If that
changes between ungrafted and grafted emacs, then the graft is NG (no
good).

Cheers
> > 

bug#72045: Emacs graft lookup still fails

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Sonntag, dem 14.07.2024 um 12:27 -0400 schrieb Suhail Singh:
>> However, perhaps there is a way to ensure that the proposed
>> replacement doesn't have a different ABI_VERSION.  Could this be
>> caught by a test or "sanity checker" of some kind?
> You could just print the value of comp-native-version-dir.  If that
> changes between ungrafted and grafted emacs, then the graft is NG (no
> good).

Yes, of course.  The part that's not clear to me (due to my limited
understanding of Guix internals) is how to trigger it at the time the
package replacement is being considered / done.  Perhaps that would be
the responsiblity of the emacs-build-system.

-- 
Suhail

bug#72045: Emacs graft lookup still fails

[Suhail Singh]

Suhail Singh <suhailsingh247@gmail.com> writes:

> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>
>> Am Sonntag, dem 14.07.2024 um 12:27 -0400 schrieb Suhail Singh:
>>> However, perhaps there is a way to ensure that the proposed
>>> replacement doesn't have a different ABI_VERSION.  Could this be
>>> caught by a test or "sanity checker" of some kind?
>> You could just print the value of comp-native-version-dir.  If that
>> changes between ungrafted and grafted emacs, then the graft is NG (no
>> good).
>
> Yes, of course.  The part that's not clear to me (due to my limited
> understanding of Guix internals) is how to trigger it at the time the
> package replacement is being considered / done.  Perhaps that would be
> the responsiblity of the emacs-build-system.

Whoops.  Of course not emacs-build-system, since Emacs uses
glib-or-gtk-build-system.

-- 
Suhail

bug#72045: [PATCH v2 1/2] gnu: Add system test for Emacs.

[Liliana Marie Prikler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]

* gnu/tests/emacs.scm: New file.
---
Hi Guix,

this series adds a system test to ensure that Emacs grafts are meaningful.
With this, we can make safe decisions as to whether or not place
(replacement …)

Cheers

 gnu/tests/emacs.scm | 100 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 gnu/tests/emacs.scm

diff --git a/gnu/tests/emacs.scm b/gnu/tests/emacs.scm
new file mode 100644
index 0000000000..fba27cefd8
--- /dev/null
+++ b/gnu/tests/emacs.scm
@@ -0,0 +1,100 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2024 Liliana Marie Prikler <liliana.prikler@gmail.com>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (gnu tests emacs)
+  #:use-module (gnu tests)
+  #:use-module (gnu packages emacs)
+  #:use-module (gnu packages vim)
+  #:use-module (gnu services)
+  #:use-module (gnu system)
+  #:use-module (gnu system vm)
+  #:use-module (guix packages)
+  #:use-module (guix gexp)
+  #:use-module (srfi srfi-1)
+  #:export (%test-emacs-native-comp-replacable))
+
+(define (run-native-comp-replacable-test old-emacs new-emacs)
+  (define vm (virtual-machine (marionette-operating-system %simple-os)))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (gnu build marionette)
+                       (srfi srfi-1)
+                       (srfi srfi-64))
+
+          (define marionette (make-marionette (list #$vm)))
+          (define (emacs-native-comp-dir emacs)
+            (marionette-eval
+             `(begin
+                (use-modules (ice-9 rdelim) (ice-9 popen))
+                (read-line
+                 (open-pipe*
+                  OPEN_READ
+                  ,emacs "--batch"
+                  "--eval=(princ comp-native-version-dir)")))
+             marionette))
+          (define (emacs-effective-version emacs)
+            (marionette-eval
+             `(begin
+                (use-modules (ice-9 rdelim) (ice-9 popen))
+                (read-line
+                 (open-pipe*
+                  OPEN_READ
+                  ,emacs "--batch"
+                  "--eval=(princ (format \"%s.%s\" \
+                           emacs-major-version emacs-minor-version))")))
+             marionette))
+          (define old-emacs-bin #$(file-append old-emacs "/bin/emacs"))
+          (define new-emacs-bin #$(file-append new-emacs "/bin/emacs"))
+
+          (test-runner-current (system-test-runner #$output))
+          (test-begin "emacs-native-comp-replacable")
+          (test-equal "native-comp-dir"
+            (emacs-native-comp-dir
+             #$(file-append old-emacs "/bin/emacs"))
+            (emacs-native-comp-dir
+             #$(file-append new-emacs "/bin/emacs")))
+          (test-assert "old emacs has hierarchical layout"
+            (file-exists?
+             (string-append #$new-emacs "/lib/emacs/"
+                            (emacs-effective-version old-emacs-bin)
+                            "/native-lisp/"
+                            (emacs-native-comp-dir old-emacs-bin)
+                            "/preloaded/emacs-lisp/comp.eln")))
+          (test-assert "new emacs has hierarchical layout"
+            (file-exists?
+             (string-append #$new-emacs "/lib/emacs/"
+                            (emacs-effective-version new-emacs-bin)
+                            "/native-lisp/"
+                            (emacs-native-comp-dir new-emacs-bin)
+                            "/preloaded/emacs-lisp/comp.eln")))
+          (test-end))))
+
+  (gexp->derivation "emacs-native-comp-compatible" test))
+
+(define (package-without-replacement pkg)
+  (package (inherit pkg) (replacement #f)))
+
+(define %test-emacs-native-comp-replacable
+  (system-test
+   (name "emacs-native-comp")
+   (description "Test whether an emacs replacement (if any) is valid.")
+   (value (run-native-comp-replacable-test
+           (package-without-replacement emacs)
+           emacs))))

base-commit: e3dfed59d39ac60dd2e2b9ef9f4ef63a2a081f41
-- 
2.45.2

bug#72045: [PATCH v2 1/2] gnu: Add system test for Emacs.

[Suhail Singh]

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> +          (test-equal "native-comp-dir"
> +            (emacs-native-comp-dir
> +             #$(file-append old-emacs "/bin/emacs"))
> +            (emacs-native-comp-dir
> +             #$(file-append new-emacs "/bin/emacs")))

I like that there is a test that focuses on the native-comp-dir
directly.  Having only a test that focuses on ABI_VERSION wouldn't have
been sufficient IMO.

Minor nitpick: However, there may still be some utility in either having
an additional test for ABI_VERSION or adding a comment that a successful
evaluation of the above test also implies that the ABI_VERSION matches.

> +          (test-assert "old emacs has hierarchical layout"
> +            (file-exists?
> +             (string-append #$new-emacs "/lib/emacs/"
> +                            (emacs-effective-version old-emacs-bin)
> +                            "/native-lisp/"
> +                            (emacs-native-comp-dir old-emacs-bin)
> +                            "/preloaded/emacs-lisp/comp.eln")))

Should that say #$old-emacs instead of #$new-emacs ?

> +          (test-assert "new emacs has hierarchical layout"
> +            (file-exists?
> +             (string-append #$new-emacs "/lib/emacs/"
> +                            (emacs-effective-version new-emacs-bin)
> +                            "/native-lisp/"
> +                            (emacs-native-comp-dir new-emacs-bin)
> +                            "/preloaded/emacs-lisp/comp.eln")))

Do we need to additionally ensure that the new emacs' "hierarchical
layout" matches the old emacs' "hierarchical layout" in some way (over
and above both having them)?

> +(define %test-emacs-native-comp-replacable
> +  (system-test
> +   (name "emacs-native-comp")
> +   (description "Test whether an emacs replacement (if any) is valid.")
> +   (value (run-native-comp-replacable-test
> +           (package-without-replacement emacs)
> +           emacs))))

Ah!  So that's how it's done.  I am not qualified to review this part,
but this looks to be in the right spirit.  Hoping this is merged soon.™

-- 
Suhail

bug#72045: [PATCH v2 1/2] gnu: Add system test for Emacs.

[Liliana Marie Prikler]

Am Freitag, dem 19.07.2024 um 11:23 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > +          (test-equal "native-comp-dir"
> > +            (emacs-native-comp-dir
> > +             #$(file-append old-emacs "/bin/emacs"))
> > +            (emacs-native-comp-dir
> > +             #$(file-append new-emacs "/bin/emacs")))
> 
> I like that there is a test that focuses on the native-comp-dir
> directly.  Having only a test that focuses on ABI_VERSION wouldn't
> have been sufficient IMO.
> 
> Minor nitpick: However, there may still be some utility in either
> having an additional test for ABI_VERSION or adding a comment that a
> successful evaluation of the above test also implies that the
> ABI_VERSION matches.
We can test `comp-abi-hash` as well, should be no biggie.

> > +          (test-assert "old emacs has hierarchical layout"
> > +            (file-exists?
> > +             (string-append #$new-emacs "/lib/emacs/"
> > +                            (emacs-effective-version old-emacs-
> > bin)
> > +                            "/native-lisp/"
> > +                            (emacs-native-comp-dir old-emacs-bin)
> > +                            "/preloaded/emacs-lisp/comp.eln")))
> 
> Should that say #$old-emacs instead of #$new-emacs ?
Yes, it should.

> > +          (test-assert "new emacs has hierarchical layout"
> > +            (file-exists?
> > +             (string-append #$new-emacs "/lib/emacs/"
> > +                            (emacs-effective-version new-emacs-
> > bin)
> > +                            "/native-lisp/"
> > +                            (emacs-native-comp-dir new-emacs-bin)
> > +                            "/preloaded/emacs-lisp/comp.eln")))
> 
> Do we need to additionally ensure that the new emacs' "hierarchical
> layout" matches the old emacs' "hierarchical layout" in some way
> (over and above both having them)?
We could do so, but that'd be an expensive test.  In practice, we
assume the same layout and just poke a single file.

> > +(define %test-emacs-native-comp-replacable
> > +  (system-test
> > +   (name "emacs-native-comp")
> > +   (description "Test whether an emacs replacement (if any) is
> > valid.")
> > +   (value (run-native-comp-replacable-test
> > +           (package-without-replacement emacs)
> > +           emacs))))
> 
> Ah!  So that's how it's done.  I am not qualified to review this
> part, but this looks to be in the right spirit.  Hoping this is
> merged soon.™
You can verify this part by running "make check-system TESTS=emacs-
native-comp" and seeing that it fails for this commit but succeeds on
the next.  That's TDD :D

Cheers