GNU bug report logs - #41578
[PATCH] gnu: Add opendoas.

Previous Next

Full log

View this message in rfc822 format

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Morgan.J.Smith <at> outlook.com
Cc: 41578 <at> debbugs.gnu.org
Subject: [bug#41578] [PATCH] gnu: Add opendoas.
Date: Thu, 28 May 2020 19:57:03 +0200

[Message part 1 (text/plain, inline)]

Morgan,

Morgan.J.Smith <at> outlook.com 写道：
> * gnu/packages/admin.scm (opendoas): New variable.

Thank you!  It looks good to me.  I've queued it locally with the 
(minor) changes below, but will wait a few days for 
<https://issues.guix.gnu.org/41579> to land if that's all right 
with you.  I also need to test it as a proper setuid programme.

> +(define-public opendoas
> +  (package
> +    (name "opendoas")
> +    (version "6.6.1")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url 
> "https://github.com/Duncaen/OpenDoas.git")
> +                    (commit (string-append "v" version))))
> +              (file-name (git-file-name name version))
> +              (sha256
> +               (base32
> + 
> "07kkc5729p654jrgfsc8zyhiwicgmq38yacmwfvay2b3gmy728zn"))))
> +    (build-system gnu-build-system)
> +    (arguments (let* ((target (%current-target-system))

I've added a newline after ‘arguments’ to give your phases (and 
helpful comments) some room to breathe and keep lines from 
exceeding 80 characters.  It's mainly a matter of preference. 
Since I'm reviewing this so you're stuck with my preference.

> +                      (compiler (if target
> +                                    (string-append target 
> "-gcc")
> +                                    "gcc")))
> +                 `(#:phases
> +                   (modify-phases %standard-phases
> +                     ;; We replace the configure phase in order 
> to remove all the
> +                     ;; default flags. The configure script 
> doesn't accept most
> +                     ;; of the default flags

I shortened this to the last sentence and added a full stop here…

> +                     (replace 'configure
> +                       (lambda* (#:key configure-flags 
> #:allow-other-keys)
> +                         ;; The configure script can only be 
> told which
> +                         ;; compiler to use through environment 
> variables

…and here.  ;;-style comments are full sentences, unlike ;-ones.

> +                     (add-before 'install 'fix-makefile
> +                       (lambda* (#:key outputs 
> #:allow-other-keys)
> +                         ;; We can't chown to root as the 
> chroot doesn't have
> +                         ;; this user. Also the store is owned 
> by root so this
> +                         ;; isn't necessary.

All true, but so common a change in Guix that it's not worth a 
comment.

> +                         (substitute* "bsd.prog.mk"
> +                           (("^\tchown.*$") "")))))

Phases need to end in truth so I've added a #t here.  We get away 
without one in the previous phase because INVOKE itself is 
guaranteed to return #t.

> +                   #:configure-flags (list (string-append 
> "--prefix=" %output)
> +                                           (string-append 
> "--target=" (or ,target ""))

It didn't look to me like this was used for anything, and quoting 
Morgan on IRC:

<butterypancake> ya, the configure script really doesn't do a damn 
thing with target. But in the future it might save someone some 
time.

> +                   ;; Compiler choice is not carried over from 
> the configure script.
> +                   #:make-flags (list (string-append "CC=" 
> ,compiler))

I agree that it's nice to save future maintainers the trouble of 
retracing your steps but don't like the idea of sleeper code. 
I'll keep them as comments.

> +                   ;; There are no tests provided
> +                   #:tests? #f)))

Changed to the equivalent but more conventional

        #:tests? #f)))                 ; no test suite

> +    (native-inputs `(("bison" ,bison)))

Added a trivial newline before `.

> +    (home-page "https://github.com/Duncaen/OpenDoas")
> +    (synopsis "Portable version of OpenBSD's doas command")
> +    (description "Doas is a minimal replacement for the 
> venerable sudo.  It was
> +initially written by Ted Unangst of the OpenBSD project to 
> provide 95% of the
> +features of sudo with a fraction of the codebase.")

Thanks for including a multi-line description!  Won't stop me from 
trying to expand it some more.

> +    (license license:isc)))

Not surprisingly, libbsd/ is under a 3-clause BSD licence.  I 
added it.

> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -896,6 +896,7 @@ use 'plain-file' instead~%")
>            (file-append inetutils "/bin/ping6")
>            (file-append sudo "/bin/sudo")
>            (file-append sudo "/bin/sudoedit")
> +          (file-append opendoas "/bin/doas")
>            (file-append fuse "/bin/fusermount")
>  
>            ;; To allow mounts with the "user" option, "mount" 
>            and "umount" must

This would be a separate patch.  However, this would install doas 
on almost all systems.  I think the default list should contain 
only the minimal defaults, and I don't see doas being a must-have 
%base-package any time soon.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.