bug#31560: Commit 47a60325c broke tests/pack.scm

bug#31560: Commit 47a60325c broke tests/pack.scm

[Chris Marusich]

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

Hi,

It seems that tests/pack.scm began to fail at commit
47a60325ca650e8fc1a291c8655b4297f4de8deb.  This commit made the
following change in guix/scripts/pack.scm:

--8<---------------cut here---------------start------------->8---
           (define symlink->directives
             ;; Return "populate directives" to make the given symlink and its
             ;; parent directories.
             (match-lambda
               ((source '-> target)
                (let ((target (string-append #$profile "/" target)))
                  `((directory ,(dirname source))
-                   (,source -> ,target))))))
+                   (,source
+                    -> ,(relative-file-name (dirname source) target)))))))
--8<---------------cut here---------------end--------------->8---

It seems this causes the test to fail because it calls
relative-file-name like this in the test:

(relative-file-name "/bin"
"/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile")

which evaluates to

"../home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile"

which is not equal to

"/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile"

so the test fails.

What is the purpose of calling relative-file-name here?

-- 
Chris

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

bug#31560: Commit 47a60325c broke tests/pack.scm

[Ludovic Courtès]

Hi Chris,

Chris Marusich <cmmarusich@gmail.com> skribis:

> It seems that tests/pack.scm began to fail at commit
> 47a60325ca650e8fc1a291c8655b4297f4de8deb.  This commit made the
> following change in guix/scripts/pack.scm:
>
>            (define symlink->directives
>              ;; Return "populate directives" to make the given symlink and its
>              ;; parent directories.
>              (match-lambda
>                ((source '-> target)
>                 (let ((target (string-append #$profile "/" target)))
>                   `((directory ,(dirname source))
> -                   (,source -> ,target))))))
> +                   (,source
> +                    -> ,(relative-file-name (dirname source) target)))))))
>
> It seems this causes the test to fail because it calls
> relative-file-name like this in the test:
>
> (relative-file-name "/bin"
> "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile")
>
> which evaluates to
>
> "../home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile"
>
> which is not equal to
>
> "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile"
>
> so the test fails.

Oooh, sorry about that.

However what happens here is more something like:

  (relative-file-name
    "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin"
    "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile")

No?  Hmm not sure…

I suppose the test should ensure we get an appropriate relative symlink.

> What is the purpose of calling relative-file-name here?

The goal is to create only relative symlinks so that the tarball is
relocatable:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31360

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix pack bash-static -S /bin/B=bin/bash
/gnu/store/7dks90572s5i8nnziippsskm7f4iyqhg-tarball-pack.tar.gz
$ tar tvf /gnu/store/7dks90572s5i8nnziippsskm7f4iyqhg-tarball-pack.tar.gz |grep bin/B
lrwxrwxrwx root/root         0 1970-01-01 01:00 ./bin/B -> ../gnu/store/xqgyj976y375wv8gaqh5mz0ysbfdk7f6-profile/bin/bash
hrwxrwxrwx root/root         0 1970-01-01 01:00 ./bin/B link to ./bin/B
--8<---------------cut here---------------end--------------->8---

(The “link to” line really shouldn’t be here but let’s ignore it.)

HTH!

Ludo’.

bug#31560: Commit 47a60325c broke tests/pack.scm

[Chris Marusich]

[-- Attachment #1.1: Type: text/plain, Size: 3371 bytes --]

Hi Ludo,

I believe the attached patches fix the issue when applied to the master
branch.  The failing test now passes (along with all the other "make
check" tests).  What do you think?  If you approve, then I'll commit it
to the master branch.

ludo@gnu.org (Ludovic Courtès) writes:

> Chris Marusich <cmmarusich@gmail.com> skribis:
>
>> It seems that tests/pack.scm began to fail at commit
>> 47a60325ca650e8fc1a291c8655b4297f4de8deb.  This commit made the
>> following change in guix/scripts/pack.scm:
>>
>>            (define symlink->directives
>>              ;; Return "populate directives" to make the given symlink and its
>>              ;; parent directories.
>>              (match-lambda
>>                ((source '-> target)
>>                 (let ((target (string-append #$profile "/" target)))
>>                   `((directory ,(dirname source))
>> -                   (,source -> ,target))))))
>> +                   (,source
>> +                    -> ,(relative-file-name (dirname source) target)))))))
>>
>> It seems this causes the test to fail because it calls
>> relative-file-name like this in the test:
>>
>> (relative-file-name "/bin"
>> "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile")
>>
>> ...
>
> Oooh, sorry about that.

No worries!  Hopefully the fix is as simple as I think it is.

> However what happens here is more something like:
>
>   (relative-file-name
>     "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin"
>     "/home/marusich/guix-upgrade-gnucash/test-tmp/store/3j3mrl1sf3bcx4fzlz655mzsp4bir54j-profile/bin/guile")
>
> No?  Hmm not sure…

On my system, when I ran the test (tests/pack.scm) just now, the
variables in symlink->directives had the following values (I used pk to
view their values - if you know of a better way, I'd love to know!):

  source = "/bin/Guile"
  target = "/home/marusich/guix/test-tmp/store/p8y0nby1mybh5h75hi4hd1x0x36insnd-profile/bin/guile"
  parent = "/bin"

I believe the issue here was that we were not distinguishing between the
"source parent" and the "target parent" directories.  When calling
relative-file-name, I think we need to pass in the "target parent",
which I've done in the attached patches.

> I suppose the test should ensure we get an appropriate relative
>symlink.

Well, the test did catch the problem, so I think the test is OK as
currently written.

>> What is the purpose of calling relative-file-name here?
>
> The goal is to create only relative symlinks so that the tarball is
> relocatable:
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31360
>
> $ ./pre-inst-env guix pack bash-static -S /bin/B=bin/bash
> /gnu/store/7dks90572s5i8nnziippsskm7f4iyqhg-tarball-pack.tar.gz
> $ tar tvf /gnu/store/7dks90572s5i8nnziippsskm7f4iyqhg-tarball-pack.tar.gz |grep bin/B
> lrwxrwxrwx root/root         0 1970-01-01 01:00 ./bin/B -> ../gnu/store/xqgyj976y375wv8gaqh5mz0ysbfdk7f6-profile/bin/bash
> hrwxrwxrwx root/root         0 1970-01-01 01:00 ./bin/B link to ./bin/B
>
> (The “link to” line really shouldn’t be here but let’s ignore it.)

Understood!  Thank you for clarifying the intent.  I figured it was
something like that.  I just wanted to be certain.

-- 
Chris

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-tests-Call-self-contained-tarball-correctly.patch --]
[-- Type: text/x-patch, Size: 1176 bytes --]

From 86b15bdf4b3eb0a8105e7f456c8ba90331dd7cae Mon Sep 17 00:00:00 2001
From: Chris Marusich <cmmarusich@gmail.com>
Date: Fri, 1 Jun 2018 00:13:31 -0700
Subject: [PATCH 1/2] tests: Call self-contained-tarball correctly.

* tests/pack.scm ("self-contained-tarball"): Call the self-contained-tarball
procedure with keyword argument #:archiver instead of #:tar.  This argument
was renamed in commit 5ffac538aa604b71814ac74579626f0d3110b96e.
---
 tests/pack.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/pack.scm b/tests/pack.scm
index 3bce71507..cd7de7800 100644
--- a/tests/pack.scm
+++ b/tests/pack.scm
@@ -61,7 +61,7 @@
                                         #:symlinks '(("/bin/Guile"
                                                       -> "bin/guile"))
                                         #:compressor %gzip-compressor
-                                        #:tar %tar-bootstrap))
+                                        #:archiver %tar-bootstrap))
        (check   (gexp->derivation
                  "check-tarball"
                  #~(let ((guile (string-append "." #$profile "/bin")))
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-pack-Create-the-populate-directives-correctly.patch --]
[-- Type: text/x-patch, Size: 1900 bytes --]

From 27478e3b9d01c073542a0a53635b248af63562f7 Mon Sep 17 00:00:00 2001
From: Chris Marusich <cmmarusich@gmail.com>
Date: Thu, 31 May 2018 23:52:20 -0700
Subject: [PATCH 2/2] pack: Create the "populate directives" correctly.

* guix/scripts/pack.scm (self-contained-tarball)[build](symlink->directives):
Discriminate between "source" and "target" parent directories.
---
 guix/scripts/pack.scm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scm
index 35b8a7e72..e3a759e66 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -123,19 +123,19 @@ added to the pack."
             (match-lambda
               ((source '-> target)
                (let ((target (string-append #$profile "/" target))
-                     (parent (dirname source)))
+                     (source-parent (dirname source))
+                     (target-parent (dirname target)))
                  ;; Never add a 'directory' directive for "/" so as to
                  ;; preserve its ownnership when extracting the archive (see
                  ;; below), and also because this would lead to adding the
                  ;; same entries twice in the tarball.
-                 `(,@(if (string=? parent "/")
+                 `(,@(if (string=? source-parent "/")
                          '()
-                         `((directory ,parent)))
+                         `((directory ,source-parent)))
                    (,source
-                    -> ,(relative-file-name parent target)))))))
+                    -> ,(relative-file-name target-parent target)))))))
 
           (define directives
-            ;; Fully-qualified symlinks.
             (append-map symlink->directives '#$symlinks))
 
           ;; The --sort option was added to GNU tar in version 1.28, released
-- 
2.17.0


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

bug#31560: Commit 47a60325c broke tests/pack.scm

[Ludovic Courtès]

Hello,

Chris Marusich <cmmarusich@gmail.com> skribis:

> I believe the attached patches fix the issue when applied to the master
> branch.  The failing test now passes (along with all the other "make
> check" tests).  What do you think?  If you approve, then I'll commit it
> to the master branch.

I looked some more and the patch would create broken links.  I concluded
that the issue was simply that the test needed to be adjusted to expect
a relative symlink instead of an absolute symlink.

I first cherry-picked the fix that Ricardo made in ‘core-updates’ (same
as the one you posted) as commit
44057a461b1fa8102938c4e0f54d7cbc9dd09b03.  Then I adjusted the test in
commit ccc951cab3172adfdaf6fd2dfa8f8cdb98358a69.

Let me know if you think something is amiss!

Thank you,
Ludo’.