From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Vong Subject: Re: [PATCH] gnu: Add Mlucas. Date: Tue, 6 Oct 2015 21:43:14 +0800 Message-ID: References: <20151005130123.2091f6e4@debian> <87zizx5kte.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52798) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjSWS-0002aN-Ui for guix-devel@gnu.org; Tue, 06 Oct 2015 09:43:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjSWR-000176-K5 for guix-devel@gnu.org; Tue, 06 Oct 2015 09:43:16 -0400 Received: from mail-ig0-x232.google.com ([2607:f8b0:4001:c05::232]:34841) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjSWR-000172-FH for guix-devel@gnu.org; Tue, 06 Oct 2015 09:43:15 -0400 Received: by igbkq10 with SMTP id kq10so84649963igb.0 for ; Tue, 06 Oct 2015 06:43:15 -0700 (PDT) In-Reply-To: <87zizx5kte.fsf@gmail.com> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Alex Kost Cc: guix-devel@gnu.org Hi Alex, On 06/10/2015, Alex Kost 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 >> ...) > 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 ( ...) >> +;;; >> +;;; 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 (=CE=BB(flag-sublist) > > We use 'lambda'. I'm personally not against '=CE=BB', but maybe others > wouldn't like it. Anyway a common convention is to have a space before > "(", i.e.: > > (map (=CE=BB (flag-sublist) ...)) > I used to use LAMBDA, but one day I discovered Guile supports =CE=BB, 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: > . > 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 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 t= hem. Cheers, Alex