[bug#28693] [PATCH] gnu: Add missing input to python-py3status.

[bug#28693] [PATCH] gnu: Add missing input to python-py3status.

[Tomáš Čech]

* gnu/packages/python.scm (python-py3status): Add missing input.

file is used to determine configuration file encoding and py3status fails to
run when this tool is missing.
---
 gnu/packages/python.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
index 6ffd468b3..88f7ff1e4 100644
--- a/gnu/packages/python.scm
+++ b/gnu/packages/python.scm
@@ -11468,6 +11468,8 @@ to occurrences in strings and comments.")
         (base32
          "0i283z1pivmir61z8kbiycigc94l61v33ygzkhczf1ifq7cppyds"))))
     (build-system python-build-system)
+    (inputs
+     `(("file" ,file)))
     (arguments
      '(#:tests? #f)) ; TODO: Requires many libraries not in Guix.
     (home-page "https://github.com/ultrabug/py3status")
-- 
2.14.2

[bug#28693] [PATCH] gnu: Add missing input to python-py3status.

[Christopher Baines]

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

On Wed,  4 Oct 2017 00:57:57 +0200
Tomáš Čech <sleep_walker@gnu.org> wrote:

> * gnu/packages/python.scm (python-py3status): Add missing input.
> 
> file is used to determine configuration file encoding and py3status
> fails to run when this tool is missing.

Hey Tomáš,

I don't know much about python-py3status, but I had a look at building
it with and without this patch, and it worked for me both times.

When building with this patch, so with file as an additional input, the
resulting store item does not reference the file store item (which you
can check using guix size, which lists the referenced store items [1]).

Given file is used at runtime, it would be good if the resulting store
item for python-py3status referenced file. As far as I can see, file is
used here [2], so you could use the substitute* procedure to replace
the command name with an absolute path to the binary in the store. This
absolute path would then mean that file is recorded as a "runtime"
dependency of python-py3status.

Other approaches include wrapping the py3status binary to ensure the
PATH environment variable includes the dependencies (like file), or
adding file as to propagated-inputs (rather than inputs), which ensures
its installed when py3status is installed.

All of these 3 approaches I've mentioned have some downsides. Patching
the files is tricky, and what is used where can change in later
releases. As for wrapping, that is inelegant, and wouldn't help if this
is used as a python library. Propagated inputs can conflict with other
packages and propagated inputs, which can cause issues for people
installing and upgrading packages.

From the number of occurrences of the subprocess module in this
package, this seems to be a problem not limited to the file tool, but a
large number of programs.

I'm imagining that you use this, so let me know what you think, and if
you have any questions?

Thanks for the patch,

Chris

1: 
→ guix size python-py3status
store item                                                       total    self
/gnu/store/ks27x0mf95gir0cdgb9h573xbava6v1k-python-3.5.3           171.5    65.2  36.7%
/gnu/store/n6nvxlk2j8ysffjh3jphn1k5silnakh6-glibc-2.25              38.5    37.1  20.9%
/gnu/store/3x53yv4v144c9xp02rs64z7j597kkqax-gcc-5.4.0-lib           68.6    30.1  16.9%
/gnu/store/42d5rjrdkln6nwvzwdc8dyd4w6iy3n5j-coreutils-8.27          86.8    14.8   8.3%
/gnu/store/m0m6bwzi8lx7kv8zbn3hjrim6flmgnf4-openssl-1.0.2l          74.6     5.9   3.3%
/gnu/store/09j7scnl3hahcmql986fsjpzj6gqsmzv-ncurses-6.0             74.3     5.7   3.2%
/gnu/store/b7y66db86k57vbb03nr4bfn9svmks4gf-bash-4.4.12             81.0     5.4   3.1%
/gnu/store/mpl9b9w60gjzsq470zka5l8p2bmy7r8v-sqlite-3.19.3           79.0     3.5   1.9%
/gnu/store/5jl21csvr3jmsa4a7d08sqa2waks3f09-gmp-6.1.2               71.2     2.6   1.5%
/gnu/store/zhrajv6qf2hzn9c3g2bb07559hyrz5xp-bash-static-4.4.12       1.4     1.4   0.8%
/gnu/store/mipizipnr4y2hly9b429j8id1albbgqi-readline-7.0            75.6     1.3   0.7%
/gnu/store/g3nari57wcfnm00kv9bnpyzdzfq4h8pk-xz-5.2.2                70.7     1.1   0.6%
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12     39.5     1.0   0.5%
/gnu/store/9ym4jxs5jhzjbf0dy5wz3ghdd8mp72r8-python-py3status-3.1   177.7     0.8   0.5%
/gnu/store/47jgabf5wx296alb06pb5iig6cqbmx0v-gdbm-1.13               69.2     0.6   0.3%
/gnu/store/5lrk6d5cjsv2s29vfji6j59gyf18rb5m-acl-2.2.52              69.3     0.4   0.2%
/gnu/store/sfx1wh27i6gsrk21p87rdyikc64v7d51-zlib-1.2.11             69.0     0.4   0.2%
/gnu/store/yz1lf55ag0904w9ib0q2kcjpdx33y7wm-attr-2.4.47             68.9     0.2   0.1%
/gnu/store/jnbb8ffxxvrw2b4z18zn0g08kqk9rsgl-libffi-3.2.1            68.8     0.1   0.1%
/gnu/store/yzlmd2nlcyjhx0ydbap7x32zjqxc3hz9-libcap-2.25             68.8     0.1   0.1%
/gnu/store/v4i3zfiwk6vl0mhdz580diqs0p6sc8vg-python-wrapper-3.5.3   176.9     0.0   0.0%
total: 177.7 MiB

2: lib/python3.5/site-packages/py3status/parse_config.py

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28693] [PATCH] gnu: Add missing input to python-py3status.

[Tomáš Čech]

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

Hi Christopher,

first, let me thank you for your long explanatory review, I really
appreciate it.


On Wed, Oct 04, 2017 at 06:59:44AM +0000, Christopher Baines wrote:
>On Wed,  4 Oct 2017 00:57:57 +0200
>Tomáš Čech <sleep_walker@gnu.org> wrote:
>
>> * gnu/packages/python.scm (python-py3status): Add missing input.
>>
>> file is used to determine configuration file encoding and py3status
>> fails to run when this tool is missing.
>
>Hey Tomáš,
>
>I don't know much about python-py3status, but I had a look at building
>it with and without this patch, and it worked for me both times.

That is correct.

>When building with this patch, so with file as an additional input, the
>resulting store item does not reference the file store item (which you
>can check using guix size, which lists the referenced store items [1]).
>
>Given file is used at runtime, it would be good if the resulting store
>item for python-py3status referenced file. As far as I can see, file is
>used here [2], so you could use the substitute* procedure to replace
>the command name with an absolute path to the binary in the store. This
>absolute path would then mean that file is recorded as a "runtime"
>dependency of python-py3status.

I have taken this approach as it seems to me as the most clean one. It
works and `guix size' command now shows the file store in the list.


>Other approaches include wrapping the py3status binary to ensure the
>PATH environment variable includes the dependencies (like file), or
>adding file as to propagated-inputs (rather than inputs), which ensures
>its installed when py3status is installed.
>
>All of these 3 approaches I've mentioned have some downsides. Patching
>the files is tricky, and what is used where can change in later
>releases.

It adds burden to maintainer to verify sufficiency and need for every
code change but besides that it looks as proper solution.

>As for wrapping, that is inelegant, and wouldn't help if this
>is used as a python library.

Agreed.

>Propagated inputs can conflict with other
>packages and propagated inputs, which can cause issues for people
>installing and upgrading packages.

This point is interesting to me - can you please give me example?

>From the number of occurrences of the subprocess module in this
>package, this seems to be a problem not limited to the file tool, but a
>large number of programs.

That is correct but other occurences are related to plugins providing
functionality. User has to prepare configuration to use that code.

Problem I'm trying to fix is causing hard-to-catch confusing error
right after start for everyone who is missing file in PATH.

I somehow miss expressing optional run-time dependencies in Guix which
would suit for this. I guess I'd go here with lazy approach and
provide basic functionality my problem and let advanced user to deal
with plugin problems by himself.

Otherwise the closure would grow beyond reason.

What do you think? Can this be the way here?


Best regards,

S_W

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

[bug#28693] [PATCH] gnu: Add missing input to python-py3status and fix call path.

[Tomáš Čech]

* gnu/packages/python.scm (python-py3status)[inputs, arguments]: Add missing
  file utility as input, adjust path used to call it.

file is used to determine configuration file encoding and py3status fails to
start when this tool is missing.
---
 gnu/packages/python.scm | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
index 6ffd468b3..426ff12eb 100644
--- a/gnu/packages/python.scm
+++ b/gnu/packages/python.scm
@@ -11468,8 +11468,21 @@ to occurrences in strings and comments.")
         (base32
          "0i283z1pivmir61z8kbiycigc94l61v33ygzkhczf1ifq7cppyds"))))
     (build-system python-build-system)
+    (inputs
+     `(("file" ,file)))
     (arguments
-     '(#:tests? #f)) ; TODO: Requires many libraries not in Guix.
+     '(#:phases
+       (modify-phases %standard-phases
+         ;; 'file' is used for detection of configuration file encoding
+         ;; let's make link the dependency to particular input
+         (add-before 'build 'patch-file-path
+           (lambda* (#:key inputs #:allow-other-keys)
+             (let ((file-path (assoc-ref inputs "file")))
+               (substitute* "py3status/parse_config.py"
+                 (("check_output\\(\\['file'")
+                  (string-append "check_output(['" file-path "/bin/file'")))
+               #t))))
+       #:tests? #f)) ; TODO: Requires many libraries not in Guix.
     (home-page "https://github.com/ultrabug/py3status")
     (synopsis "Extensible i3status wrapper written in Python")
     (description "py3status is an i3status wrapper which extends i3status
-- 
2.14.2

[bug#28693] [PATCH] gnu: Add missing input to python-py3status and fix call path.

[Marius Bakke]

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

Tomáš Čech <sleep_walker@gnu.org> writes:

> * gnu/packages/python.scm (python-py3status)[inputs, arguments]: Add missing
>   file utility as input, adjust path used to call it.
>
> file is used to determine configuration file encoding and py3status fails to
> start when this tool is missing.

LGTM, thanks!

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

[bug#28693] [PATCH] gnu: Add missing input to python-py3status.

[Christopher Baines]

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

On Thu, 5 Oct 2017 23:08:57 +0200
Tomáš Čech <sleep_walker@gnu.org> wrote:

> Hi Christopher,
> 
> first, let me thank you for your long explanatory review, I really
> appreciate it.

You're welcome :)

> On Wed, Oct 04, 2017 at 06:59:44AM +0000, Christopher Baines wrote:
> >On Wed,  4 Oct 2017 00:57:57 +0200
> >Tomáš Čech <sleep_walker@gnu.org> wrote:
> >  
> >> * gnu/packages/python.scm (python-py3status): Add missing input.
> >>
> >> file is used to determine configuration file encoding and py3status
> >> fails to run when this tool is missing.  
> >
> >Hey Tomáš,
> >
> >I don't know much about python-py3status, but I had a look at
> >building it with and without this patch, and it worked for me both
> >times.  
> 
> That is correct.
> 
> >When building with this patch, so with file as an additional input,
> >the resulting store item does not reference the file store item
> >(which you can check using guix size, which lists the referenced
> >store items [1]).
> >
> >Given file is used at runtime, it would be good if the resulting
> >store item for python-py3status referenced file. As far as I can
> >see, file is used here [2], so you could use the substitute*
> >procedure to replace the command name with an absolute path to the
> >binary in the store. This absolute path would then mean that file is
> >recorded as a "runtime" dependency of python-py3status.  
> 
> I have taken this approach as it seems to me as the most clean one. It
> works and `guix size' command now shows the file store in the list.

Great :)

> >Other approaches include wrapping the py3status binary to ensure the
> >PATH environment variable includes the dependencies (like file), or
> >adding file as to propagated-inputs (rather than inputs), which
> >ensures its installed when py3status is installed.
> >
> >All of these 3 approaches I've mentioned have some downsides.
> >Patching the files is tricky, and what is used where can change in
> >later releases.  
> 
> It adds burden to maintainer to verify sufficiency and need for every
> code change but besides that it looks as proper solution.
> 
> >As for wrapping, that is inelegant, and wouldn't help if this
> >is used as a python library.  
> 
> Agreed.
> 
> >Propagated inputs can conflict with other
> >packages and propagated inputs, which can cause issues for people
> >installing and upgrading packages.  
> 
> This point is interesting to me - can you please give me example?

So say a user has the file package installed in their profile, and also
has python-py3status installed in their profile. If python-py3status
propagates file, there is the opportunity for conflicts.

A profile directory is a union of some of the files in the component
packages. So, file from the profile has something for bin/file and
file propagated from python-py3status will also have bin/file. If you
installed the file package in your profile a while ago, and only
installed the python-py3status package recently, then there is a
possibility that they differ.

When this happens, there is an error shown, I found an example here [1].

1: https://lists.gnu.org/archive/html/guix-devel/2017-07/msg00001.html

I'm not sure how good this explanation is, so let me know if its
helpful or not.

> >From the number of occurrences of the subprocess module in this
> >package, this seems to be a problem not limited to the file tool,
> >but a large number of programs.  
> 
> That is correct but other occurences are related to plugins providing
> functionality. User has to prepare configuration to use that code.

Ok, that's good.

> Problem I'm trying to fix is causing hard-to-catch confusing error
> right after start for everyone who is missing file in PATH.
> 
> I somehow miss expressing optional run-time dependencies in Guix which
> would suit for this. I guess I'd go here with lazy approach and
> provide basic functionality my problem and let advanced user to deal
> with plugin problems by himself.
> 
> Otherwise the closure would grow beyond reason.
> 
> What do you think? Can this be the way here?

Compared to optional dependencies in other systems like Debian, they
still leave the user to sort out problems like this. I guess Guix could
offer a way of having optional inputs, but I'm not sure how that would
help. Also, multiple outputs to separate out the dependencies might be
more appropriate, but probably too much effort to do right now.

I'll reply to the patch separately.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28693] [PATCH] gnu: Add missing input to python-py3status and fix call path.

[Christopher Baines]

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

On Thu,  5 Oct 2017 23:24:39 +0200
Tomáš Čech <sleep_walker@gnu.org> wrote:

> * gnu/packages/python.scm (python-py3status)[inputs, arguments]: Add
> missing file utility as input, adjust path used to call it.

I would usually put the changelog last, as I think this is more
consistent with other committers. Also, maybe this could be more
specific like:

    * gnu/packages/python.scm (python-py3status)[inputs]: Add file.
      [arguments]: Modify phases to patch py3status to call file with an
      absolute path.

> file is used to determine configuration file encoding and py3status
> fails to start when this tool is missing.
> ---
>  gnu/packages/python.scm | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
> index 6ffd468b3..426ff12eb 100644
> --- a/gnu/packages/python.scm
> +++ b/gnu/packages/python.scm
> @@ -11468,8 +11468,21 @@ to occurrences in strings and comments.")
>          (base32
>           "0i283z1pivmir61z8kbiycigc94l61v33ygzkhczf1ifq7cppyds"))))
>      (build-system python-build-system)
> +    (inputs
> +     `(("file" ,file)))
>      (arguments
> -     '(#:tests? #f)) ; TODO: Requires many libraries not in Guix.
> +     '(#:phases
> +       (modify-phases %standard-phases
> +         ;; 'file' is used for detection of configuration file
> encoding
> +         ;; let's make link the dependency to particular input
> +         (add-before 'build 'patch-file-path
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let ((file-path (assoc-ref inputs "file")))
> +               (substitute* "py3status/parse_config.py"
> +                 (("check_output\\(\\['file'")
> +                  (string-append "check_output(['" file-path
> "/bin/file'")))
> +               #t))))
> +       #:tests? #f)) ; TODO: Requires many libraries not in Guix.
>      (home-page "https://github.com/ultrabug/py3status")
>      (synopsis "Extensible i3status wrapper written in Python")
>      (description "py3status is an i3status wrapper which extends
> i3status

This change looks fine to me, and I checked that it builds as well.
Please push :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

bug#28693: [PATCH] gnu: Add missing input to python-py3status and fix call path.

[Tomáš Čech]

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

On Fri, Oct 06, 2017 at 06:32:50AM +0000, Christopher Baines wrote:
>On Thu,  5 Oct 2017 23:24:39 +0200
>Tomáš Čech <sleep_walker@gnu.org> wrote:
>
>> * gnu/packages/python.scm (python-py3status)[inputs, arguments]: Add
>> missing file utility as input, adjust path used to call it.
>
>I would usually put the changelog last, as I think this is more
>consistent with other committers. Also, maybe this could be more
>specific like:
>
>    * gnu/packages/python.scm (python-py3status)[inputs]: Add file.
>      [arguments]: Modify phases to patch py3status to call file with an
>      absolute path.

I applied this change and pushed the commit.

Thanks for your time!

S_W

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