Re: [Patch] add keychain

[James Richardson <james@jamestechnotes.com>]

Hartmut Goebel writes:

Thank you for the feedback! Much appreciated!

> Am 22.09.2016 um 14:10 schrieb James Richardson:
>> * gnu/packages/keychain.scm: Add new file.
>
> I suggest putting this into some other file, e.g. crypto.scm or ssh.scm.
> Otherwise we have a file for a single package.

I wasn't sure of the policy. I will move it to one of these files.

>
>> * gnu/packages/keychain.scm: (keychain): New variable.
>
> If you stay with the separate file, this line is not needed.
>
>> +       #:builder (begin
>> +                   (use-modules (guix build utils))
>> +                   (let ((bin-dir (string-append %output "/bin"))
>> +                         (man1-dir (string-append %output "/share/man/man1"))
>> +                         (tar (string-append
>> +                               (assoc-ref %build-inputs "tar")
>> +                               "/bin/tar"))
>> +                         (bzip2 (assoc-ref %build-inputs "bzip2"))
> You may want to have a look at audio.scm:freepats for a bit shorter and
> more obvious code.

I will take a look.

>
>> +                     (mkdir-p bin-dir)
>> +                     (mkdir-p man1-dir)
>> +                     (setenv "PATH" (string-append bzip2 "/bin"))
>> +                     (system* tar "xfv" source)
>
> Please unpack the source first, the code is more obvious then.
>> +                     (copy-file (string-append ,name "-"
>> +                                               ,version "/keychain.1")
> If you run this using "with-directory-excursion", the code is more
> obvious and simpler.
>> +                                (string-append man1-dir "/keychain.1"))
>> +                     (copy-file (string-append ,name "-"
>> +                                               ,version "/keychain")
>> +                                (string-append bin-dir "/keychain"))))))
>
> You can use install-file and save the make-p above.
>> +    (native-inputs `(("bzip2" ,bzip2)
>> +                     ("tar" ,tar)

I wasn't aware of of install-file. I will make these updates.

>
> Please warp like this:
>
> +    (native-inputs
> +     `(("bzip2" ,bzip2)
> +      ("tar" ,tar)
>
>
>> +                     ("source" ,source)))
>
>
> No need for listing the source here.
>

I will work on the wording.

>> +    (synopsis "Key manager for OpenSSH")
>
> Not only for *open*ssh, but for other implementations, too. And für
> GnuPG-Agent, Maybe even talk abpout the Agent in the synopsis.
>
> Keychains itself says it is a "agent manager".
>
>> +    (description
>> +     "Keychain is an OpenSSH key manager, typically run from
>> +~/.bash_profile.  When keychain is run, it checks for a running ssh-agent,
>> +otherwise it starts one.  It saves the ssh-agent environment variables to
>> +~/.keychain/$HOSTNAME-sh, so that subsequent logins and
>> +non-interactive shells such as cron jobs can source the file and make
>> +passwordless ssh connections.  In addition, when keychain runs, it
>> +verifies that the key files specified on the command-line are known to
>> +ssh-agent, otherwise it loads them, prompting you for a password
>> +if necessary.")
>
>
> The text above is a details workflow description. For me the text form
> the keychain.spec-file is more meaningful. Maybe you want to combine them?
>
>     Keychain is a manager for OpenSSH, ssh.com, Sun SSH and GnuPG agents.
>     It acts as a front-end to the agents, allowing you to easily have one
>     long-running agent process per system, rather than per login session.
>     This reduces the number of times you need to enter your passphrase
>     from once per new login session to once every time your local machine
>     is rebooted.

I'll try to have these changes in the next few days. Thank you for the
review.

--
James Richardson