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

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

[Morgan.J.Smith]

From: Morgan Smith <Morgan.J.Smith@outlook.com>

* gnu/packages/admin.scm (opendoas): New variable.
---
 gnu/packages/admin.scm | 51 ++++++++++++++++++++++++++++++++++++++++++
 gnu/system.scm         |  1 +
 2 files changed, 52 insertions(+)

diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
index b0a43d9644..594ec62c1d 100644
--- a/gnu/packages/admin.scm
+++ b/gnu/packages/admin.scm
@@ -1389,6 +1389,57 @@ commands and their arguments.")
     ;; See <http://www.sudo.ws/sudo/license.html>.
     (license license:x11)))
 
+(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))
+                      (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
+                     (replace 'configure
+                       (lambda* (#:key configure-flags #:allow-other-keys)
+                         ;; The configure script can only be told which
+                         ;; compiler to use through environment variables
+                         (setenv "CC" ,compiler)
+                         (apply invoke "./configure" configure-flags)))
+                     (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.
+                         (substitute* "bsd.prog.mk"
+                           (("^\tchown.*$") "")))))
+                   #:configure-flags (list (string-append "--prefix=" %output)
+                                           (string-append "--target=" (or ,target ""))
+                                           "--with-timestamp")
+                   ;; Compiler choice is not carried over from the configure script
+                   #:make-flags (list (string-append "CC=" ,compiler))
+                   ;; There are no tests provided
+                   #:tests? #f)))
+    (native-inputs `(("bison" ,bison)))
+    (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.")
+    (license license:isc)))
+
 (define-public wpa-supplicant-minimal
   (package
     (name "wpa-supplicant-minimal")
diff --git a/gnu/system.scm b/gnu/system.scm
index d929187695..d5fd0979a1 100644
--- 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
-- 
2.26.2

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

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 5173 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

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

[Morgan Smith]

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

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

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

Morgan,

Morgan Smith 写道：
> 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.

Don't give up!

> 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!

Well, Marius stole my thunder with your other patch, but welcome 
aboard!

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]