bug#25728: [PATCH 2/2] gnu: Add colorforth.

[Ricardo Wurmus]

address@hidden writes:

> From: ng0 <address@hidden>
>
> * gnu/packages/forth.scm (colorforth): New variable.
> ---

The patch to change the module name is fine (although I’d move the
copyright update to this patch).

>  gnu/packages/forth.scm | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/gnu/packages/forth.scm b/gnu/packages/forth.scm
> index 21a0fc2de..8854a9246 100644
> --- a/gnu/packages/forth.scm
> +++ b/gnu/packages/forth.scm
> @@ -21,7 +21,9 @@
>    #:use-module ((guix licenses) #:prefix license:)
>    #:use-module (guix packages)
>    #:use-module (guix download)
> +  #:use-module (guix git-download)
>    #:use-module (guix build-system gnu)
> +  #:use-module (gnu packages assembly)
>    #:use-module (gnu packages m4))
>  
>  (define-public gforth
> @@ -58,3 +60,39 @@ and history.  A generic virtual machine environment, 
> vmgen, is also
>  included.")
>      (home-page "https://www.gnu.org/software/gforth/";)
>      (license license:gpl3+)))
> +
> +(define-public colorforth
> +  (let ((commit "94aec438f1ded202681f18801b98c52dc3beee41")
> +        (revision "1"))
> +    (package
> +      (name "colorforth")
> +      (version (string-append "0.0.0-" revision "." (string-take commit 7)))
> +      (source (origin
> +                (method git-fetch)
> +                (uri (git-reference
> +                      (url "https://github.com/narke/colorForth";)
> +                      (commit commit)))
> +                (sha256
> +                 (base32
> +                  "0s602k568bm6vmvpahsms77liicg38vksn59j5m8ax4h9l9ca77r"))))
> +      (arguments
> +       `(#:tests? #f
> +         #:phases
> +         (modify-phases %standard-phases
> +           (delete 'configure) ; no configure script
> +           (replace 'install ; There is no 'install

Please change the comment to “no install target” or similar.  “no
'install” is confusing because “'install” is a quoted symbol and that
has no meaning outside of Scheme.

> +             (lambda _
> +               (install-file "cf2012.img"
> +                             (string-append (assoc-ref %outputs "out")
> +                                            "/bin")))))))

Please use “outputs” instead of “%outputs”.  Is the target “bin”
directory created during the build?

Please also make the phase end with “#t”.

> +      (native-inputs
> +       `(("nasm" ,nasm)))
> +      (build-system gnu-build-system)
> +      (home-page "https://github.com/narke/colorForth";)
> +      (synopsis "Native 32-bit colorForth for PCs, Bochs and Qemu")
> +      (description
> +       "Native colorForth for 32-bit PCs, at least compilable on Linux
> + and runnable on both Bochs and Qemu.  It is adapted from
> + @url{http://sourceforge.net/projects/colorforth, colorforth}.
> + The original colorforth is public domain software.")

Please change the description.  The first sentence fragment should be a
full sentence.  I don’t think “32-bit PCs” should be mentioned, nor
should compatibility with Linux be mentioned (do they mean the kernel or
GNU?).  Also the last sentence should not be included.

Could you write a description that describes the package, i.e. tells
potential users why they would want to use it?

Looks like it’s written in x86 assembly.  This would be worth mentioning
(and I think that’s what “32-bit PCs” implied).

Could you please send an updated patch?

-- 
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net