GNU bug report logs - #72045
Emacs graft lookup still fails

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 72045 in the body.
You can then email your comments to 72045 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: bug-guix <at> gnu.org
Subject: Emacs graft lookup still fails
Date: Wed, 10 Jul 2024 22:06:13 +0200

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

Message #8 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: 72045 <at> debbugs.gnu.org
Subject: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and native version
 dir without version.
Date: Sat, 13 Jul 2024 07:49:06 +0200

* 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

Message #11 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: 72045 <at> debbugs.gnu.org
Subject: [PATCH 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 13 Jul 2024 07:53:35 +0200

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

Message #14 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and
 native version dir without version.
Date: Sat, 13 Jul 2024 12:08:02 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #17 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: bug#72045: [PATCH 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 13 Jul 2024 12:14:09 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #20 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: bug#72045: [PATCH 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 13 Jul 2024 18:59:50 +0200

Am Samstag, dem 13.07.2024 um 12:14 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> 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

Message #23 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and
 native version dir without version.
Date: Sat, 13 Jul 2024 19:26:10 +0200

Am Samstag, dem 13.07.2024 um 12:08 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> 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

Message #26 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org,
 Suhail Singh <suhailsingh247 <at> gmail.com>, andrew <at> trop.in
Subject: Re: bug#72045: [PATCH RFC 1/2] gnu: emacs: Compute ABI hash and
 native version dir without version.
Date: Sat, 13 Jul 2024 13:59:07 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #29 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: "Suhail Singh" <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sat, 13 Jul 2024 19:22:46 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #32 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sun, 14 Jul 2024 10:50:41 +0200

Am Samstag, dem 13.07.2024 um 19:22 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> 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

Message #35 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org, Suhail Singh <suhailsingh247 <at> gmail.com>
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sun, 14 Jul 2024 12:27:10 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #38 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sun, 14 Jul 2024 18:46:52 +0200

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
> > 

Message #41 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org, Suhail Singh <suhailsingh247 <at> gmail.com>
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sun, 14 Jul 2024 12:56:01 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #44 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org, Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Subject: Re: bug#72045: Emacs graft lookup still fails
Date: Sun, 14 Jul 2024 12:59:05 -0400

Suhail Singh <suhailsingh247 <at> gmail.com> writes:

> Liliana Marie Prikler <liliana.prikler <at> 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

Message #47 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org, Suhail Singh <suhailsingh247 <at> gmail.com>
Subject: Re: [PATCH v2 1/2] gnu: Add system test for Emacs.
Date: Fri, 19 Jul 2024 11:23:57 -0400

Liliana Marie Prikler <liliana.prikler <at> 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

Message #50 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: 72045 <at> debbugs.gnu.org
Subject: Re: [PATCH v2 1/2] gnu: Add system test for Emacs.
Date: Fri, 19 Jul 2024 17:42:52 +0200

Am Freitag, dem 19.07.2024 um 11:23 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> 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

Message #53 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: 72045 <at> debbugs.gnu.org
Cc: Suhail Singh <suhailsingh247 <at> gmail.com>
Subject: [PATCH v2 1/2] gnu: Add system test for Emacs.
Date: Fri, 19 Jul 2024 09:35:34 +0200

* 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 <at> 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

Message #56 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org,
 Suhail Singh <suhailsingh247 <at> gmail.com>, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 10:31:59 -0400

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

> +    ;; Run `make check-system TESTS=emacs-native-comp' to ensure that grafts
> +    ;; can meaningfully be applied.

Either this isn't the correct invocation or I am doing something wrong.
Running that command in guix checkout seems to run many tests (possibly
all of them?).  After applying this patch I get the following test suite
summary:

#+begin_quote
  ...
  ============================================================================
  Testsuite summary for GNU Guix 1.4.0-23.843b85c
  ============================================================================
  # TOTAL: 2451
  # PASS:  2341
  # SKIP:  86
  # XFAIL: 2
  # FAIL:  22
  # XPASS: 0
  # ERROR: 0
  ============================================================================
  See ./test-suite.log
  Please report to bug-guix <at> gnu.org
  ============================================================================
  ...
#+end_quote

-- 
Suhail

Message #59 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 17:02:21 +0200

Am Samstag, dem 20.07.2024 um 10:31 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> gmail.com> writes:
> 
> > +    ;; Run `make check-system TESTS=emacs-native-comp' to ensure
> > that grafts
> > +    ;; can meaningfully be applied.
> 
> Either this isn't the correct invocation or I am doing something
> wrong.  Running that command in guix checkout seems to run many tests
> (possibly all of them?).  After applying this patch I get the
> following test suite summary:
> 
> #+begin_quote
>   ...
>  
> =====================================================================
>   Testsuite summary for GNU Guix 1.4.0-23.843b85c
> =====================================================================
>   # TOTAL: 2451
>   # PASS:  2341
>   # SKIP:  86
>   # XFAIL: 2
>   # FAIL:  22
>   # XPASS: 0
>   # ERROR: 0
>  
> =====================================================================
>   See ./test-suite.log
>   Please report to bug-guix <at> gnu.org
> =====================================================================
>   ...
> #+end_quote
Well, I get

#+begin_quote
Selected 1 system tests...
/gnu/store/rszxrbzryn0cwmdq5mppr7yqqgij4gzd-emacs-native-comp-
compatible
#+end_quote

That's quite strange.

Message #62 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org,
 Suhail Singh <suhailsingh247 <at> gmail.com>, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 11:25:04 -0400

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

> Selected 1 system tests...

I get this line as well early on.  However, after that lots more things
happen.

Could you please confirm the guix checkout commit you're applying your
patch on?

-- 
Suhail

Message #65 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 17:42:23 +0200

Am Samstag, dem 20.07.2024 um 11:25 -0400 schrieb Suhail Singh:
> Liliana Marie Prikler <liliana.prikler <at> gmail.com> writes:
> 
> > Selected 1 system tests...
> 
> I get this line as well early on.  However, after that lots more
> things happen.

> 
> Could you please confirm the guix checkout commit you're applying
> your patch on?
Commit 9724e61cda80e4c59a2eb419a453887ecc551b9a, plus some very
unrelated stuff I have lying locally on master.  Particularly, commits
to sign off.

Cheers

Message #68 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org,
 Suhail Singh <suhailsingh247 <at> gmail.com>, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 12:34:02 -0400

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

> Commit 9724e61cda80e4c59a2eb419a453887ecc551b9a

Okay, I can confirm that it's working when applied on that commit.

I am not sure whether the issue I was observing on
24163eea584663568b68e19f364256fc7396b61f were due to incorrect
configuration on my part or something else (jury is out on that one).
However, for the purposes of #72045 I believe v3 looks good.

-- 
Suhail

Message #71 received at 72045 <at> debbugs.gnu.org (full text, mbox):

From: Suhail Singh <suhailsingh247 <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045 <at> debbugs.gnu.org,
 Liliana Marie Prikler <liliana.prikler <at> gmail.com>, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sat, 20 Jul 2024 12:47:41 -0400

Suhail Singh <suhailsingh247 <at> gmail.com> writes:

> I am not sure whether the issue I was observing on
> 24163eea584663568b68e19f364256fc7396b61f

Ah.  I was simply unlucky.  The failing tests were related to the build
of guile 3.0.10.  This commit has since been reverted in master.
Apologies for the noise.

-- 
Suhail

Message #76 received at 72045-done <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Suhail Singh <suhailsingh247 <at> gmail.com>
Cc: cox.katherine.e+guix <at> gmail.com, 72045-done <at> debbugs.gnu.org, andrew <at> trop.in
Subject: Re: [PATCH v3 2/2] gnu: emacs-minimal: Ungraft.
Date: Sun, 21 Jul 2024 12:05:15 +0200

Am Samstag, dem 20.07.2024 um 12:34 -0400 schrieb Suhail Singh:
> However, for the purposes of #72045 I believe v3 looks good.
For the purposes of #72045, I'm marking it as done.

Cheers

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.