Re: [PATCH] gnu: Add Mlucas.

[Alex Vong <alexvong1995@gmail.com>]

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