Re: [PATCH] gnu: Add Mlucas.

[Alex Kost <alezost@gmail.com>]

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