[PATCH] gnu: tailon: Add missing inputs.

Done

Details

2 participants

Christopher Baines
Marius Bakke

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Debbugs page

Christopher Baines wrote 8 years ago

Recipients:(address . guix-patches@gnu.org)

Message-ID:20170504064715.9779-1-mail@cbaines.net

* gnu/packages/logging.scm (tailon)[inputs]: Add grep, gawk, sed and coreutils
  as inputs.
  [arguments]: Wrap bin/tailon to include some inputs in the PATH.
---
 gnu/packages/logging.scm | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Toggle diff (42 lines)diff --git a/gnu/packages/logging.scm b/gnu/packages/logging.scm
index 2523d65f6..060521eab 100644
--- a/gnu/packages/logging.scm
+++ b/gnu/packages/logging.scm
@@ -26,6 +26,8 @@
   #:use-module (guix build-system gnu)
   #:use-module (guix build-system python)
   #:use-module (gnu packages)
+  #:use-module (gnu packages base)
+  #:use-module (gnu packages gawk)
   #:use-module (gnu packages perl)
   #:use-module (gnu packages python)
   #:use-module (gnu packages autotools))
@@ -108,7 +110,25 @@ command line.")
     (inputs
      `(("python-pyyaml" ,python-pyyaml)
        ("python-sockjs-tornado" ,python-sockjs-tornado)
-       ("python-tornado" ,python-tornado)))
+       ("python-tornado" ,python-tornado)
+       ("grep" ,grep)
+       ("gawk" ,gawk)
+       ("sed" ,sed)
+       ("tail" ,coreutils)))
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (add-after 'install 'wrap-tailon-path
+                    (lambda* (#:key inputs outputs #:allow-other-keys)
+                      (let ((out (assoc-ref outputs "out")))
+                        (wrap-program (string-append out "/bin/tailon")
+                          `("PATH" ":" prefix ,(map
+                                                (lambda (input)
+                                                  (string-append
+                                                   (assoc-ref inputs input)
+                                                   "/bin"))
+                                                '("grep" "gawk" "sed" "tail"))))
+                        #t))))))
     (home-page "https://tailon.readthedocs.io/")
     (synopsis
      "Webapp for looking at and searching through log files")
-- 
2.12.0

Marius Bakke wrote 8 years ago

Recipients:

Message-ID:877f1v1cwq.fsf@fastmail.com

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (4 lines)

> * gnu/packages/logging.scm (tailon)[inputs]: Add grep, gawk, sed and coreutils

> as inputs.

> [arguments]: Wrap bin/tailon to include some inputs in the PATH.

Generally it's better to fully qualify the paths to these programs in

the code. Is that difficult here?

I think all of these inputs are already available, so you can do e.g.:

(substitute "foo"

(("'grep'") (which "grep))

(("'gawk'") (which "gawk")))

...instead of referencing the inputs explicitly.

Christopher Baines wrote 8 years ago

[PATCH] gnu: tailon: Use absolute paths for commands.

Recipients:(address . 26770@debbugs.gnu.org)

Message-ID:20170515061155.1380-1-mail@cbaines.net

* gnu/packages/logging.scm (tailon)[arguments]: Patch commands.py to reference
  grep, awk, sed and tail by absolute paths.
---
 gnu/packages/logging.scm | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Toggle diff (29 lines)diff --git a/gnu/packages/logging.scm b/gnu/packages/logging.scm
index 2523d65f6..11bbfef52 100644
--- a/gnu/packages/logging.scm
+++ b/gnu/packages/logging.scm
@@ -109,6 +109,22 @@ command line.")
      `(("python-pyyaml" ,python-pyyaml)
        ("python-sockjs-tornado" ,python-sockjs-tornado)
        ("python-tornado" ,python-tornado)))
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (add-after 'install 'wrap-tailon-path
+                    (lambda* (#:key inputs outputs #:allow-other-keys)
+                      (let ((out (assoc-ref outputs "out")))
+                        (substitute* (find-files out "commands.py")
+                          (("self\\.first_in_path\\('grep'\\)")
+                           (string-append"'" (which "grep") "'"))
+                          (("self\\.first_in_path\\('gawk', 'awk'\\)")
+                           (string-append"'" (which "gawk") "'"))
+                          (("self\\.first_in_path\\('gsed', 'sed'\\)")
+                           (string-append"'" (which "sed") "'"))
+                          (("self\\.first_in_path\\('gtail', 'tail'\\)")
+                           (string-append"'" (which "tail") "'")))
+                        #t))))))
     (home-page "https://tailon.readthedocs.io/")
     (synopsis
      "Webapp for looking at and searching through log files")
-- 
2.12.0

Christopher Baines wrote 8 years ago

Re: bug#26770: [PATCH] gnu: tailon: Add missing inputs.

Recipients:(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 26770@debbugs.gnu.org)

Message-ID:20170515072549.5c933ddc@cbaines.net

On Fri, 05 May 2017 18:33:41 +0200

Marius Bakke <mbakke@fastmail.com> wrote:

Toggle quote (9 lines)

> Christopher Baines <mail@cbaines.net> writes:

> > * gnu/packages/logging.scm (tailon)[inputs]: Add grep, gawk, sed

> > and coreutils as inputs.

> > [arguments]: Wrap bin/tailon to include some inputs in the PATH.

> Generally it's better to fully qualify the paths to these programs in

> the code. Is that difficult here?

Good point, I've send an updated patch that does this.

Toggle quote (7 lines)

> I think all of these inputs are already available, so you can do e.g.:

> (substitute "foo"

> (("'grep'") (which "grep))

> (("'gawk'") (which "gawk")))

> ...instead of referencing the inputs explicitly.

That works, I've changed that in the latest patch also.

Thanks for your review :)

Marius Bakke wrote 8 years ago

Re: bug#26770: [PATCH] gnu: tailon: Use absolute paths for commands.

Recipients:

Message-ID:87o9uuf6vl.fsf@fastmail.com

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (3 lines)

> * gnu/packages/logging.scm (tailon)[arguments]: Patch commands.py to reference

> grep, awk, sed and tail by absolute paths.

Thanks for this!

[...]

Toggle quote (14 lines)> +         (add-after 'install 'wrap-tailon-path
> +                    (lambda* (#:key inputs outputs #:allow-other-keys)
> +                      (let ((out (assoc-ref outputs "out")))
> +                        (substitute* (find-files out "commands.py")
> +                          (("self\\.first_in_path\\('grep'\\)")
> +                           (string-append"'" (which "grep") "'"))
> +                          (("self\\.first_in_path\\('gawk', 'awk'\\)")
> +                           (string-append"'" (which "gawk") "'"))
> +                          (("self\\.first_in_path\\('gsed', 'sed'\\)")
> +                           (string-append"'" (which "sed") "'"))
> +                          (("self\\.first_in_path\\('gtail', 'tail'\\)")
> +                           (string-append"'" (which "tail") "'")))
> +                        #t))))))

Is there any particular reason this phase runs after 'install'? I think
we should try to avoid modifying files after they have been copied to
the store, but if doing this substitution earlier is difficult I guess
it's okay with a comment.

Christopher Baines wrote 8 years ago

[PATCH] gnu: tailon: Use absolute paths for commands.

Recipients:(address . 26770@debbugs.gnu.org)

Message-ID:20170516194001.2591-1-mail@cbaines.net

* gnu/packages/logging.scm (tailon)[arguments]: Patch commands.py to reference
  grep, awk, sed and tail by absolute paths.
---
 gnu/packages/logging.scm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Toggle diff (28 lines)diff --git a/gnu/packages/logging.scm b/gnu/packages/logging.scm
index 2523d65f6..203279f33 100644
--- a/gnu/packages/logging.scm
+++ b/gnu/packages/logging.scm
@@ -109,6 +109,21 @@ command line.")
      `(("python-pyyaml" ,python-pyyaml)
        ("python-sockjs-tornado" ,python-sockjs-tornado)
        ("python-tornado" ,python-tornado)))
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'patch-commands.py
+                     (lambda args
+                       (substitute* (find-files "." "commands\\.py")
+                         (("self\\.first_in_path\\('grep'\\)")
+                          (string-append"'" (which "grep") "'"))
+                         (("self\\.first_in_path\\('gawk', 'awk'\\)")
+                          (string-append"'" (which "gawk") "'"))
+                         (("self\\.first_in_path\\('gsed', 'sed'\\)")
+                          (string-append"'" (which "sed") "'"))
+                         (("self\\.first_in_path\\('gtail', 'tail'\\)")
+                          (string-append"'" (which "tail") "'")))
+                       #t)))))
     (home-page "https://tailon.readthedocs.io/")
     (synopsis
      "Webapp for looking at and searching through log files")
-- 
2.12.0

Christopher Baines wrote 8 years ago

Recipients:(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 26770@debbugs.gnu.org)

Message-ID:20170516204928.4edd18b0@cbaines.net

On Mon, 15 May 2017 17:59:42 +0200

Marius Bakke <mbakke@fastmail.com> wrote:

Toggle quote (24 lines)> Christopher Baines <mail@cbaines.net> writes:
> > +         (add-after 'install 'wrap-tailon-path
> > +                    (lambda* (#:key inputs outputs
> > #:allow-other-keys)
> > +                      (let ((out (assoc-ref outputs "out")))
> > +                        (substitute* (find-files out "commands.py")
> > +                          (("self\\.first_in_path\\('grep'\\)")
> > +                           (string-append"'" (which "grep") "'"))
> > +                          (("self\\.first_in_path\\('gawk',
> > 'awk'\\)")
> > +                           (string-append"'" (which "gawk") "'"))
> > +                          (("self\\.first_in_path\\('gsed',
> > 'sed'\\)")
> > +                           (string-append"'" (which "sed") "'"))
> > +                          (("self\\.first_in_path\\('gtail',
> > 'tail'\\)")
> > +                           (string-append"'" (which "tail") "'")))
> > +                        #t))))))  
> 
> Is there any particular reason this phase runs after 'install'? I
> think we should try to avoid modifying files after they have been
> copied to the store, but if doing this substitution earlier is
> difficult I guess it's okay with a comment.

No, I just put it there by default, but I can see why doing the
substitution earlier would be better. I've sent another patch that
moves it to after the unpack phase.

Marius Bakke wrote 8 years ago

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 26770-done@debbugs.gnu.org)

Message-ID:87o9urcz5y.fsf@fastmail.com

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (31 lines)> On Mon, 15 May 2017 17:59:42 +0200
> Marius Bakke <mbakke@fastmail.com> wrote:
>
>> Christopher Baines <mail@cbaines.net> writes:
>> > +         (add-after 'install 'wrap-tailon-path
>> > +                    (lambda* (#:key inputs outputs
>> > #:allow-other-keys)
>> > +                      (let ((out (assoc-ref outputs "out")))
>> > +                        (substitute* (find-files out "commands.py")
>> > +                          (("self\\.first_in_path\\('grep'\\)")
>> > +                           (string-append"'" (which "grep") "'"))
>> > +                          (("self\\.first_in_path\\('gawk',
>> > 'awk'\\)")
>> > +                           (string-append"'" (which "gawk") "'"))
>> > +                          (("self\\.first_in_path\\('gsed',
>> > 'sed'\\)")
>> > +                           (string-append"'" (which "sed") "'"))
>> > +                          (("self\\.first_in_path\\('gtail',
>> > 'tail'\\)")
>> > +                           (string-append"'" (which "tail") "'")))
>> > +                        #t))))))  
>> 
>> Is there any particular reason this phase runs after 'install'? I
>> think we should try to avoid modifying files after they have been
>> copied to the store, but if doing this substitution earlier is
>> difficult I guess it's okay with a comment.
>
> No, I just put it there by default, but I can see why doing the
> substitution earlier would be better. I've sent another patch that
> moves it to after the unpack phase. 

Applied, thank you! My rationale was that the store may be on slow
storage (say NFS), whereas the build directory is probably a tmpfs
or local storage.

Note: I replaced the "find-files" invocation with just the one file
path. Hope that was okay!

Closed

Christopher Baines wrote 8 years ago

Recipients:(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 26770-done@debbugs.gnu.org)

Message-ID:bd3f0095-de16-60c7-c649-4d4b89a9ad9f@cbaines.net

On 17/05/17 15:53, Marius Bakke wrote:

Toggle quote (40 lines)> Christopher Baines <mail@cbaines.net> writes:
> 
>> On Mon, 15 May 2017 17:59:42 +0200
>> Marius Bakke <mbakke@fastmail.com> wrote:
>>
>>> Christopher Baines <mail@cbaines.net> writes:
>>>> +         (add-after 'install 'wrap-tailon-path
>>>> +                    (lambda* (#:key inputs outputs
>>>> #:allow-other-keys)
>>>> +                      (let ((out (assoc-ref outputs "out")))
>>>> +                        (substitute* (find-files out "commands.py")
>>>> +                          (("self\\.first_in_path\\('grep'\\)")
>>>> +                           (string-append"'" (which "grep") "'"))
>>>> +                          (("self\\.first_in_path\\('gawk',
>>>> 'awk'\\)")
>>>> +                           (string-append"'" (which "gawk") "'"))
>>>> +                          (("self\\.first_in_path\\('gsed',
>>>> 'sed'\\)")
>>>> +                           (string-append"'" (which "sed") "'"))
>>>> +                          (("self\\.first_in_path\\('gtail',
>>>> 'tail'\\)")
>>>> +                           (string-append"'" (which "tail") "'")))
>>>> +                        #t))))))  
>>>
>>> Is there any particular reason this phase runs after 'install'? I
>>> think we should try to avoid modifying files after they have been
>>> copied to the store, but if doing this substitution earlier is
>>> difficult I guess it's okay with a comment.
>>
>> No, I just put it there by default, but I can see why doing the
>> substitution earlier would be better. I've sent another patch that
>> moves it to after the unpack phase. 
> 
> Applied, thank you! My rationale was that the store may be on slow
> storage (say NFS), whereas the build directory is probably a tmpfs
> or local storage.
> 
> Note: I replaced the "find-files" invocation with just the one file
> path. Hope that was okay!

Yep, looks good. Thanks for your review :)

Attachment: signature.asc

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 26770@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 26770

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date