Re: [PATCH] gnu: Add kakoune

[Leo Famulari <leo@famulari.name>]

[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]

On Fri, Dec 30, 2016 at 03:44:13PM +1100, Carlo Zancanaro wrote:
> From 57b2f9e6af20606e302fc4040515d7a92bde73fc Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Fri, 30 Dec 2016 02:15:59 +1100
> Subject: [PATCH] gnu: Add kakoune
> To: guix-devel@gnu.org
> 
> * gnu/packages/patches/kakoune-fix-bash-path.patch: New file.
> * gnu/packages/patches/kakoune-leave-manpages-uncompressed.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Register patches.
> * gnu/packages/text-editors.scm (kakoune): New variable.

Thank you!

> diff --git a/gnu/packages/patches/kakoune-fix-bash-path.patch b/gnu/packages/patches/kakoune-fix-bash-path.patch
> diff --git a/gnu/packages/patches/kakoune-leave-manpages-uncompressed.patch b/gnu/packages/patches/kakoune-leave-manpages-uncompressed.patch

Patches require some commentary explaining what problem they are solving
and, if the approach they take is unclear, how they do it. If they are
copied from somewhere else, they need attribution.

In this case, we need to explain why upstream's approach of finding a
POSIX shell doesn't work.

> diff --git a/gnu/packages/text-editors.scm b/gnu/packages/text-editors.scm

> +(define-public kakoune
> +  (let ((commit "4f986776423f5cd5fd80a16735f873ae23c10da2"))

Can you say why you chose this commit? Is it simply the most recent
commit when you made the package?

> +    (package
> +      (name "kakoune")
> +      (version (string-append "0.0.1-" (string-take commit 7)))

Please add a revision counter, as described in the manual, section 7.6.3
Version Numbers. The resulting version will look like this:


> +      (version (string-append "0.0.0-" revision "." (string-take commit 7)))

> +         (file-name (string-append "kakoune-" version))

Please append '-checkout' to the end of the file-name.

> +           (add-before 'configure 'patch-bash-path
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               (substitute* "src/shell_manager.cc"
> +                 (("__BASH_PATH__")
> +                  (assoc-ref inputs "bash")))))

I wonder if we can do this change all in the same place. It's sort of
complicated to use a patch and a substitution.

Also, using a patch to get half-way there will cause the source code
given by `guix build --source` to be unbuildable, right? So, I think we
should find another way to fix whatever is wrong with the shell-finding
logic. Perhaps it will require cooperation with the upstream
maintainers, I don't know.

> +           ;; Kakoune doesn't have a configure script, but it still
> +           ;; requires us to do some setup/configuration.
> +           (replace 'configure
> +             (lambda* (#:key outputs #:allow-other-keys)
> +               ;; Make has to be run in the src/ directory, so let's
> +               ;; move there now.
> +               (chdir "src")
> +               ;; Boost is compiled with the older ABI, so we can't use
> +               ;; the new ABI if we want to link againt it.
> +               (setenv "CPPFLAGS" "-D_GLIBCXX_USE_CXX11_ABI=0")
> +               (setenv "PREFIX" (assoc-ref outputs "out"))))

I think that CPPFLAGS and PREFIX should be set in #:make-flags. This
phase would then be renamed to 'chdir' and we'd delete the 'configure'
phase.

> +           (add-before 'check 'fix-test-permissions
> +             (lambda* _
> +               ;; The tests fail if they can't write over files in the
> +               ;; test directory, so make sure we can write everywhere
> +               ;; we might possibly need to.
> +               (system* "chmod" "-R" "u+w" "../test"))))))

This seems like a bug. Can you report it upstream?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]