[bug#49494] [PATCH 0/7] Add nncp

[Arun Isaac]

Hi Sarah,

I have pushed patches 1-6 to master after implementing your suggestion
for patch 6 (klauspost-compress). I'm sending a WIP v2 of patch 7 (nncp)
in a following email. The tests are failing despite implementing your
suggestion. Any help in that regard would be much appreciated.

>> +(define-public nncp
>> +  (package
>> +    (name "nncp")
>> +    (version "7.2.0")

In patch v2, I have updated to the latest version 7.5.0.

>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     `(#:tests? #f                      ; tests fail
>
> It is not a good idea to just disable tests without knowing why they
> fail (and leaving a comment explaining why).

True, I agree.

>> +       #:modules ((guix build gnu-build-system)
>> +                  ((guix build go-build-system) #:prefix go:)
>> +                  (guix build union)
>                      ^ this module isn't necessary
>

[...]

>> +                  (guix build utils))
>> +       #:imported-modules (,@%gnu-build-system-modules
>> +                           (guix build union)
>> +                           (guix build go-build-system))
>
> This can probably just be
>   #:imported-modules ,%go-build-system-modules

Good catch! Implemented both suggestions.

>> +               (setenv "BINDIR" (string-append out "/bin"))
>> +               (setenv "INFODIR" (string-append out "/share/info"))
>> +               (setenv "DOCDIR" (string-append out "/share/doc/nncp")))
>
> Consider perhaps:
>   (setenv "DOCDIR" (string-append out "/share/doc/nncp"
>                                   ,(package-version this-package)))

I've removed the version number from DOCDIR since that's what most
packages are doing. Even the configure phase of the gnu-build-system
does not put the version number in docdir. Only the
install-license-files of the gnu-build-system puts the version number
in, and that's probably a bug.

> Does CFGPATH need to be set?

I have now set CFGPATH TO /etc/nncp.hjson.

> I took a quick look at the source and it looks like you'll also need:
>
>   (substitute* '("src/toss_test.go" "src/pipe.go")
>     (("/bin/sh") (which "sh")))
>   (substitute* "src/toss_test.go"
>     (("; cat") (string-append "; " (which "cat"))))
>
> Which also makes the tests succeed.

Good catch, but tests still don't succeed (at least on my machine).

>> +    (inputs
>> +     `(("go" ,go)))

I have moved go to native-inputs.

>> +    (native-inputs
>> +     `(("texinfo" ,texinfo)))
>> +    (propagated-inputs
>> +     `(("go-github-com-davecgh-go-xdr" ,go-github-com-davecgh-go-xdr)
>> +       ("go-github-com-dustin-go-humanize" 
>> ,go-github-com-dustin-go-humanize)
>> +       ("go-github-com-flynn-noise" ,go-github-com-flynn-noise)
>> +       ("go-github-com-gorhill-cronexpr" ,go-github-com-gorhill-cronexpr)
>> +       ("go-github-com-hjson-hjson-go" ,go-github-com-hjson-hjson-go)
>> +       ("go-github-com-klauspost-compress" 
>> ,go-github-com-klauspost-compress)
>> +       ("go-golang-org-x-crypto" ,go-golang-org-x-crypto)
>> +       ("go-golang-org-x-net" ,go-golang-org-x-net)
>> +       ("go-golang-org-x-term" ,go-golang-org-x-term)
>> +       ("go-lukechampine-com-blake3" ,go-lukechampine-com-blake3)))
>
> Since this is an end-user package, these can be regular inputs.

Done!

> I also notice that nncp can use `sendmail`; should `sendmail` be an
> input as well?

I think sendmail need not be an input. There are many sendmail
compatible implementations and we can leave it up to the user to install
one in their profile and configure nncp accordingly.

> This package is also retaining references to the Go compiler package;
> re-adding this phase from go-build-system fixes that:
>
>   (add-after 'install 'remove-go-references
>              (assoc-ref go:%standard-phases 'remove-go-references))

Done!

Thanks,
Arun

signature.asc
Description: PGP signature