[bug#41578] [PATCH] gnu: Add opendoas.

[Morgan Smith <Morgan.J.Smith@outlook.com>]

Thanks so much for the review Tobias!

I went back to figure out exactly what --target did in the configure
script. Now there is a bunch of logic that doesn't go anywhere so at
first glance it doesn't look like this, but I'm fairly certain that all
it does is check if it's Linux. If it is Linux, than it will add some
cflags in the configure script. These flags never make their way out of
the configure script. Honestly, the configure script is so bad I'm
tempted to make a pull request.

I'm a little sad I didn't get torn apart more for technical reasons. I
learned that phases must end in #t and a few style points. Maybe next
time I'll throw in some terrible mistakes.

Also I realize that I totally forgot to put the copyright notice in!
Could you throw
";;; Copyright © 2020 Morgan Smith <Morgan.J.Smith@outlook.com>"
in there somewhere? I'm excited to get my name in this project!


Thanks,

Morgan


On 2020-05-28 13:57, Tobias Geerinckx-Rice wrote:
> Morgan,
> 
> Morgan.J.Smith@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