[PATCH] gnu: Add Mlucas.

[PATCH] gnu: Add Mlucas.

[Alex Vong]

From e5155b52f636bfee849268b19b81f5b6608540fd Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1995@gmail.com>
Date: Mon, 5 Oct 2015 12:49:49 +0800
Subject: [PATCH] gnu: Add Mlucas.

* gnu/packages/mlucas.scm: New file.
* gnu-system.am (GNU_SYSTEM_MODULES): Register it.
---
 gnu-system.am           |   1 +
 gnu/packages/mlucas.scm | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 284 insertions(+)
 create mode 100644 gnu/packages/mlucas.scm

diff --git a/gnu-system.am b/gnu-system.am
index 577c6e8..2a5ec03 100644
--- a/gnu-system.am
+++ b/gnu-system.am
@@ -215,6 +215,7 @@ GNU_SYSTEM_MODULES =				\
   gnu/packages/mg.scm				\
   gnu/packages/miscfiles.scm			\
   gnu/packages/mit-krb5.scm			\
+  gnu/packages/mlucas.scm			\
   gnu/packages/moe.scm				\
   gnu/packages/moreutils.scm			\
   gnu/packages/mpd.scm				\
diff --git a/gnu/packages/mlucas.scm b/gnu/packages/mlucas.scm
new file mode 100644
index 0000000..ff641f2
--- /dev/null
+++ b/gnu/packages/mlucas.scm
@@ -0,0 +1,283 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2015 Alex Vong <alexvong1995@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 packages mlucas)
+  #:use-module (srfi srfi-1)
+  #:use-module (guix packages)
+  #:use-module (guix download)
+  #:use-module (guix build-system gnu)
+  #:use-module (guix licenses)
+  #:use-module (gnu packages autogen)
+  #:use-module (gnu packages autotools)
+  #:use-module (gnu packages perl))
+
+
+;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
+;;;
+;;; The data strcture flag-list is constrcuted by (flag-list <flag-sublist>...)
+;;; The constructor flag-list does something to the argument,
+;;; such as trimming whitespaces, to ensure no two arguments mean the same.
+;;;
+;;; The data structure flag-sublist is in fact an ordinary list
+;;; with the following structure (<flag-type-symbol> <flag-string>...)
+;;;
+;;; Here is an example:
+;;; (flag-list
+;;;  '(CFLAGS "-O2" "-g")
+;;;  '(LDFLAGS "-lm" "-lpthread"))
+;;;
+;;; flag-list+ and flag-list- are analogous to
+;;; numberic + and - but operate on flag-list.
+;;;
+;;; flag-list->string-list converts flag-list into
+;;; configure-flags-compatible string-list.
+;;;
+
+;;; selectors of flag-sublist
+(define (flag-type flag-sublist)
+  (car flag-sublist))
+(define (flag-string-list flag-sublist)
+  (cdr flag-sublist))
+
+;;; constructor of flag-list
+(define (flag-list . flag-lst)
+  ;; Trim leading and trailing whitespaces of all flag-string
+  ;; in flag-list.
+  (define (trim-flag-string flag-lst)
+    (map (λ(flag-sublist)
+           (cons (flag-type flag-sublist)
+                 (map string-trim-both
+                      (flag-string-list flag-sublist))))
+         flag-lst))
+  ;; Sort flag-list using flag-type of flag-sublist,
+  ;; this will make it easier to add two flag-list together.
+  (define (sort-flag-list flag-lst)
+    (sort-list flag-lst
+               (λ(a b)
+                 (string<? (symbol->string (flag-type a))
+                           (symbol->string (flag-type b))))))
+  ;; Given a sorted flag-list,
+  ;; combine flag-sublist which have the same flag-type.
+  (define (merge-sorted-flag-list flag-lst)
+    (letrec ( ; append 2 flag-sublist and make sure no duplicate flag-string
+             (append-flag-sublist
+              (λ(flag-sublist1 flag-sublist2)
+                (cond ((null? flag-sublist1) flag-sublist2)
+                      ((null? flag-sublist2) flag-sublist1)
+                      (else
+                       (cons (flag-type flag-sublist1)
+                             (lset-union string=?
+                                         (flag-string-list flag-sublist1)
+                                         (flag-string-list flag-sublist2)))))))
+             ;; join list of flag-sublist using append-flag-sublist
+             (join-flag-sublist
+              (λ(list-of-flag-sublist)
+                (fold append-flag-sublist '() list-of-flag-sublist))))
+      (if (null? flag-lst)
+          '()
+          (let* ((current-type (flag-type (car flag-lst)))
+                 (same-type? (λ(flag-sublist)
+                               (eq? (flag-type flag-sublist)
+                                    current-type))))
+            (cons (join-flag-sublist
+                   (take-while same-type? flag-lst))
+                  (merge-sorted-flag-list
+                   (drop-while same-type? flag-lst)))))))
+  ((compose merge-sorted-flag-list
+            sort-flag-list
+            trim-flag-string)
+   flag-lst))
+
+;;; set-like operators for flag-list
+(define (flag-list+ . list-of-flag-list)
+  (apply flag-list (concatenate list-of-flag-list)))
+(define (flag-list- flag-list1 . list-of-flag-list)
+  (define (flag-list-difference flag-sublist1 flag-list)
+    (let ((found (find (λ(flag-sublist2)
+                         (eq? (flag-type flag-sublist1)
+                              (flag-type flag-sublist2)))
+                       flag-list)))
+      (if (eq? found #f)
+          flag-sublist1
+          (cons (flag-type flag-sublist1)
+                (lset-difference string=?
+                                 (flag-string-list flag-sublist1)
+                                 (flag-string-list found))))))
+  (let ((flag-list2 (apply flag-list+ list-of-flag-list)))
+    (map (λ(flag-sublist)
+           (flag-list-difference flag-sublist flag-list2))
+         flag-list1)))
+
+;;; convert flag-list to string-list
+(define (flag-list->string-list flag-lst)
+  (map (λ(flag-sublist)
+         (let ((environment-variable
+                (string-append (symbol->string
+                                (flag-type flag-sublist))
+                               "=")))
+           (string-join (cons environment-variable
+                              (flag-string-list flag-sublist)))))
+       flag-lst))
+
+
+;;; build flags used in dpkg-buildflags
+
+(define default-flag-list
+  (flag-list
+   '(CFLAGS "-g" "-O2")))
+
+(define format-flag-list
+  (flag-list
+   '(CFLAGS "-Wformat" "-Werror=format-security")))
+
+(define fortify-flag-list
+  (flag-list
+   '(CPPFLAGS "-D_FORTIFY_SOURCE=2")))
+
+(define stackprotectorstrong-flag-list
+  (flag-list
+   '(CFLAGS "-fstack-protector-strong")))
+
+(define relro-flag-list
+  (flag-list
+   '(LDFLAGS "-Wl,-z,relro")))
+
+(define bind-now-flag-list
+  (flag-list
+   '(LDFLAGS "-Wl,-z,now")))
+
+(define pie-flag-list
+  (flag-list
+   '(CFLAGS "-fPIE")
+   '(LDFLAGS "-fPIE" "-pie")))
+
+(define all-flag-list
+  (flag-list+ default-flag-list
+              format-flag-list
+              fortify-flag-list
+              stackprotectorstrong-flag-list
+              relro-flag-list
+              bind-now-flag-list
+              pie-flag-list))
+
+
+;;; implement the bootstrap-build-system using syntax-case macro
+;;; bootstrap-build-system use a bootstrap script
+;;; to run autoreconf and generate documentation.
+(define-syntax package*
+  (lambda(x)
+    ;; add autoconf, automake and perl as build dependencies
+    ;; Modify the gnu-build-system
+    ;; by adding bootstrap phase before configure phase.
+    (define (extend-fields s-exp)
+      (cond ((eq? (car s-exp) 'inputs)
+	     (list 'inputs
+		   (list 'quasiquote
+			 (append '(("autoconf" ,autoconf)
+				   ("automake" ,automake)
+				   ("perl" ,perl))
+				 (cadadr s-exp)))))
+	    ((eq? (car s-exp) 'arguments)
+	     (list
+	      'arguments
+	      (list
+	       'quasiquote
+	       (append
+		'(#:phases
+		  (modify-phases %standard-phases
+				 (add-before 'configure
+					     'bootstrap
+					     (λ _
+					       (zero?
+						(system "./bootstrap"))))))
+		(cadadr s-exp)))))
+	    (else s-exp)))
+    (syntax-case x ()
+      ((_ . lst)
+       (if (any (λ(sublist)
+		  (equal? sublist
+			  '(build-system
+			    bootstrap-build-system)))
+		(syntax->datum #'lst))
+	   #`(package (build-system gnu-build-system)
+		      #,@(datum->syntax
+			  x
+			  (map extend-fields
+			       (remove (λ(sublist)
+					 (equal? sublist
+						 '(build-system
+						   bootstrap-build-system)))
+				       (syntax->datum #'lst)))))
+	   #`(package #,@ #'lst))))))
+
+
+(define-public mlucas
+  ;; descriptions of the package
+  (let ((short-description
+         "Program to perform Lucas-Lehmer test on a Mersenne number")
+        (long-description
+         "mlucas is an open-source (and free/libre) program
+for performing Lucas-Lehmer test on prime-exponent Mersenne numbers,
+that is, integers of the form 2 ^ p - 1, with prime exponent p.
+In short, everything you need to search for world-record Mersenne primes!
+It has been used in the verification of various Mersenne primes,
+including the 45th, 46th and 48th found Mersenne prime.
+
+You may use it to test any suitable number as you wish,
+but it is preferable that you do so in a coordinated fashion,
+as part of the Great Internet Mersenne Prime Search (GIMPS).
+For more information on GIMPS,
+see <http://www.mersenne.org/prime.html> for details.
+")
+        ;; some dpkg-buildflags and custom build flags presented as flag-list
+        (custom-flag-list
+         (flag-list-
+          (flag-list+ all-flag-list
+                      (flag-list
+                       '(CFLAGS "-Ofast"
+                                "-pipe"
+                                "-flto"
+                                "-fno-aggressive-loop-optimizations")
+                       '(LDFLAGS "-Wl,--as-needed")))
+          default-flag-list)))
+    ;; start package definition
+    (package*
+     (name "mlucas")
+     (version "14.1")
+     (source (origin
+	      (method url-fetch)
+	      (uri (string-append "http://hogranch.com/mayer/src/C/mlucas-"
+				  version
+				  ".tar.xz"))
+	      (sha256
+	       (base32
+		"1i6j1479icxfwp3ixs6dk65qilv9hn7213q3iibndlgwjfmh0gb4"))))
+     (build-system bootstrap-build-system)
+     (arguments
+      `(#:configure-flags
+	'("--disable-NORMAL-CFLAGS"
+	  "--disable-TRICKY-CFLAGS"
+	  "--enable-MLUCAS-DEFAULT-PATH"
+	  "--enable-verbose-compiler"
+	  ,@(flag-list->string-list custom-flag-list))))
+     (inputs `(("autogen" ,autogen)))
+     (synopsis short-description)
+     (description long-description)
+     (home-page "http://hogranch.com/mayer/README.html")
+     (license gpl2+))))
-- 
2.6.0

Re: [PATCH] gnu: Add Mlucas.

[Mathieu Lirzin]

Alex Vong <alexvong1995@gmail.com> writes:

> From e5155b52f636bfee849268b19b81f5b6608540fd Mon Sep 17 00:00:00 2001
> From: Alex Vong <alexvong1995@gmail.com>
> Date: Mon, 5 Oct 2015 12:49:49 +0800
> Subject: [PATCH] gnu: Add Mlucas.
>
> * gnu/packages/mlucas.scm: New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Register it.
> ---

This is quite an unusual patch.  :)

>  gnu-system.am           |   1 +
>  gnu/packages/mlucas.scm | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 284 insertions(+)
>  create mode 100644 gnu/packages/mlucas.scm
>
[...]
> +(define-module (gnu packages mlucas)
> +  #:use-module (srfi srfi-1)
> +  #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix build-system gnu)
> +  #:use-module (guix licenses)
> +  #:use-module (gnu packages autogen)
> +  #:use-module (gnu packages autotools)
> +  #:use-module (gnu packages perl))
> +
> +
> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
> +;;;
> +;;; The data strcture flag-list is constrcuted by (flag-list <flag-sublist>...)
                                            ^^
“constructed”

> +;;; The constructor flag-list does something to the argument,
> +;;; such as trimming whitespaces, to ensure no two arguments mean the same.
> +;;;
> +;;; The data structure flag-sublist is in fact an ordinary list
> +;;; with the following structure (<flag-type-symbol> <flag-string>...)
> +;;;
> +;;; Here is an example:
> +;;; (flag-list
> +;;;  '(CFLAGS "-O2" "-g")
> +;;;  '(LDFLAGS "-lm" "-lpthread"))
> +;;;
> +;;; flag-list+ and flag-list- are analogous to
> +;;; numberic + and - but operate on flag-list.
> +;;;
> +;;; flag-list->string-list converts flag-list into
> +;;; configure-flags-compatible string-list.
> +;;;

IIUC these procedures are not meant to be specific the definition of package
mlucas.  So this should be in some other commit and maybe located in other
file in “guix/guix/build/...” directory.

Can you explain the problem you faced with the current Guix methods to achieve
that, and what are the benefits of ‘flag-list’ comparing to them?

Example:
--8<---------------cut here---------------start------------->8---
    (arguments
     '(#:configure-flags ...
       #:make-flags ...
--8<---------------cut here---------------end--------------->8---

[...]
> +;;; implement the bootstrap-build-system using syntax-case macro
> +;;; bootstrap-build-system use a bootstrap script
> +;;; to run autoreconf and generate documentation.
> +(define-syntax package*
> +  (lambda(x)
> +    ;; add autoconf, automake and perl as build dependencies
> +    ;; Modify the gnu-build-system
> +    ;; by adding bootstrap phase before configure phase.
> +    (define (extend-fields s-exp)

I'm not competent enough to judge if it's a useful build-system to add but
this should be done in another commit and in
“guix/guix/build/bootstap-build-system.scm” if yes.

> +
> +(define-public mlucas
> +  ;; descriptions of the package
> +  (let ((short-description
> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
> +        (long-description
> +         "mlucas is an open-source (and free/libre) program
                          ^^^

Being a GNU project, we use the term “free software”, but in the context of a
description it is not relevant to describe freedom of a package since every
package in Guix is free software.

I think there will be more details to cover for an extensive review, but let's
discuss the big lines first.

Thanks for your contribution!

--
Mathieu Lirzin

Re: [PATCH] gnu: Add Mlucas.

[Alex Kost]

Hello and thank you for contributing.  This is a tremendous patch for
the first attempt!

As Mathieu noted, if this auxiliary code for adding 'flags' is needed,
it should be separated from the package commit and it shouldn't be
placed in the package module.

You will probably receive useful comments on the matter of the patch, I
just have some general notes on the scheme code.

Please remove TABs: we use spaces instead of tabulation.

Typos:

[...]
> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
                     manipulate

> +;;;
> +;;; The data strcture flag-list is constrcuted by (flag-list <flag-sublist>...)
                structure             constructed

> +;;; The constructor flag-list does something to the argument,
> +;;; such as trimming whitespaces, to ensure no two arguments mean the same.
> +;;;
> +;;; The data structure flag-sublist is in fact an ordinary list
> +;;; with the following structure (<flag-type-symbol> <flag-string>...)
> +;;;
> +;;; Here is an example:
> +;;; (flag-list
> +;;;  '(CFLAGS "-O2" "-g")
> +;;;  '(LDFLAGS "-lm" "-lpthread"))
> +;;;
> +;;; flag-list+ and flag-list- are analogous to
> +;;; numberic + and - but operate on flag-list.
       numeric

> +;;; flag-list->string-list converts flag-list into
> +;;; configure-flags-compatible string-list.
> +;;;
> +
> +;;; selectors of flag-sublist
> +(define (flag-type flag-sublist)
> +  (car flag-sublist))
> +(define (flag-string-list flag-sublist)
> +  (cdr flag-sublist))

IMO it is clearer to write it like this:

  (define flag-type first)
  (define flag-string-list second)

Although I think it is better to use records for such things.  We also
have 'define-record-type*' in (guix records) module.

(also some people don't like car/cdr with passion)

> +;;; constructor of flag-list
> +(define (flag-list . flag-lst)
> +  ;; Trim leading and trailing whitespaces of all flag-string
> +  ;; in flag-list.
> +  (define (trim-flag-string flag-lst)
> +    (map (λ(flag-sublist)

We use 'lambda'.  I'm personally not against 'λ', but maybe others
wouldn't like it.  Anyway a common convention is to have a space before
"(", i.e.:

  (map (λ (flag-sublist) ...))

[...]
> +;;; implement the bootstrap-build-system using syntax-case macro
> +;;; bootstrap-build-system use a bootstrap script
> +;;; to run autoreconf and generate documentation.
> +(define-syntax package*

This is not how new build systems are implemented.  Look at
(guix build-system ...) modules.

Also I'm not sure if it is worth to make a new build system just for
adding 'autoreconf' phase.  If you grep package modules for "autoreconf"
or "autogen", you will see many examples how to add such a phase to a
gnu-build-system.

> +  (lambda(x)
> +    ;; add autoconf, automake and perl as build dependencies
> +    ;; Modify the gnu-build-system
> +    ;; by adding bootstrap phase before configure phase.
> +    (define (extend-fields s-exp)
> +      (cond ((eq? (car s-exp) 'inputs)
> +	     (list 'inputs
> +		   (list 'quasiquote
> +			 (append '(("autoconf" ,autoconf)
> +				   ("automake" ,automake)
> +				   ("perl" ,perl))
> +				 (cadadr s-exp)))))

And absolutely all people don't like 'cadadr'!!  Please use 'match' for
such things:
<https://www.gnu.org/software/guile/manual/html_node/Pattern-Matching.html>.

[...]
> +(define-public mlucas
> +  ;; descriptions of the package
> +  (let ((short-description
> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
> +        (long-description
> +         "mlucas is an open-source (and free/libre) program
> +for performing Lucas-Lehmer test on prime-exponent Mersenne numbers,
> +that is, integers of the form 2 ^ p - 1, with prime exponent p.
> +In short, everything you need to search for world-record Mersenne primes!
> +It has been used in the verification of various Mersenne primes,
> +including the 45th, 46th and 48th found Mersenne prime.
> +
> +You may use it to test any suitable number as you wish,
> +but it is preferable that you do so in a coordinated fashion,
> +as part of the Great Internet Mersenne Prime Search (GIMPS).
> +For more information on GIMPS,
> +see <http://www.mersenne.org/prime.html> for details.
> +")

This is not necessary, just put these directly into 'synopsis' and
'description'.

-- 
Alex

Re: [PATCH] gnu: Add Mlucas.

[Alex Vong]

Hi Mathieu,

On 05/10/2015, Mathieu Lirzin <mthl@openmailbox.org> wrote:
> Alex Vong <alexvong1995@gmail.com> writes:
>
>> From e5155b52f636bfee849268b19b81f5b6608540fd Mon Sep 17 00:00:00 2001
>> From: Alex Vong <alexvong1995@gmail.com>
>> Date: Mon, 5 Oct 2015 12:49:49 +0800
>> Subject: [PATCH] gnu: Add Mlucas.
>>
>> * gnu/packages/mlucas.scm: New file.
>> * gnu-system.am (GNU_SYSTEM_MODULES): Register it.
>> ---
>
> This is quite an unusual patch.  :)
>
>>  gnu-system.am           |   1 +
>>  gnu/packages/mlucas.scm | 283
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 284 insertions(+)
>>  create mode 100644 gnu/packages/mlucas.scm
>>
> [...]
>> +(define-module (gnu packages mlucas)
>> +  #:use-module (srfi srfi-1)
>> +  #:use-module (guix packages)
>> +  #:use-module (guix download)
>> +  #:use-module (guix build-system gnu)
>> +  #:use-module (guix licenses)
>> +  #:use-module (gnu packages autogen)
>> +  #:use-module (gnu packages autotools)
>> +  #:use-module (gnu packages perl))
>> +
>> +
>> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
>> +;;;
>> +;;; The data strcture flag-list is constrcuted by (flag-list
>> <flag-sublist>...)
>                                             ^^
> “constructed”
How do you manage to detect this? I have run codespell on the file but
it doesn't report anything!

>
>> +;;; The constructor flag-list does something to the argument,
>> +;;; such as trimming whitespaces, to ensure no two arguments mean the
>> same.
>> +;;;
>> +;;; The data structure flag-sublist is in fact an ordinary list
>> +;;; with the following structure (<flag-type-symbol> <flag-string>...)
>> +;;;
>> +;;; Here is an example:
>> +;;; (flag-list
>> +;;;  '(CFLAGS "-O2" "-g")
>> +;;;  '(LDFLAGS "-lm" "-lpthread"))
>> +;;;
>> +;;; flag-list+ and flag-list- are analogous to
>> +;;; numberic + and - but operate on flag-list.
>> +;;;
>> +;;; flag-list->string-list converts flag-list into
>> +;;; configure-flags-compatible string-list.
>> +;;;
>
> IIUC these procedures are not meant to be specific the definition of
> package
> mlucas.  So this should be in some other commit and maybe located in other
> file in “guix/guix/build/...” directory.
>
> Can you explain the problem you faced with the current Guix methods to
> achieve
> that, and what are the benefits of ‘flag-list’ comparing to them?
>
Flag-list is something similar to dpkg-buildflags(1), it has several
sets of flags which are useful to almost all programs. For example,
fortify flags will attempt to replace insecure unlimited length buffer
function calls with length-limited ones. stackprotectorstrong will
protect your program from stack smashing. In Debian, there is also a
set of default build flags for packaging. I think it is a good idea,
since not all upstream know these source hardening related flags, we
should be using those by default but with an option to turn off
individual sets of flags. This is like a layer above configure flags
and make flags.

> Example:
> --8<---------------cut here---------------start------------->8---
>     (arguments
>      '(#:configure-flags ...
>        #:make-flags ...
> --8<---------------cut here---------------end--------------->8---
>
> [...]
>> +;;; implement the bootstrap-build-system using syntax-case macro
>> +;;; bootstrap-build-system use a bootstrap script
>> +;;; to run autoreconf and generate documentation.
>> +(define-syntax package*
>> +  (lambda(x)
>> +    ;; add autoconf, automake and perl as build dependencies
>> +    ;; Modify the gnu-build-system
>> +    ;; by adding bootstrap phase before configure phase.
>> +    (define (extend-fields s-exp)
>
> I'm not competent enough to judge if it's a useful build-system to add but
> this should be done in another commit and in
> “guix/guix/build/bootstap-build-system.scm” if yes.
>
I should be more a newbie than you do. I just wonder if it is possible
to modify the build system by a little, such as adding a bootstrap
phase and some new inputs, and then give a name to the new build
system. Since it seems many packages are repeating this step, I think
it will be great if we have this mean of abstraction.

By the way, I know the macro looks ugly since this is my first time
writing a macro.

>> +
>> +(define-public mlucas
>> +  ;; descriptions of the package
>> +  (let ((short-description
>> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
>> +        (long-description
>> +         "mlucas is an open-source (and free/libre) program
>                           ^^^
>
> Being a GNU project, we use the term “free software”, but in the context of
> a
> description it is not relevant to describe freedom of a package since every
> package in Guix is free software.
>
"open-source" is actually mentioned in the upstream description, so I
add the description inside the parenthesis. But I can remove both if
it is desired.

> I think there will be more details to cover for an extensive review, but
> let's
> discuss the big lines first.
>
> Thanks for your contribution!
>
Thanks!

> --
> Mathieu Lirzin
>
>

Cheers,
Alex

Re: [PATCH] gnu: Add Mlucas.

[Ludovic Courtès]

Alex Vong <alexvong1995@gmail.com> skribis:

> On 05/10/2015, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>> Alex Vong <alexvong1995@gmail.com> writes:

[...]

>> I'm not competent enough to judge if it's a useful build-system to add but
>> this should be done in another commit and in
>> “guix/guix/build/bootstap-build-system.scm” if yes.
>>
> I should be more a newbie than you do. I just wonder if it is possible
> to modify the build system by a little, such as adding a bootstrap
> phase and some new inputs, and then give a name to the new build
> system. Since it seems many packages are repeating this step, I think
> it will be great if we have this mean of abstraction.

It’s not uncommon, indeed.  However, the details on how to bootstrap are
not standard: Often ‘autoreconf -vfi’ will do, sometimes it’s
‘./bootstrap’, sometimes ‘./autogen.sh’, etc.

Now the proposed build system could maybe try these variants one after
the other.

Also, the set of dependencies varies: sometimes it’s Autoconf, sometimes
Autoconf+Automake, sometimes Autoconf+Automake+Libtool, etc.  So I think
the set of dependencies should be kept explicit–i.e., packages have to
add stuff to ‘native-inputs’.

Could you try to make this build system as a standalone commit, leaving
out the build flags code for a separate discussion?

The commit would add (guix build-system gnu-bootstrap) for instance (I
call it this way because it bootstraps specifically the GNU build
system, not CMake, etc.) and (guix build gnu-bootstrap-build-system).
The latter would simply add one phase to ‘%standard-phases’.

Does that make sense?

>>> +
>>> +(define-public mlucas
>>> +  ;; descriptions of the package
>>> +  (let ((short-description
>>> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
>>> +        (long-description
>>> +         "mlucas is an open-source (and free/libre) program
>>                           ^^^
>>
>> Being a GNU project, we use the term “free software”, but in the context of
>> a
>> description it is not relevant to describe freedom of a package since every
>> package in Guix is free software.
>>
> "open-source" is actually mentioned in the upstream description, so I
> add the description inside the parenthesis. But I can remove both if
> it is desired.

Yes please.  Everything is free in Guix anyway.  :-)

Also, as Alex mentions, the synopsis and description must be literal
strings in the ‘description’ and ‘synopsis’ fields.  This allows those
strings to be extracted for translation (see “Synopses and Descriptions”
in the manual.)

Thank you, and welcome!

Ludo’.

Re: [PATCH] gnu: Add Mlucas.

[Alex Vong]

Hi Alex,

On 06/10/2015, Alex Kost <alezost@gmail.com> wrote:
> Hello and thank you for contributing.  This is a tremendous patch for
> the first attempt!
>
> As Mathieu noted, if this auxiliary code for adding 'flags' is needed,
> it should be separated from the package commit and it shouldn't be
> placed in the package module.
>
> You will probably receive useful comments on the matter of the patch, I
> just have some general notes on the scheme code.
>
> Please remove TABs: we use spaces instead of tabulation.
>
> Typos:
>
> [...]
>> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
>                      manipulate
>
>> +;;;
>> +;;; The data strcture flag-list is constrcuted by (flag-list
>> <flag-sublist>...)
>                 structure             constructed
>
>> +;;; The constructor flag-list does something to the argument,
>> +;;; such as trimming whitespaces, to ensure no two arguments mean the
>> same.
>> +;;;
>> +;;; The data structure flag-sublist is in fact an ordinary list
>> +;;; with the following structure (<flag-type-symbol> <flag-string>...)
>> +;;;
>> +;;; Here is an example:
>> +;;; (flag-list
>> +;;;  '(CFLAGS "-O2" "-g")
>> +;;;  '(LDFLAGS "-lm" "-lpthread"))
>> +;;;
>> +;;; flag-list+ and flag-list- are analogous to
>> +;;; numberic + and - but operate on flag-list.
>        numeric
>
Fixed. Thanks for telling me the TAB issue and spelling mistakes.

>> +;;; flag-list->string-list converts flag-list into
>> +;;; configure-flags-compatible string-list.
>> +;;;
>> +
>> +;;; selectors of flag-sublist
>> +(define (flag-type flag-sublist)
>> +  (car flag-sublist))
>> +(define (flag-string-list flag-sublist)
>> +  (cdr flag-sublist))
>
> IMO it is clearer to write it like this:
>
>   (define flag-type first)
>   (define flag-string-list second)
>
> Although I think it is better to use records for such things.  We also
> have 'define-record-type*' in (guix records) module.
>
> (also some people don't like car/cdr with passion)
>
I think SECOND is CADR instead of CDR. Am I right? I will read about
DEFINE-RECORD-TYPE, it sounds fun to define new types.

>> +;;; constructor of flag-list
>> +(define (flag-list . flag-lst)
>> +  ;; Trim leading and trailing whitespaces of all flag-string
>> +  ;; in flag-list.
>> +  (define (trim-flag-string flag-lst)
>> +    (map (λ(flag-sublist)
>
> We use 'lambda'.  I'm personally not against 'λ', but maybe others
> wouldn't like it.  Anyway a common convention is to have a space before
> "(", i.e.:
>
>   (map (λ (flag-sublist) ...))
>
I used to use LAMBDA, but one day I discovered Guile supports λ, so I
have used it since then. I will follow the space convention anyway.

> [...]
>> +;;; implement the bootstrap-build-system using syntax-case macro
>> +;;; bootstrap-build-system use a bootstrap script
>> +;;; to run autoreconf and generate documentation.
>> +(define-syntax package*
>
> This is not how new build systems are implemented.  Look at
> (guix build-system ...) modules.
>
> Also I'm not sure if it is worth to make a new build system just for
> adding 'autoreconf' phase.  If you grep package modules for "autoreconf"
> or "autogen", you will see many examples how to add such a phase to a
> gnu-build-system.
>
>> +  (lambda(x)
>> +    ;; add autoconf, automake and perl as build dependencies
>> +    ;; Modify the gnu-build-system
>> +    ;; by adding bootstrap phase before configure phase.
>> +    (define (extend-fields s-exp)
>> +      (cond ((eq? (car s-exp) 'inputs)
>> +	     (list 'inputs
>> +		   (list 'quasiquote
>> +			 (append '(("autoconf" ,autoconf)
>> +				   ("automake" ,automake)
>> +				   ("perl" ,perl))
>> +				 (cadadr s-exp)))))
>
> And absolutely all people don't like 'cadadr'!!  Please use 'match' for
> such things:
> <https://www.gnu.org/software/guile/manual/html_node/Pattern-Matching.html>.
>
Yes, (second (second ...) is probably better than cadadr. I should
really try pattern matcher. Do you know any tutorial on it? However, I
think I am not making a new build system anymore. The reason will be
noted on the next reply.

> [...]
>> +(define-public mlucas
>> +  ;; descriptions of the package
>> +  (let ((short-description
>> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
>> +        (long-description
>> +         "mlucas is an open-source (and free/libre) program
>> +for performing Lucas-Lehmer test on prime-exponent Mersenne numbers,
>> +that is, integers of the form 2 ^ p - 1, with prime exponent p.
>> +In short, everything you need to search for world-record Mersenne
>> primes!
>> +It has been used in the verification of various Mersenne primes,
>> +including the 45th, 46th and 48th found Mersenne prime.
>> +
>> +You may use it to test any suitable number as you wish,
>> +but it is preferable that you do so in a coordinated fashion,
>> +as part of the Great Internet Mersenne Prime Search (GIMPS).
>> +For more information on GIMPS,
>> +see <http://www.mersenne.org/prime.html> for details.
>> +")
>
> This is not necessary, just put these directly into 'synopsis' and
> 'description'.
>
No problem, I will be fixing those.

> --
> Alex
>
Thanks for the detailed suggestion. My reply is kind of short compared to them.

Cheers,
Alex

Re: [PATCH] gnu: Add Mlucas.

[Alex Vong]

Hi Ludovic,

On 06/10/2015, Ludovic Courtès <ludo@gnu.org> wrote:
> Alex Vong <alexvong1995@gmail.com> skribis:
>
>> On 05/10/2015, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>>> Alex Vong <alexvong1995@gmail.com> writes:
>
> [...]
>
>>> I'm not competent enough to judge if it's a useful build-system to add
>>> but
>>> this should be done in another commit and in
>>> “guix/guix/build/bootstap-build-system.scm” if yes.
>>>
>> I should be more a newbie than you do. I just wonder if it is possible
>> to modify the build system by a little, such as adding a bootstrap
>> phase and some new inputs, and then give a name to the new build
>> system. Since it seems many packages are repeating this step, I think
>> it will be great if we have this mean of abstraction.
>
> It’s not uncommon, indeed.  However, the details on how to bootstrap are
> not standard: Often ‘autoreconf -vfi’ will do, sometimes it’s
> ‘./bootstrap’, sometimes ‘./autogen.sh’, etc.
>
> Now the proposed build system could maybe try these variants one after
> the other.
>
> Also, the set of dependencies varies: sometimes it’s Autoconf, sometimes
> Autoconf+Automake, sometimes Autoconf+Automake+Libtool, etc.  So I think
> the set of dependencies should be kept explicit–i.e., packages have to
> add stuff to ‘native-inputs’.
>
> Could you try to make this build system as a standalone commit, leaving
> out the build flags code for a separate discussion?
>
> The commit would add (guix build-system gnu-bootstrap) for instance (I
> call it this way because it bootstraps specifically the GNU build
> system, not CMake, etc.) and (guix build gnu-bootstrap-build-system).
> The latter would simply add one phase to ‘%standard-phases’.
>
> Does that make sense?
>
I think if the set of dependencies should be kept explicit, then it
seems the only thing we are left to abstract away is trying the
commands ``autoreconf -vfi'', ``./bootstrap'' and ``./autogen.sh''.
But I think the command is better left for the package maintainer to
decide since the bootstrap script may have unusual name. (I have seen
``bootstrap.sh'' for instance.) My original though is to let the
package maintainer to pass in the bootstrap command string. However,
if it is the case, then gnu-bootstrap build-system isn't abstracting
anything at all. So I think I'll go back to the original solution of
adding a new phase and specifying autotools dependencies instead.

>>>> +
>>>> +(define-public mlucas
>>>> +  ;; descriptions of the package
>>>> +  (let ((short-description
>>>> +         "Program to perform Lucas-Lehmer test on a Mersenne number")
>>>> +        (long-description
>>>> +         "mlucas is an open-source (and free/libre) program
>>>                           ^^^
>>>
>>> Being a GNU project, we use the term “free software”, but in the context
>>> of
>>> a
>>> description it is not relevant to describe freedom of a package since
>>> every
>>> package in Guix is free software.
>>>
>> "open-source" is actually mentioned in the upstream description, so I
>> add the description inside the parenthesis. But I can remove both if
>> it is desired.
>
> Yes please.  Everything is free in Guix anyway.  :-)
>
> Also, as Alex mentions, the synopsis and description must be literal
> strings in the ‘description’ and ‘synopsis’ fields.  This allows those
> strings to be extracted for translation (see “Synopses and Descriptions”
> in the manual.)
>
Yes I will fix those.

> Thank you, and welcome!
>
> Ludo’.
>
Thanks too!

Cheers,
Alex

Re: [PATCH] gnu: Add Mlucas.

[Alex Kost]

Alex Vong (2015-10-06 16:43 +0300) wrote:

> Hi Alex,
>
> On 06/10/2015, Alex Kost <alezost@gmail.com> wrote:
[...]
>>> +;;; selectors of flag-sublist
>>> +(define (flag-type flag-sublist)
>>> +  (car flag-sublist))
>>> +(define (flag-string-list flag-sublist)
>>> +  (cdr flag-sublist))
>>
>> IMO it is clearer to write it like this:
>>
>>   (define flag-type first)
>>   (define flag-string-list second)
>>
>> Although I think it is better to use records for such things.  We also
>> have 'define-record-type*' in (guix records) module.
>>
>> (also some people don't like car/cdr with passion)
>>
> I think SECOND is CADR instead of CDR. Am I right? I will read about
> DEFINE-RECORD-TYPE, it sounds fun to define new types.

Ouch, yes, 'second' is 'cadr', my bad.  Actually I think 'first',
'second', etc. are not much better then 'car', 'cadr', etc.  It is
usually possible and desirable to avoid such things, although I confess
I use 'car'-s sometimes (when Ludovic doesn't see :-)).

>>> +;;; constructor of flag-list
>>> +(define (flag-list . flag-lst)
>>> +  ;; Trim leading and trailing whitespaces of all flag-string
>>> +  ;; in flag-list.
>>> +  (define (trim-flag-string flag-lst)
>>> +    (map (λ(flag-sublist)
>>
>> We use 'lambda'.  I'm personally not against 'λ', but maybe others
>> wouldn't like it.  Anyway a common convention is to have a space before
>> "(", i.e.:
>>
>>   (map (λ (flag-sublist) ...))
>>
> I used to use LAMBDA, but one day I discovered Guile supports λ, so I
> have used it since then. I will follow the space convention anyway.

I like "λ" and I will use it too, if it is (will be) allowed.  Thanks
for raising this.

[...]
>> And absolutely all people don't like 'cadadr'!!  Please use 'match' for
>> such things:
>> <https://www.gnu.org/software/guile/manual/html_node/Pattern-Matching.html>.
>>
> Yes, (second (second ...) is probably better than cadadr. I should
> really try pattern matcher. Do you know any tutorial on it? However, I
> think I am not making a new build system anymore. The reason will be
> noted on the next reply.

I've never searched for a tutorial.  I learnt how to use 'match' from
the Guile manual and the Guix source code.  There is also a cool
'match-lambda' macro which can be often met in the Guix code, but it is
not documented in the Guile manual.

-- 
Alex

Re: [PATCH] gnu: Add Mlucas.

[Mathieu Lirzin]

Alex Vong <alexvong1995@gmail.com> writes:

> On 05/10/2015, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>> Alex Vong <alexvong1995@gmail.com> writes:
>>> +
>>> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
>>> +;;;
>>> +;;; The data strcture flag-list is constrcuted by (flag-list
>>> <flag-sublist>...)
>>                                             ^^
>> “constructed”
> How do you manage to detect this? I have run codespell on the file but
> it doesn't report anything!

With nothing more than my eagle eyes!  ;)

--
Mathieu Lirzin

Re: [PATCH] gnu: Add Mlucas.

[Alex Kost]

Mathieu Lirzin (2015-10-06 18:06 +0300) wrote:

> Alex Vong <alexvong1995@gmail.com> writes:
>
>> On 05/10/2015, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>>> Alex Vong <alexvong1995@gmail.com> writes:
>>>> +
>>>> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
>>>> +;;;
>>>> +;;; The data strcture flag-list is constrcuted by (flag-list
>>>> <flag-sublist>...)
>>>                                             ^^
>>> “constructed”
>> How do you manage to detect this? I have run codespell on the file but
>> it doesn't report anything!
>
> With nothing more than my eagle eyes!  ;)

I used "M-x flyspell-buffer" to find the others.

-- 
Alex

Re: [PATCH] gnu: Add Mlucas.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Ouch, yes, 'second' is 'cadr', my bad.  Actually I think 'first',
> 'second', etc. are not much better then 'car', 'cadr', etc.  It is
> usually possible and desirable to avoid such things, although I confess
> I use 'car'-s sometimes (when Ludovic doesn't see :-)).

Bah, caught you!

Ludo’.

Re: [PATCH] gnu: Add Mlucas.

[Ludovic Courtès]

Hi!

Alex Vong <alexvong1995@gmail.com> skribis:

> On 06/10/2015, Ludovic Courtès <ludo@gnu.org> wrote:

[...]

>> It’s not uncommon, indeed.  However, the details on how to bootstrap are
>> not standard: Often ‘autoreconf -vfi’ will do, sometimes it’s
>> ‘./bootstrap’, sometimes ‘./autogen.sh’, etc.
>>
>> Now the proposed build system could maybe try these variants one after
>> the other.
>>
>> Also, the set of dependencies varies: sometimes it’s Autoconf, sometimes
>> Autoconf+Automake, sometimes Autoconf+Automake+Libtool, etc.  So I think
>> the set of dependencies should be kept explicit–i.e., packages have to
>> add stuff to ‘native-inputs’.
>>
>> Could you try to make this build system as a standalone commit, leaving
>> out the build flags code for a separate discussion?
>>
>> The commit would add (guix build-system gnu-bootstrap) for instance (I
>> call it this way because it bootstraps specifically the GNU build
>> system, not CMake, etc.) and (guix build gnu-bootstrap-build-system).
>> The latter would simply add one phase to ‘%standard-phases’.
>>
>> Does that make sense?
>>
> I think if the set of dependencies should be kept explicit, then it
> seems the only thing we are left to abstract away is trying the
> commands ``autoreconf -vfi'', ``./bootstrap'' and ``./autogen.sh''.
> But I think the command is better left for the package maintainer to
> decide since the bootstrap script may have unusual name. (I have seen
> ``bootstrap.sh'' for instance.) My original though is to let the
> package maintainer to pass in the bootstrap command string. However,
> if it is the case, then gnu-bootstrap build-system isn't abstracting
> anything at all. So I think I'll go back to the original solution of
> adding a new phase and specifying autotools dependencies instead.

Yeah, makes sense.  A phase that guesses the command to run may still
save a few lines here and there, but I agree that it may not be that
beneficial overall.

Thanks,
Ludo’.