[bug#28660] [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[bug#28660] [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[Thomas Danckaert]

[-- Attachment #1: Type: Text/Plain, Size: 494 bytes --]

Hi Guix,

I slightly botched the last numpy update: setting $SHELL works, but 
then we have to set $SHELL for every package which uses numpy's 
distutils.  I don't know how many packages there are, but it includes 
scipy and python-hdf (and therefore those builds are currently broken 
on master...).

The attached patch should fix the issue for all packages, by setting 
the default shell to the “/bin/sh” of the build environment's bash 
package (i.e. bash-minimal).

Thomas

[-- Attachment #2: 0001-gnu-python-numpy-Give-sh-store-location-instead-of-s.patch --]
[-- Type: Text/X-Patch, Size: 1842 bytes --]

From cfaebf1b09c71585b5513629005e7cf3c5d17508 Mon Sep 17 00:00:00 2001
From: Thomas Danckaert <thomas.danckaert@gmail.com>
Date: Sun, 1 Oct 2017 14:32:04 +0200
Subject: [PATCH] gnu: python-numpy: Give sh store location instead of setting
 $SHELL.

* gnu/packages/python.scm (python-numpy): [arguments] Don't set $SHELL in the
  environment, but embed the location of bash as a default shell.  Otherwise,
  we have to set $SHELL for every package which uses numpy's distutils.
---
 gnu/packages/python.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
index e95c22de1..267715b0f 100644
--- a/gnu/packages/python.scm
+++ b/gnu/packages/python.scm
@@ -3742,8 +3742,6 @@ between language specification and implementation aspects.")
        (modify-phases %standard-phases
         (add-before 'build 'set-environment-variables
          (lambda* (#:key inputs #:allow-other-keys)
-           ;; numpy's distutils uses $SHELL to run external commands.
-          (setenv "SHELL" "bash")
           (call-with-output-file "site.cfg"
             (lambda (port)
               (format port
@@ -3762,6 +3760,10 @@ include_dirs = ~a/include
                       (assoc-ref inputs "openblas")
                       (assoc-ref inputs "lapack")
                       (assoc-ref inputs "lapack"))))
+          ;; Insert bash store location for default shell /bin/sh.
+          (substitute* "numpy/distutils/exec_command.py"
+            (("(os.environ.get\\('SHELL', ')(/bin/sh'\\))" match match-start match-end)
+            (string-append match-start (assoc-ref inputs "bash") match-end)))
           ;; Use "gcc" executable, not "cc".
           (substitute* "numpy/distutils/system_info.py"
             (("c = distutils\\.ccompiler\\.new_compiler\\(\\)")
-- 
2.14.1

[bug#28660] [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[Christopher Baines]

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

On Sun, 01 Oct 2017 14:44:04 +0200 (CEST)
Thomas Danckaert <post@thomasdanckaert.be> wrote:

> Hi Guix,
> 
> I slightly botched the last numpy update: setting $SHELL works, but 
> then we have to set $SHELL for every package which uses numpy's 
> distutils.  I don't know how many packages there are, but it includes 
> scipy and python-hdf (and therefore those builds are currently broken 
> on master...).
> 
> The attached patch should fix the issue for all packages, by setting 
> the default shell to the “/bin/sh” of the build environment's bash 
> package (i.e. bash-minimal).

Could bash be specified as an input, and that used instead? This would
mean that the behaviour of the package is more isolated from the
environment.

For example, on non GuixSD systems, /bin/sh could be something other
than bash. But, if the numpy package depends on some bash in the store,
and uses that, then it would be isolated from this.

What do you think?

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

[bug#28660] [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[Thomas Danckaert]

From: Christopher Baines <mail@cbaines.net>
Subject: Re: [bug#28660] [PATCH] gnu: python-numpy: Give sh store 
location instead of setting $SHELL.
Date: Wed, 4 Oct 2017 07:30:48 +0100

> Could bash be specified as an input, and that used instead? This 
> would
> mean that the behaviour of the package is more isolated from the
> environment.

Sounds good, but I'm afraid I don't understand what you mean (maybe 
it's still too early in the morning).  If we add a "bash" to the 
package inputs, it will still come from the store.  So probably you 
mean something else by “specified as an input”?

> For example, on non GuixSD systems, /bin/sh could be something other
> than bash. But, if the numpy package depends on some bash in the 
> store,
> and uses that, then it would be isolated from this.

That's true, though users on such systems could set $SHELL to 
override the default one from the store.  But maybe avoiding having 
to do that is what you mean by “more isolated from the environment”?

The shell is used for very limited purposes anyway (essentially to 
run C and Fortran compilers to build extenstions, AFAIU), but I'm 
happy to improve the patch if it's useful.  If you have a solution 
that allows users to use their own /bin/sh if they want, but still 
works for all builds of dependent packages on Guix, please explain :-)

Thomas

[bug#28660] [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[Christopher Baines]

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

On Wed, 04 Oct 2017 09:04:36 +0200 (CEST)
Thomas Danckaert <post@thomasdanckaert.be> wrote:

> From: Christopher Baines <mail@cbaines.net>
> Subject: Re: [bug#28660] [PATCH] gnu: python-numpy: Give sh store 
> location instead of setting $SHELL.
> Date: Wed, 4 Oct 2017 07:30:48 +0100
> 
> > Could bash be specified as an input, and that used instead? This 
> > would
> > mean that the behaviour of the package is more isolated from the
> > environment.  
> 
> Sounds good, but I'm afraid I don't understand what you mean (maybe 
> it's still too early in the morning).  If we add a "bash" to the 
> package inputs, it will still come from the store.  So probably you 
> mean something else by “specified as an input”?
> 
> > For example, on non GuixSD systems, /bin/sh could be something other
> > than bash. But, if the numpy package depends on some bash in the 
> > store,
> > and uses that, then it would be isolated from this.  
> 
> That's true, though users on such systems could set $SHELL to 
> override the default one from the store.  But maybe avoiding having 
> to do that is what you mean by “more isolated from the environment”?
> 
> The shell is used for very limited purposes anyway (essentially to 
> run C and Fortran compilers to build extenstions, AFAIU), but I'm 
> happy to improve the patch if it's useful.  If you have a solution 
> that allows users to use their own /bin/sh if they want, but still 
> works for all builds of dependent packages on Guix, please explain :-)

Sorry Thomas, ignore what I originally said. I thought from reading the
patch that this was patching numpy to use /bin/sh . Now after building
it and looking at the resulting files, I can see that it makes the
default use bash from the store [1]. Which was exactly what I was
suggesting, but you were already doing that.

With this new, hopefully more correct interpretation, this patch looks
fine to me :)

1: 
sh = os.environ.get('SHELL', '/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/sh')

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

bug#28660: [PATCH] gnu: python-numpy: Give sh store location instead of setting $SHELL.

[Thomas Danckaert]

From: Christopher Baines <mail@cbaines.net>
Subject: Re: [bug#28660] [PATCH] gnu: python-numpy: Give sh store 
location instead of setting $SHELL.
Date: Wed, 4 Oct 2017 08:49:44 +0100

> Sorry Thomas, ignore what I originally said. I thought from reading 
> the
> patch that this was patching numpy to use /bin/sh . Now after 
> building
> it and looking at the resulting files, I can see that it makes the
> default use bash from the store. Which was exactly what I was
> suggesting, but you were already doing that.
>
> With this new, hopefully more correct interpretation, this patch 
> looks
> fine to me :)

Yes, that's the purpose exactly :)  I've slightly reworded the commit 
message & comment to hopefully make it less confusing, and pushed.

Thanks for taking a look!

Thomas