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

* gnu/packages/fpga.scm (abc): New variable.
---
 gnu/packages/fpga.scm | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

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

> 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?

I only install the binary and no library so it doesn't really matter. We can drop the comment - if we should need the library later we can find the comment in the mailing list archives.

> 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.

Yeah, back then I didn't take care of it. Sorry. Please add.

> “'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”

Yeah.

> “install-file” does not need to be given a target file name, just a
> target directory.

Ok!

> > +    (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?

Yes please do.

Note that Leo also proposed to write

    (license (license:non-copyleft "https://fedoraproject.org/wiki/Licensing:MIT#Modern_Variants")))))

as license.

> > Thanks again!  Would you like me to take care of the changes or would
> > you prefer to send an updated patch?  
> 
> Yes please do.

Whoops. I just saw that I forgot to send yosys in the patch series. Therefore I'll send a new series with it and the fixes.

Danny Milosavljevic <dannym@scratchpost.org> writes:

>> > Thanks again!  Would you like me to take care of the changes or would
>> > you prefer to send an updated patch?  
>> 
>> Yes please do.
>
> Whoops. I just saw that I forgot to send yosys in the patch series. Therefore I'll send a new series with it and the fixes.

Oh, great.  Thank you very much for your efforts!
I hope this round of reviews wasn’t discouraging or frustrating.

~~ Ricardo

If you have time, also take a look at the Arrayfire, pocl and OpenCL stuff at

  https://github.com/genenetwork/guix-bioinformatics/tree/master/gn/packages

it is not finished, but a lot of work has gone in already.

Pj.

On Thu, Aug 18, 2016 at 05:10:12PM +0200, Danny Milosavljevic wrote:
> > > Thanks again!  Would you like me to take care of the changes or would
> > > you prefer to send an updated patch?  
> > 
> > Yes please do.
> 
> Whoops. I just saw that I forgot to send yosys in the patch series. Therefore I'll send a new series with it and the fixes.
> 

--