Re: [PATCH v2 2/6] gnu: fpga: Add abc.

[Ricardo Wurmus]

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