[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: Add bioawk.
From: |
Roel Janssen |
Subject: |
Re: [PATCH] gnu: Add bioawk. |
Date: |
Fri, 11 Mar 2016 00:00:11 +0100 |
User-agent: |
mu4e 0.9.17; emacs 25.1.50.2 |
0001-gnu-Add-bioawk.patch
Description: Text Data
Dear Leo and Ricardo,
Thank you for reviewing this patch. I hope it is fine now.
Leo Famulari writes:
> On Tue, Mar 08, 2016 at 09:10:14PM +0100, Roel Janssen wrote:
>> From 990eda92a62dc25d0f5792437a82e119c5e3579f Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <address@hidden>
>> Date: Tue, 8 Mar 2016 21:06:53 +0100
>> Subject: [PATCH] gnu: Add bioawk.
>>
>> * gnu/packages/bioinformatics.scm (bioawk): New variable.
>
> Thanks for the patch!
>
> [...]
>
>> + (propagated-inputs
>> + `(("zlib" ,zlib)))
>
> I changed this to a plain input, and then checked the references of the
> resulting build, and zlib is referenced. Considering that, does it need
> to be propagated into the user's profile? Propagated inputs should be
> avoided if possible.
You're right. I simply get confused about inputs, native-inputs and
propagated-inputs.
I made it an input in my new patch.
>> + (native-inputs
>> + `(("bison" ,bison)))
>> + (arguments
>> + `(#:parallel-build? #f
>> + #:phases
>> + (modify-phases %standard-phases
>> + (delete 'configure)
>> + (delete 'check)
>
> Can you say why tests are disabled? It can be as simple as "no test
> suite" if that is accurate.
Sure.
>> + (replace
>> + 'install
>> + (lambda* (#:key outputs #:allow-other-keys)
>> + (let ((bin (string-append (assoc-ref outputs "out") "/bin")))
>> + (install-file "bioawk" bin)))))))
>
> The git repo includes a manpage 'awk.1'. Can you send an updated patch
> that installs that as well?
I added it as bioawk.1 instead, to avoid confusion with awk.
Good catch!
Ricardo Wurmus writes:
> Roel Janssen <address@hidden> writes:
>
>> Please let me know when something is wrong with the patch.
>
> In addition to Leo’s comments here are mine:
>
>> + (arguments
>> + `(#:parallel-build? #f
>
> Why is parallel-build disabled? Could you add a comment?
Sure.
>> + #:phases
>> + (modify-phases %standard-phases
>> + (delete 'configure)
>> + (delete 'check)
>
> We usually just do “#:tests? #f” with a comment, instead of deleting the
> “check” phase.
Sorry for the redundancy. I submitted two patches before receiving
comments about this. Fixed in my new patch, and I'll apply this in
future patches as well.
>> + (replace
>> + 'install
>
> Please put “'install” on the same line as “(replace”.
Should I do this for other (replace ...) as well? If I want to replace
the build phase, should I put it on the same line as well?
>> + (lambda* (#:key outputs #:allow-other-keys)
>> + (let ((bin (string-append (assoc-ref outputs "out") "/bin")))
>> + (install-file "bioawk" bin)))))))
>> + (home-page "https://github.com/lh3/bioawk")
>> + (synopsis "AWK with bioinformatics extensions")
>> + (description "Bioawk is an extension to Brian Kernighan's awk, adding
>> the
>> +support of several common biological data formats, including optionally
>> gzip'ed
>> +BED, GFF, SAM, VCF, FASTA/Q and TAB-delimited formats with column names. It
>> +also adds a few built-in functions and an command line option to use TAB as
>> the
>
> “a command-line option”, not “an”
Good catch! Fixed in my new patch.
Kind regards,
Roel Janssen