diff mbox

gnu: bash-minimal: Assume getcwd works correctly when cross-compiling

Message ID 7tpomxfxoe.fsf@gmail.com
State New
Headers show

Commit Message

Carlos Sánchez de La Lama Oct. 18, 2016, 4:07 p.m. UTC
* gnu/packages/bash.scm (bash-minimal): Assume getcwd works correctly
when cross compiling.
---
 gnu/packages/bash.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ludovic Courtès Oct. 19, 2016, 3:33 p.m. UTC | #1
csanchezdll@gmail.com (Carlos Sánchez de La Lama) skribis:

> * gnu/packages/bash.scm (bash-minimal): Assume getcwd works correctly
> when cross compiling.
> ---
>  gnu/packages/bash.scm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/bash.scm b/gnu/packages/bash.scm
> index f3d8517..b07367c 100644
> --- a/gnu/packages/bash.scm
> +++ b/gnu/packages/bash.scm
> @@ -247,7 +247,8 @@ without modification.")
>                   "--disable-nls"
>  
>                   ,@(if (%current-target-system)
> -                       '("bash_cv_job_control_missing=no")
> +                       '("bash_cv_job_control_missing=no"
> +			 "bash_cv_getcwd_malloc=yes")

No tabs please.  Otherwise LGTM, thanks!

Ludo’.
Ludovic Courtès Oct. 19, 2016, 7:54 p.m. UTC | #2
ludo@gnu.org (Ludovic Courtès) skribis:

> csanchezdll@gmail.com (Carlos Sánchez de La Lama) skribis:
>
>> * gnu/packages/bash.scm (bash-minimal): Assume getcwd works correctly
>> when cross compiling.
>> ---
>>  gnu/packages/bash.scm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gnu/packages/bash.scm b/gnu/packages/bash.scm
>> index f3d8517..b07367c 100644
>> --- a/gnu/packages/bash.scm
>> +++ b/gnu/packages/bash.scm
>> @@ -247,7 +247,8 @@ without modification.")
>>                   "--disable-nls"
>>  
>>                   ,@(if (%current-target-system)
>> -                       '("bash_cv_job_control_missing=no")
>> +                       '("bash_cv_job_control_missing=no"
>> +			 "bash_cv_getcwd_malloc=yes")
>
> No tabs please.  Otherwise LGTM, thanks!

I spoke too fast.  On master (Bash 4.3), this is unnecessary AFAICS:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix build bash-minimal --target=mips64el-linux-gnu --no-grafts --no-build-hook
/gnu/store/1v6l54xzmzb19hdg5bizvzfz995lh1cp-bash-minimal-4.3.42-doc
/gnu/store/gawbc9mj2i37ycym06rbybi1k4kn8zfq-bash-minimal-4.3.42-include
/gnu/store/vic4zf9gpdzrcvj8kimb9cs3049ld60d-bash-minimal-4.3.42
$ git describe
v0.11.0-1743-gfe9bdb5
--8<---------------cut here---------------end--------------->8---

Is this addressing a problem you had with Bash 4.4 (on core-updates)?

TIA,
Ludo’.
Carlos Sánchez de La Lama Oct. 19, 2016, 8:54 p.m. UTC | #3
Hi Ludo,

>>> * gnu/packages/bash.scm (bash-minimal): Assume getcwd works correctly
>>> when cross compiling.
>> No tabs please.  Otherwise LGTM, thanks!
> I spoke too fast.  On master (Bash 4.3), this is unnecessary AFAICS:
> 
> --8<---------------cut here---------------start------------->8---
> $ ./pre-inst-env guix build bash-minimal --target=mips64el-linux-gnu --no-grafts --no-build-hook
> /gnu/store/1v6l54xzmzb19hdg5bizvzfz995lh1cp-bash-minimal-4.3.42-doc
> /gnu/store/gawbc9mj2i37ycym06rbybi1k4kn8zfq-bash-minimal-4.3.42-include
> /gnu/store/vic4zf9gpdzrcvj8kimb9cs3049ld60d-bash-minimal-4.3.42
> $ git describe
> v0.11.0-1743-gfe9bdb5
> --8<---------------cut here---------------end--------------->8---

Building bash was never a problem, it worked also for me without the patch. The problem is that, when cross-compiling, bash "configure" scrip cannot test whether system getcwd works properly or not, and thus decides to use its own version of it, which does not correctly work inside bind mounts (and therefore fails in the chroot, but works outside it).

Thus, unpatched bash will fail when used to build another derivation. The problem was first reported here:

https://lists.gnu.org/archive/html/guix-devel/2013-10/msg00063.html

Although in that case Mark worked it around by having /tmp in a different partition, which incidentally causes bash-provided getcwd to work correctly.

Maybe I can put a more detailed explanation on bash.scm; I packed the patch in a hurry in case Efraim needed it for its bootstrapping also.

My changes make bash "configure" assume the system getcwd works correctly (which in our case we know it does as dependencies are controlled).

> Is this addressing a problem you had with Bash 4.4 (on core-updates)?

No, it happens with bash-4.3 in master (and probably with most other recent versions as well).

Thanks!

Carlos
Ludovic Courtès Oct. 20, 2016, 12:55 p.m. UTC | #4
Hi Carlos,

Carlos Sánchez de La Lama <csanchezdll@gmail.com> skribis:

>>>> * gnu/packages/bash.scm (bash-minimal): Assume getcwd works
>>>> correctly when cross compiling.
>>> No tabs please.  Otherwise LGTM, thanks!  I spoke too fast.  On
>> master (Bash 4.3), this is unnecessary AFAICS:
>> 
>> --8<---------------cut here---------------start------------->8--- $
>> ./pre-inst-env guix build bash-minimal --target=mips64el-linux-gnu
>> --no-grafts --no-build-hook
>> /gnu/store/1v6l54xzmzb19hdg5bizvzfz995lh1cp-bash-minimal-4.3.42-doc
>> /gnu/store/gawbc9mj2i37ycym06rbybi1k4kn8zfq-bash-minimal-4.3.42-include
>> /gnu/store/vic4zf9gpdzrcvj8kimb9cs3049ld60d-bash-minimal-4.3.42 $
>> git describe v0.11.0-1743-gfe9bdb5 --8<---------------cut
>> here---------------end--------------->8---
>
> Building bash was never a problem, it worked also for me without the
> patch. The problem is that, when cross-compiling, bash "configure"
> scrip cannot test whether system getcwd works properly or not, and
> thus decides to use its own version of it, which does not correctly
> work inside bind mounts (and therefore fails in the chroot, but works
> outside it).
>
> Thus, unpatched bash will fail when used to build another
> derivation. The problem was first reported here:
>
> https://lists.gnu.org/archive/html/guix-devel/2013-10/msg00063.html

OK, I see.

> Although in that case Mark worked it around by having /tmp in a
> different partition, which incidentally causes bash-provided getcwd to
> work correctly.
>
> Maybe I can put a more detailed explanation on bash.scm; I packed the
> patch in a hurry in case Efraim needed it for its bootstrapping also.

A detailed explanation in bash.scm would be awesome.  :-)
Could you do that?

I reread the thread and it’s not clear to me what the conclusion is.  I
don’t clearly see how Bash’s ‘getcwd’ could be wrong.  Most likely this
has to do with “..” lookup in the presence of bind mounts and
assumptions that Bash’s ‘getcwd’ makes on st_ino and st_dev values.  Oh
well, it’s probably enough to know that that ‘getcwd’ is buggy.  ;-)

Thanks,
Ludo’.
diff mbox

Patch

diff --git a/gnu/packages/bash.scm b/gnu/packages/bash.scm
index f3d8517..b07367c 100644
--- a/gnu/packages/bash.scm
+++ b/gnu/packages/bash.scm
@@ -247,7 +247,8 @@  without modification.")
                  "--disable-nls"
 
                  ,@(if (%current-target-system)
-                       '("bash_cv_job_control_missing=no")
+                       '("bash_cv_job_control_missing=no"
+			 "bash_cv_getcwd_malloc=yes")
                        '()))))))))
 
 (define-public static-bash