diff mbox series

[bug#59073] gnu: r-minimal: Avoid referencing /gnu/store/[^-]+-glibc-[^-]+-static.

Message ID 20221106090558.24507-1-mail@cbaines.net
State New
Headers show
Series [bug#59073] gnu: r-minimal: Avoid referencing /gnu/store/[^-]+-glibc-[^-]+-static. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git-branch success View Git branch
cbaines/applying patch success
cbaines/issue success View issue

Commit Message

Christopher Baines Nov. 6, 2022, 9:05 a.m. UTC
As this causes the following error when computing a cross-compilation
derivation for r-minimal and related packages.

I think this is coming from the string appearing in the sources section of the
derivation, but not being a valid store path.

* gnu/packages/statistics.scm (r-minimal)[arguments]: Split string involving
/gnu/store.
---
 gnu/packages/statistics.scm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Simon Tournier Nov. 7, 2022, 9:27 a.m. UTC | #1
Hi,

On dim., 06 nov. 2022 at 10:05, Christopher Baines <mail@cbaines.net> wrote:

> * gnu/packages/statistics.scm (r-minimal)[arguments]: Split string involving
> /gnu/store.

It would imply rebuild many packages, no?  master, staging or
core-updates?

> ---
>  gnu/packages/statistics.scm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/statistics.scm b/gnu/packages/statistics.scm
> index 2d4dbe4a31..43988ad00e 100644
> --- a/gnu/packages/statistics.scm
> +++ b/gnu/packages/statistics.scm
> @@ -470,7 +470,11 @@ (define-public r-minimal
>                                      "util-macros"
>                                      "graphite2"))
>                            "|"
> -                          "/gnu/store/[^-]+-glibc-[^-]+-static"
> +                          ;; Be careful when including store paths in the
> +                          ;; build script, since they might be treated as
> +                          ;; references
> +                          "/gnu/store"
> +                          "/[^-]+-glibc-[^-]+-static"
>                            ")/lib")) ""))))))))))))

LGTM.


Cheers,
simon
Christopher Baines Nov. 7, 2022, 9:32 a.m. UTC | #2
zimoun <zimon.toutoune@gmail.com> writes:

> Hi,
>
> On dim., 06 nov. 2022 at 10:05, Christopher Baines <mail@cbaines.net> wrote:
>
>> * gnu/packages/statistics.scm (r-minimal)[arguments]: Split string involving
>> /gnu/store.
>
> It would imply rebuild many packages, no?  master, staging or
> core-updates?

It does, my plan was/is to ask Ricardo if he could maybe include it when
pushing the next big batch of r related changes?

Chris
Simon Tournier Nov. 7, 2022, 2:10 p.m. UTC | #3
Hi Chris,

On lun., 07 nov. 2022 at 10:32, Christopher Baines <mail@cbaines.net> wrote:

> It does, my plan was/is to ask Ricardo if he could maybe include it when
> pushing the next big batch of r related changes?

I am preparing a Bioconductor upgrade, so it could be part of it.
Ricardo, WDYT?


Cheers,
simon
Ludovic Courtès Nov. 7, 2022, 9:36 p.m. UTC | #4
Christopher Baines <mail@cbaines.net> skribis:

> As this causes the following error when computing a cross-compilation
> derivation for r-minimal and related packages.

Which error?  :-)

Ludo’.
Christopher Baines Nov. 8, 2022, 7:13 a.m. UTC | #5
Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> skribis:
>
>> As this causes the following error when computing a cross-compilation
>> derivation for r-minimal and related packages.
>
> Which error?  :-)

Good spot, I've updated the patch/commit message.

But it's this:

  → guix build --target=aarch64-linux-gnu r-minimal
  guix build: error: path ‘/gnu/store/[^-]+-glibc-[^-]+-static’ is not valid

I'm not sure why this seems to only affect cross derivations. It's not
the only problem that's happened for cross derivations though, so it
might be good to expand the linting to cover them (maybe even for just
one common target like aarch64-linux-gnu).

Thanks,

Chris
Ludovic Courtès Nov. 8, 2022, 1:09 p.m. UTC | #6
Hi,

Christopher Baines <mail@cbaines.net> skribis:

>   → guix build --target=aarch64-linux-gnu r-minimal
>   guix build: error: path ‘/gnu/store/[^-]+-glibc-[^-]+-static’ is not valid

Interesting bug!

I pushed a fix and explanation:

--8<---------------cut here---------------start------------->8---
commit 3bd4b92f55f40119349e39902a9b800de98040d2

    build-system/gnu: Turn #:phases into a gexp when cross-compiling.
    
    Previously, we'd get this error:
    
      $ guix build --target=aarch64-linux-gnu r-minimal -d --no-grafts
      guix build: error: path ‘/gnu/store/[^-]+-glibc-[^-]+-static’ is not valid
    
    This is because the sexp would be passed as an input of the surrounding
    gexp in 'gnu-cross-build', and thus
    "/gnu/store/[^-]+-glibc-[^-]+-static" would be interpreted as a source
    file name, as in this example:
    
      scheme@(guix gexp)> #~(foo #$(list 'whatever "/gnu/store/[^-]+-glibc-[^-]+-static"))
      $11 = #<gexp (foo #<gexp-input (whatever "/gnu/store/[^-]+-glibc-[^-]+-static"):out>) 7f098badec30>
      scheme@(guix gexp)> (gexp-inputs $11)
      $12 = (#<gexp-input "/gnu/store/[^-]+-glibc-[^-]+-static":out>)
    
    Fixes <https://issues.guix.gnu.org/59073>.
    Reported by Christopher Baines <mail@cbaines.net>.
    
    * guix/build-system/gnu.scm (gnu-cross-build): When PHASES is a pair,
    pass it through 'sexp->gexp'.
--8<---------------cut here---------------end--------------->8---

That said, this code is bogus:

  1. We should refrain from using non-literal strings as patterns in
     ‘substitute*’.

  2. We should call ‘%store-directory’ from (guix build utils) instead
     of hardcoding “/gnu/store”.

Both of these should be fixed at some point.

Thanks,
Ludo’.
Simon Tournier Nov. 8, 2022, 9:32 p.m. UTC | #7
Hi,

On Tue, 08 Nov 2022 at 14:09, Ludovic Courtès <ludo@gnu.org> wrote:

> Interesting bug!
>
> I pushed a fix and explanation:

Oh! :-)

> That said, this code is bogus:
>
>   1. We should refrain from using non-literal strings as patterns in
>      ‘substitute*’.

What does it mean “non-literal string” here?


Cheers,
simon
Ludovic Courtès Nov. 10, 2022, 2:07 p.m. UTC | #8
Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

>> That said, this code is bogus:
>>
>>   1. We should refrain from using non-literal strings as patterns in
>>      ‘substitute*’.
>
> What does it mean “non-literal string” here?

A string that is not a literal (or constant, if you prefer).

This is OK:

  (substitute* f
    ((".*") …))

This is not:

  (substitute* f
    (((string-append "." (star))) …))

Ludo’.
Simon Tournier Nov. 10, 2022, 2:40 p.m. UTC | #9
Hi,

On jeu., 10 nov. 2022 at 15:07, Ludovic Courtès <ludo@gnu.org> wrote:

> A string that is not a literal (or constant, if you prefer).
>
> This is OK:
>
>   (substitute* f
>     ((".*") …))
>
> This is not:
>
>   (substitute* f
>     (((string-append "." (star))) …))

Thanks for explaining.

Cheers,
simon
diff mbox series

Patch

diff --git a/gnu/packages/statistics.scm b/gnu/packages/statistics.scm
index 2d4dbe4a31..43988ad00e 100644
--- a/gnu/packages/statistics.scm
+++ b/gnu/packages/statistics.scm
@@ -470,7 +470,11 @@  (define-public r-minimal
                                     "util-macros"
                                     "graphite2"))
                           "|"
-                          "/gnu/store/[^-]+-glibc-[^-]+-static"
+                          ;; Be careful when including store paths in the
+                          ;; build script, since they might be treated as
+                          ;; references
+                          "/gnu/store"
+                          "/[^-]+-glibc-[^-]+-static"
                           ")/lib")) ""))))))))))))
 
 (define-public rmath-standalone