From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH v2 2/6] gnu: fpga: Add abc. Date: Thu, 18 Aug 2016 12:24:00 +0200 Message-ID: <87twei74u7.fsf@elephly.net> References: <20160816180653.22524-1-dannym@scratchpost.org> <20160816180653.22524-3-dannym@scratchpost.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:51306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baKUc-0003VA-Cz for guix-devel@gnu.org; Thu, 18 Aug 2016 06:24:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1baKUZ-0006t7-5m for guix-devel@gnu.org; Thu, 18 Aug 2016 06:24:10 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:24149) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baKUY-0006ow-TT for guix-devel@gnu.org; Thu, 18 Aug 2016 06:24:07 -0400 In-reply-to: <20160816180653.22524-3-dannym@scratchpost.org> 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" To: Danny Milosavljevic Cc: guix-devel@gnu.org Hi Danny, thanks for the patch! Although there are a couple of minor issues I think we can take it as is and make a few changes before pushing to master. Or you could send a new version of the patch if you prefer that. > * gnu/packages/fpga.scm (abc): New variable. > --- > gnu/packages/fpga.scm | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm > index 112d53b..7571f87 100644 > --- a/gnu/packages/fpga.scm > +++ b/gnu/packages/fpga.scm > @@ -38,3 +38,46 @@ > #:use-module (gnu packages version-control) > #:use-module (gnu packages libftdi)) > +;; To compile as C code (default): > +;; make sure that CC=gcc and ABC_NAMESPACE is not defined. > +;; To compile as C++ code with namespaces: > +;; make sure that CC=g++ and ABC_NAMESPACE is set to the namespace. > +;; For example, add -DABC_NAMESPACE=xxx to OPTFLAGS. What does this mean for us? Should we offer two packages? Or is this only needed for developer users of this package? I’m inclined to just drop this comment. What do you think? > +(define-public abc > + (let ((commit "5ae4b975c49c")) Others have already mentioned the cosmetic indentation issues, so I won’t do it here :) Before applying the patch we just need to make sure to run it through Emacs once more to be sure the indentation is consistent. We prefer to have the full commit here and abbreviate it in the version string. We usually also add a “revision” or “guix-revision” variable (starting at 0 or 1). That’s easier to update than having to modify the version string directly each time the commit changes. > + (package > + (name "abc") > + (version (string-append "0.0-" (string-take commit 7))) Here we normally take 9 characters of the hash. The guix-internal revision should be prefixed here. > + (source (origin > + (method url-fetch) > + (uri > + (string-append "https://bitbucket.org/alanmi/abc/get/" > + commit ".zip")) > + (file-name (string-append name "-" version > "-checkout.zip")) I don’t think we need the “-checkout” part. > + (sha256 > + (base32 > + "1syygi1x40rdryih3galr4q8yg1w5bvdzl75hd27v1xq0l5bz3d0")))) > + (build-system gnu-build-system) > + (native-inputs > + `(("unzip" ,unzip))) > + (inputs > + `(("readline" ,readline))) > + (arguments > + `(#:tests? #f ; 'check target does not exist. “'check” (with the leading quote indicating that it is a Scheme symbol) is a phase name in Guix. It would be less confusing if the comment just said “no check target” because this is about a Makefile target, not about a Guix build phase. > + #:phases > + (modify-phases %standard-phases > + (delete 'configure) > + (replace 'install > + (lambda* (#:key outputs #:allow-other-keys) > + (let* ((out (assoc-ref outputs "out")) > + (outbin (string-append out "/bin")) > + (target (string-append outbin "/abc"))) > + (mkdir-p outbin) > + (copy-file "abc" target))))))) Please end the phase with #t as “copy-file” doesn’t have a specified return value. Instead of “copy-file” you could use this: (mkdir-p outbin) (install-file "abc" outbin) “install-file” does not need to be given a target file name, just a target directory. > + (home-page "http://people.eecs.berkeley.edu/~alanmi/abc/") > + (synopsis "Sequential Logic Synthesis and Formal Verification") Let’s put this in lower case (except for the first word). Thanks again! Would you like me to take care of the changes or would you prefer to send an updated patch? ~~ Ricardo