diff mbox series

[bug#63987] gnu: Add emacs-jinx.

Message ID 88d5ddf1b26b5f4c23601780aa29158e46a6cbfe.1686354110.git.mekeor@posteo.de
State New
Headers show
Series [bug#63987] gnu: Add emacs-jinx. | expand

Commit Message

Mekeor Melire June 9, 2023, 11 p.m. UTC
* gnu/packages/emacs-xyz.scm (emacs-jinx): New variable.
---
Hello Guix! I have some questions about this patch that I'm submitting:

- emacs-jinx needs gcc at build time. Should I use gcc-toolchain (or
  rather the internal gcc) package?

- When using the module (gnu packages commencement), I first got the
  following warning. That's why I decided to use it with a prefix
  instead. What do you think about that?

  WARNING: (gnu packages emacs-xyz): `canonical-package' imported from
  both (gnu packages base) and (gnu packages commencement)

- I did not introduce any (revision "0") variable or so. I hope that's
  okay. I'm not sure when to use it.

- I placed the package declaration right after other spelling related
  Emacs packages. Unfortunately, the declaration of emacs-jit-spell is
  not close. I could send another additional patch that moves all
  spelling-related Emacs packages into an own section, if you'd like.
  Like this:

  ;;;
  ;;; Spelling
  ;;;

Anyways. Cheers!

 gnu/packages/emacs-xyz.scm | 73 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)


base-commit: 297ba5c15a32845ab8514aeb6f405ebd4290142d

Comments

Mekeor Melire June 18, 2023, 11:34 a.m. UTC | #1
Hello Guix,

my previous patch added a package definition for emacs-jinx. The package definition works by installing the accompanying jinx-mod.so into the package's $out/lib directory, and patching jinx.el so that it loads that library from the correct path.

The maintainer of the Emacs package suggests to rather install jinx-mod.so into $out/share/emacs/site-lisp/jinx-.../ instead, i.e. right next to the .el-files: https://github.com/minad/jinx/issues/86#issuecomment-1596102645

What do you think?
Liliana Marie Prikler June 18, 2023, 11:56 a.m. UTC | #2
Hi Mekeor,

Am Sonntag, dem 18.06.2023 um 11:34 +0000 schrieb Mekeor Melire:
> Hello Guix,
> 
> my previous patch added a package definition for emacs-jinx. The
> package definition works by installing the accompanying jinx-mod.so
> into the package's $out/lib directory, and patching jinx.el so that
> it loads that library from the correct path.
> 
> The maintainer of the Emacs package suggests to rather install jinx-
> mod.so into $out/share/emacs/site-lisp/jinx-.../ instead, i.e. right
> next to the .el-files:
> https://github.com/minad/jinx/issues/86#issuecomment-1596102645
> 
> What do you think?
The way you did it SGTM.  If you fear that it might shadow other shared
libraries (probably an unnecessary fear, but who knows?), you could use
lib/emacs instead.  Other comments will come in a separate reply to
your first mail.  

Stay tuned!
Liliana Marie Prikler June 18, 2023, 12:14 p.m. UTC | #3
Am Freitag, dem 09.06.2023 um 23:00 +0000 schrieb mekeor@posteo.de:
> * gnu/packages/emacs-xyz.scm (emacs-jinx): New variable.
> ---
> Hello Guix! I have some questions about this patch that I'm
> submitting:
> 
> - emacs-jinx needs gcc at build time. Should I use gcc-toolchain (or
>   rather the internal gcc) package?
Regular GCC ought to suffice.  I'm not sure why it isn't an implicit
input of emacs-build-system, but you get what you code for.

> - When using the module (gnu packages commencement), I first got the
>   following warning. That's why I decided to use it with a prefix
>   instead. What do you think about that?
> 
>   WARNING: (gnu packages emacs-xyz): `canonical-package' imported
> from
>   both (gnu packages base) and (gnu packages commencement)
> 
> - I did not introduce any (revision "0") variable or so. I hope
> that's
>   okay. I'm not sure when to use it.
I personally only find that okay when using tagged releases, but
there's different opinions on that and slightly varying styles
throughout the code base.  I'd prefer it if you used the 
  (commit (f version)) 
pattern, where f is some transformation – including (commit version).

> - I placed the package declaration right after other spelling related
>   Emacs packages. Unfortunately, the declaration of emacs-jit-spell
> is
>   not close. I could send another additional patch that moves all
>   spelling-related Emacs packages into an own section, if you'd like.
>   Like this:
Sorting-wise, why not place it right before emacs-jit-spell?
jinx < jit.

>   ;;;
>   ;;; Spelling
>   ;;;
> 
> Anyways. Cheers!
> 
>  gnu/packages/emacs-xyz.scm | 73
> ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
> index 0ea9732bfa..7e4f1a1fea 100644
> --- a/gnu/packages/emacs-xyz.scm
> +++ b/gnu/packages/emacs-xyz.scm
> @@ -148,6 +148,7 @@
>  
>  (define-module (gnu packages emacs-xyz)
>    #:use-module ((guix licenses) #:prefix license:)
> +  #:use-module ((gnu packages commencement) #:prefix commencement:)
>    #:use-module (guix packages)
>    #:use-module (guix cvs-download)
>    #:use-module (guix download)
> @@ -265,6 +266,7 @@ (define-module (gnu packages emacs-xyz)
>    #:use-module (gnu packages virtualization)
>    #:use-module (gnu packages web-browsers)
>    #:use-module (gnu packages wget)
> +  #:use-module (gnu packages enchant)
>    #:use-module (guix utils)
>    #:use-module (srfi srfi-1)
>    #:use-module (ice-9 match))
> @@ -31922,6 +31924,77 @@ (define-public emacs-spell-fu
>  that runs from the syntax highlighter without starting external
> processes.")
>        (license license:gpl3+))))
>  
> +(define-public emacs-jinx
> +  (package
> +    (name "emacs-jinx")
> +    (version "0.8")
> +    (source
> +      (origin
> +        (method git-fetch)
> +        (uri
> +          (git-reference
> +            (url "https://github.com/minad/jinx")
> +            (commit "7fced90fdaca5a482cd08b80967e0eac5ee8d885")))
> +        (file-name (git-file-name name version))
> +        (sha256
> +          (base32
> +           
> "1y097rnf9zg26jf4vh74a0laddfp4x6pp1fjqs3xqgwc0cmdq59w"))))
> +    (build-system emacs-build-system)
> +    (propagated-inputs (list emacs-compat))
> +    (native-inputs
> +      (list
> +        emacs-compat
> +        enchant
> +        commencement:gcc-toolchain
> +        pkg-config
> +        texinfo))
> +    (inputs (list enchant))
> +    (arguments
> +      (list
> +        #:phases
> +        #~(modify-phases %standard-phases
> +            ;; Compile the accompanying jinx-mod.c file with Emacs
> and
> +            ;; jinx.el. This needs to happen after expand-load-path
> phase so
> +            ;; that jinx.el is able to load emacs-compat.
> +            (add-after 'expand-load-path 'compile-jinx-mod-c
> +              (lambda _
> +                (invoke
> +                  "emacs" "--batch" "-L" "."
> +                  "-l" "jinx.el"
> +                  "-f" "jinx--load-module")
> +                (install-file "jinx-mod.so"
> +                  (string-append #$output "/lib"))))
The install-file ought to go in it's own phase, imho.
> +            ;; Patch Jinx.el to load the previously compiled jinx-
> mod.so from
> +            ;; correct output path instead of attempting to compile
> it.
> +            (add-after 'compile-jinx-mod-c 'use-compiled-jinx-mod-so
> +              (lambda _
> +                (let ((file "jinx.el"))
> +                  (make-file-writable file)
> +                  (emacs-substitute-sexps file
> +                    ("\"Compile and load dynamic module.\""
> +                      `(module-load
> +                         ,(string-append #$output "/lib/jinx-
> mod.so")))))))
You might want to use a patch instead if a simple substitute-sexps.  
> +            (add-after 'install 'makeinfo
> +              (lambda _
> +                (invoke "emacs" "--batch"
> +                  "--eval=(require 'ox-texinfo)"
> +                  "--eval=(find-file \"README.org\")"
> +                  "--eval=(org-texinfo-export-to-info)")
> +                (install-file "jinx.info"
> +                  (string-append #$output "/share/info")))))))
Again, split build and install.
> +    (home-page "https://github.com/minad/jinx")
> +    (synopsis "Emacs Enchanted Spell Checker")
> +    (description
> +      "Jinx is a fast just-in-time spell-checker for Emacs.  Jinx
> highlights
> +misspelled words in the text of the visible portion of the buffer. 
> For
> +efficiency, Jinx highlights misspellings lazily, recognizes window
> boundaries
> +and text folding, if any.
"Jinx highlights misspellings lazily and honours window boundaries as
well as potential text foldings."

>   For example, when unfolding or scrolling, only the
> +newly visible part of the text is checked if it has not been checked
> before.
This is somewhat gratuitous information, leave it out :)

> +Each misspelling can be corrected from a list of dictionary words
> presented as
> +a completion menu.  
That's something you'd expect, leave it.

> Jinx's high performance and low resource usage comes from
> +directly calling the widely-used API of the Enchant library.")
Maybe mention this in the first sentence instead of making it the last:
"Jinx is a just-in-time spell-checker for Emacs based on the Enchant
library."


Cheers
Mekeor Melire June 20, 2023, 10:04 p.m. UTC | #4
2023-06-18 13:56 liliana.prikler@gmail.com:

> Hi Mekeor,
>
> Am Sonntag, dem 18.06.2023 um 11:34 +0000 schrieb Mekeor Melire:
> > Hello Guix,
> >
> > my previous patch added a package definition for emacs-jinx. 
> > The
> > package definition works by installing the accompanying 
> > jinx-mod.so
> > into the package's $out/lib directory, and patching jinx.el so 
> > that
> > it loads that library from the correct path.
> >
> > The maintainer of the Emacs package suggests to rather install 
> > jinx-
> > mod.so into $out/share/emacs/site-lisp/jinx-.../ instead, i.e. 
> > right
> > next to the .el-files:
> > https://github.com/minad/jinx/issues/86#issuecomment-1596102645
> >
> > What do you think?
> The way you did it SGTM.  If you fear that it might shadow other 
> shared
> libraries (probably an unnecessary fear, but who knows?), you 
> could use
> lib/emacs instead.  Other comments will come in a separate reply 
> to
> your first mail.
>
> Stay tuned!

Daniel Mendler says: "My suggestion is that you put the library 
file [jinx-mod.so] in some appropriate directory, which must be on 
the load-path. I assume
that there exists some convention on Guix on where to install 
native modules." 
https://github.com/minad/jinx/issues/86#issuecomment-1597221252

And I think they are right. We should have a convention about where to install native modules (.so-files) and add that directory to Emacs' load-path.

One approach would be to use $out/lib/emacs (or a new subdirectory of it). E.g. we would install jinx-mod.so into that path and Guix would add it to the EMACSLOADPATH environment-variable. (Currently, Guix only makes use of .../lib/emacs/native-site-lisp which is added to EMACSNATIVELOADPATH environment-variable.)

Another approach would be to agree on installing *.so-files into $out/share/site-lisp/PACKAGE-VERSION/ so that it is already present Emacs' load-path as is.

What do you think?
Liliana Marie Prikler June 21, 2023, 4:23 a.m. UTC | #5
Am Dienstag, dem 20.06.2023 um 22:04 +0000 schrieb Mekeor Melire:
> Daniel Mendler says: "My suggestion is that you put the library 
> file [jinx-mod.so] in some appropriate directory, which must be on 
> the load-path. I assume that there exists some convention on Guix on
> where to install native modules." 
> https://github.com/minad/jinx/issues/86#issuecomment-1597221252
> 
> And I think they are right. We should have a convention about where
> to install native modules (.so-files) and add that directory to
> Emacs' load-path.
> 
> One approach would be to use $out/lib/emacs (or a new subdirectory of
> it). E.g. we would install jinx-mod.so into that path and Guix would
> add it to the EMACSLOADPATH environment-variable. (Currently, Guix
> only makes use of .../lib/emacs/native-site-lisp which is added to
> EMACSNATIVELOADPATH environment-variable.)
What's the functional difference between EMACSLOADPATH and
EMACSNATIVELOADPATH here?  Why would the latter not work?

> Another approach would be to agree on installing *.so-files into
> $out/share/site-lisp/PACKAGE-VERSION/ so that it is already present
> Emacs' load-path as is.
> 
> What do you think?
/lib/emacs/native-site-lisp sounds like the wrong place regardless, as
does /share/site-lisp.  Perhaps /lib/emacs/site-mod?

Cheers
diff mbox series

Patch

diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index 0ea9732bfa..7e4f1a1fea 100644
--- a/gnu/packages/emacs-xyz.scm
+++ b/gnu/packages/emacs-xyz.scm
@@ -148,6 +148,7 @@ 
 
 (define-module (gnu packages emacs-xyz)
   #:use-module ((guix licenses) #:prefix license:)
+  #:use-module ((gnu packages commencement) #:prefix commencement:)
   #:use-module (guix packages)
   #:use-module (guix cvs-download)
   #:use-module (guix download)
@@ -265,6 +266,7 @@  (define-module (gnu packages emacs-xyz)
   #:use-module (gnu packages virtualization)
   #:use-module (gnu packages web-browsers)
   #:use-module (gnu packages wget)
+  #:use-module (gnu packages enchant)
   #:use-module (guix utils)
   #:use-module (srfi srfi-1)
   #:use-module (ice-9 match))
@@ -31922,6 +31924,77 @@  (define-public emacs-spell-fu
 that runs from the syntax highlighter without starting external processes.")
       (license license:gpl3+))))
 
+(define-public emacs-jinx
+  (package
+    (name "emacs-jinx")
+    (version "0.8")
+    (source
+      (origin
+        (method git-fetch)
+        (uri
+          (git-reference
+            (url "https://github.com/minad/jinx")
+            (commit "7fced90fdaca5a482cd08b80967e0eac5ee8d885")))
+        (file-name (git-file-name name version))
+        (sha256
+          (base32
+            "1y097rnf9zg26jf4vh74a0laddfp4x6pp1fjqs3xqgwc0cmdq59w"))))
+    (build-system emacs-build-system)
+    (propagated-inputs (list emacs-compat))
+    (native-inputs
+      (list
+        emacs-compat
+        enchant
+        commencement:gcc-toolchain
+        pkg-config
+        texinfo))
+    (inputs (list enchant))
+    (arguments
+      (list
+        #:phases
+        #~(modify-phases %standard-phases
+            ;; Compile the accompanying jinx-mod.c file with Emacs and
+            ;; jinx.el. This needs to happen after expand-load-path phase so
+            ;; that jinx.el is able to load emacs-compat.
+            (add-after 'expand-load-path 'compile-jinx-mod-c
+              (lambda _
+                (invoke
+                  "emacs" "--batch" "-L" "."
+                  "-l" "jinx.el"
+                  "-f" "jinx--load-module")
+                (install-file "jinx-mod.so"
+                  (string-append #$output "/lib"))))
+            ;; Patch Jinx.el to load the previously compiled jinx-mod.so from
+            ;; correct output path instead of attempting to compile it.
+            (add-after 'compile-jinx-mod-c 'use-compiled-jinx-mod-so
+              (lambda _
+                (let ((file "jinx.el"))
+                  (make-file-writable file)
+                  (emacs-substitute-sexps file
+                    ("\"Compile and load dynamic module.\""
+                      `(module-load
+                         ,(string-append #$output "/lib/jinx-mod.so")))))))
+            (add-after 'install 'makeinfo
+              (lambda _
+                (invoke "emacs" "--batch"
+                  "--eval=(require 'ox-texinfo)"
+                  "--eval=(find-file \"README.org\")"
+                  "--eval=(org-texinfo-export-to-info)")
+                (install-file "jinx.info"
+                  (string-append #$output "/share/info")))))))
+    (home-page "https://github.com/minad/jinx")
+    (synopsis "Emacs Enchanted Spell Checker")
+    (description
+      "Jinx is a fast just-in-time spell-checker for Emacs.  Jinx highlights
+misspelled words in the text of the visible portion of the buffer.  For
+efficiency, Jinx highlights misspellings lazily, recognizes window boundaries
+and text folding, if any.  For example, when unfolding or scrolling, only the
+newly visible part of the text is checked if it has not been checked before.
+Each misspelling can be corrected from a list of dictionary words presented as
+a completion menu.  Jinx's high performance and low resource usage comes from
+directly calling the widely-used API of the Enchant library.")
+    (license license:gpl3+)))
+
 (define-public emacs-org-emms
   (let ((commit "07a8917f3d628c32e5de1dbd118ac08203772533")
         (revision "1"))