[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: Add Mlucas.
Re: [PATCH] gnu: Add Mlucas.
Tue, 6 Oct 2015 21:43:14 +0800
On 06/10/2015, Alex Kost <address@hidden> 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.
>> +;;; Procedures to manupulate build flags, similar to dpkg-buildflags.
>> +;;; The data strcture flag-list is constrcuted by (flag-list
> structure constructed
>> +;;; The constructor flag-list does something to the argument,
>> +;;; such as trimming whitespaces, to ensure no two arguments mean the
>> +;;; 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.
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
>> + (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:
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
>> +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
No problem, I will be fixing those.
Thanks for the detailed suggestion. My reply is kind of short compared to them.