GNU bug report logs - #23723
patch-shebang phase breaks symlinks

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23723 in the body.
You can then email your comments to 23723 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: bug-guix <at> gnu.org
Subject: patch-shebang phase breaks symlinks
Date: Wed, 08 Jun 2016 01:42:31 +0200

Hi Guix,

It seems that the patch-shebang functionality does not deal gracefully
with symlinks: it just overwrites them!

After struggling somewhat with getting the recently packaged node 6.0.0
to behave, I found out that `patch-shebang' in (guix build
gnu-build-system) does not work properly on symlinks.

To illustrate, in this specific case, there was an executable
script included with the node tarball, namely
`lib/node-modules/npm/bin/npm-cli.js' with an env-based shebang:
`/usr/bin/env node'.

As the `node' executable will only be available after the `install'
phase of the build system, it is not patched before hand. During the
`install' phase, a symlink is created from `bin/npm' to
`lib/node-modules/npm/bin/npm-cli.js' (in the store output directory
this time, of course). Then the `patch-shebangs' phase finds this
symlink and proceeds to patch it. Instead of transparently following the
symlink and patching the `npm-cli.js' script, the `npm' symlink is
overwritten with a shebang-patched copy of `npm-cli.js'.

For node, this is a problem because of how node loads run-time
dependencies; load paths are resolved relative to the actual file, not
the symlink.

- Jelle

Message #8 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 23723 <at> debbugs.gnu.org
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Fri, 10 Jun 2016 14:42:39 +0200

Hello!

Jelle Licht <jlicht <at> fsfe.org> skribis:

> It seems that the patch-shebang functionality does not deal gracefully
> with symlinks: it just overwrites them!
>
> After struggling somewhat with getting the recently packaged node 6.0.0
> to behave, I found out that `patch-shebang' in (guix build
> gnu-build-system) does not work properly on symlinks.

There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
touches only regular files (see ‘list-of-files’).

However, ‘patch-source-shebangs’ indeed overwrites symlinks.  Is it the
one that’s causing problems?

> To illustrate, in this specific case, there was an executable
> script included with the node tarball, namely
> `lib/node-modules/npm/bin/npm-cli.js' with an env-based shebang:
> `/usr/bin/env node'.
>
> As the `node' executable will only be available after the `install'
> phase of the build system, it is not patched before hand. During the
> `install' phase, a symlink is created from `bin/npm' to
> `lib/node-modules/npm/bin/npm-cli.js' (in the store output directory
> this time, of course). Then the `patch-shebangs' phase finds this
> symlink and proceeds to patch it. Instead of transparently following the
> symlink and patching the `npm-cli.js' script, the `npm' symlink is
> overwritten with a shebang-patched copy of `npm-cli.js'.

I don’t think ‘patch-shebangs’ is to blame; could it be
‘patch-source-shebangs’?

Thanks!

Ludo’.

Message #11 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 23723 <at> debbugs.gnu.org, Jelle Licht <jlicht <at> fsfe.org>
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Sat, 11 Jun 2016 15:32:26 +0200

Hi

Ludovic Courtès <ludo <at> gnu.org> writes:
> Hello!
>
> Jelle Licht <jlicht <at> fsfe.org> skribis:
>
>> It seems that the patch-shebang functionality does not deal gracefully
>> with symlinks: it just overwrites them!
>>
>> After struggling somewhat with getting the recently packaged node 6.0.0
>> to behave, I found out that `patch-shebang' in (guix build
>> gnu-build-system) does not work properly on symlinks.
>
> There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
> touches only regular files (see ‘list-of-files’).
>

It seems I made a mistake when writing the bug report; I am talking
about the `patch-shebang' defined in (guix build utils). My apologies.

Also, seeing as my experience with the stat utility and similarly styled
programming libraries was lacking, I decided to play around with the
definition of `list-of-files': It actually does include symlinks, as
(stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
Looking into this a bit more, it seems that calling `stat' gives the
exact same results on both the linked-to-file and the symlink to that
file.

For the particular problem I ran into to be fixed, it is imperative that
`list-of-files' of `patch-shebangs' includes the symlink; it does after
all need to be patched. The way this patching currently happens just
clobbers symlinks.

> However, ‘patch-source-shebangs’ indeed overwrites symlinks.  Is it the
> one that’s causing problems?
>
>> To illustrate, in this specific case, there was an executable
>> script included with the node tarball, namely
>> `lib/node-modules/npm/bin/npm-cli.js' with an env-based shebang:
>> `/usr/bin/env node'.
>>
>> As the `node' executable will only be available after the `install'
>> phase of the build system, it is not patched before hand. During the
>> `install' phase, a symlink is created from `bin/npm' to
>> `lib/node-modules/npm/bin/npm-cli.js' (in the store output directory
>> this time, of course). Then the `patch-shebangs' phase finds this
>> symlink and proceeds to patch it. Instead of transparently following the
>> symlink and patching the `npm-cli.js' script, the `npm' symlink is
>> overwritten with a shebang-patched copy of `npm-cli.js'.
>
> I don’t think ‘patch-shebangs’ is to blame; could it be
> ‘patch-source-shebangs’?

AFAIK, it is definitely the 'patch-shebangs' phase that is to blame for
my woes; removing it using modify-phases makes the issue disappear, when
looking at the build dir in the store.

>
> Thanks!
>
> Ludo’.

Looking forward to your thoughts on the matter

- Jelle

Message #14 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 23723 <at> debbugs.gnu.org
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Sun, 12 Jun 2016 12:29:52 +0200

[Message part 1 (text/plain, inline)]

Jelle Licht <jlicht <at> fsfe.org> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>> Hello!
>>
>> Jelle Licht <jlicht <at> fsfe.org> skribis:
>>
>>> It seems that the patch-shebang functionality does not deal gracefully
>>> with symlinks: it just overwrites them!
>>>
>>> After struggling somewhat with getting the recently packaged node 6.0.0
>>> to behave, I found out that `patch-shebang' in (guix build
>>> gnu-build-system) does not work properly on symlinks.
>>
>> There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
>> touches only regular files (see ‘list-of-files’).
>>
>
> It seems I made a mistake when writing the bug report; I am talking
> about the `patch-shebang' defined in (guix build utils). My apologies.
>
> Also, seeing as my experience with the stat utility and similarly styled
> programming libraries was lacking, I decided to play around with the
> definition of `list-of-files': It actually does include symlinks, as
> (stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
> Looking into this a bit more, it seems that calling `stat' gives the
> exact same results on both the linked-to-file and the symlink to that
> file.
>
> For the particular problem I ran into to be fixed, it is imperative that
> `list-of-files' of `patch-shebangs' includes the symlink; it does after
> all need to be patched. The way this patching currently happens just
> clobbers symlinks.

My bad, indeed, ‘list-of-files’ should use ‘lstat’ instead of ‘stat’.

I think a patch like attached should solve the problem.  WDYT?

We can apply it to core-updates-next if that’s fine with you.

Thanks,
Ludo’.

[Message part 2 (text/x-patch, inline)]

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index 2abaa6e..299eb5d 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -172,9 +172,8 @@ files such as `.in' templates.  Most scripts honor $SHELL and
 $CONFIG_SHELL, but some don't, such as `mkinstalldirs' or Automake's
 `missing' script."
   (for-each patch-shebang
-            (remove (lambda (file)
-                      (or (not (file-exists? file)) ;dangling symlink
-                          (file-is-directory? file)))
+            (filter (lambda (file)
+                      (eq? 'regular (lstat file)))
                     (find-files "."))))
 
 (define (patch-generated-file-shebangs . rest)
@@ -303,7 +302,7 @@ makefiles."
   (define (list-of-files dir)
     (map (cut string-append dir "/" <>)
          (or (scandir dir (lambda (f)
-                            (let ((s (stat (string-append dir "/" f))))
+                            (let ((s (lstat (string-append dir "/" f))))
                               (eq? 'regular (stat:type s)))))
              '())))
 

Message #19 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 23723 <at> debbugs.gnu.org, Jelle Licht <jlicht <at> fsfe.org>
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Mon, 13 Jun 2016 21:50:01 +0200

Ludovic Courtès <ludo <at> gnu.org> writes:

> Jelle Licht <jlicht <at> fsfe.org> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>> Hello!
>>>
>>> Jelle Licht <jlicht <at> fsfe.org> skribis:
>>>
>>>> It seems that the patch-shebang functionality does not deal gracefully
>>>> with symlinks: it just overwrites them!
>>>>
>>>> After struggling somewhat with getting the recently packaged node 6.0.0
>>>> to behave, I found out that `patch-shebang' in (guix build
>>>> gnu-build-system) does not work properly on symlinks.
>>>
>>> There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
>>> touches only regular files (see ‘list-of-files’).
>>>
>>
>> It seems I made a mistake when writing the bug report; I am talking
>> about the `patch-shebang' defined in (guix build utils). My apologies.
>>
>> Also, seeing as my experience with the stat utility and similarly styled
>> programming libraries was lacking, I decided to play around with the
>> definition of `list-of-files': It actually does include symlinks, as
>> (stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
>> Looking into this a bit more, it seems that calling `stat' gives the
>> exact same results on both the linked-to-file and the symlink to that
>> file.
>>
>> For the particular problem I ran into to be fixed, it is imperative that
>> `list-of-files' of `patch-shebangs' includes the symlink; it does after
>> all need to be patched. The way this patching currently happens just
>> clobbers symlinks.
>
> My bad, indeed, ‘list-of-files’ should use ‘lstat’ instead of ‘stat’.

This would be one way of fixing this bug. I'd rather see that
`patch-shebang' in (guix build utils) checks for symlinks, and if so,
patches the actual file instead of the symlink. This is the approach I
currently use in my tree to use node 6.0. Would there be any downside to
this approach?

>
> I think a patch like attached should solve the problem.  WDYT?
>
> We can apply it to core-updates-next if that’s fine with you.
>
> Thanks,
> Ludo’.

If this is the approach you'd rather follow, that is okay with me as
well. I just think that a phase that transparently patches all files in
bin/sbin, whether they are actual files or symlinks, would be more
useful.

Greetings,
- Jelle

Message #22 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 23723 <at> debbugs.gnu.org
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Tue, 14 Jun 2016 09:56:57 +0200

Jelle Licht <jlicht <at> fsfe.org> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Jelle Licht <jlicht <at> fsfe.org> skribis:
>>
>>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>>> Hello!
>>>>
>>>> Jelle Licht <jlicht <at> fsfe.org> skribis:
>>>>
>>>>> It seems that the patch-shebang functionality does not deal gracefully
>>>>> with symlinks: it just overwrites them!
>>>>>
>>>>> After struggling somewhat with getting the recently packaged node 6.0.0
>>>>> to behave, I found out that `patch-shebang' in (guix build
>>>>> gnu-build-system) does not work properly on symlinks.
>>>>
>>>> There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
>>>> touches only regular files (see ‘list-of-files’).
>>>>
>>>
>>> It seems I made a mistake when writing the bug report; I am talking
>>> about the `patch-shebang' defined in (guix build utils). My apologies.
>>>
>>> Also, seeing as my experience with the stat utility and similarly styled
>>> programming libraries was lacking, I decided to play around with the
>>> definition of `list-of-files': It actually does include symlinks, as
>>> (stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
>>> Looking into this a bit more, it seems that calling `stat' gives the
>>> exact same results on both the linked-to-file and the symlink to that
>>> file.
>>>
>>> For the particular problem I ran into to be fixed, it is imperative that
>>> `list-of-files' of `patch-shebangs' includes the symlink; it does after
>>> all need to be patched. The way this patching currently happens just
>>> clobbers symlinks.
>>
>> My bad, indeed, ‘list-of-files’ should use ‘lstat’ instead of ‘stat’.
>
> This would be one way of fixing this bug. I'd rather see that
> `patch-shebang' in (guix build utils) checks for symlinks, and if so,
> patches the actual file instead of the symlink. This is the approach I
> currently use in my tree to use node 6.0. Would there be any downside to
> this approach?

Both would work, but I think the “spirit” is that symlinks are supposed
to be transparent, and tools/procedures that operate on files shouldn’t
try to do anything smart about symlinks.  Thus I have a slight
preference for pushing the smartness to the edges.  WDYT?

Thanks,
Ludo’.

Message #25 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 23723 <at> debbugs.gnu.org
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Tue, 14 Jun 2016 10:22:33 +0200

[Message part 1 (text/plain, inline)]

Seems like a good policy in general.
I'll apply your patch and fix up node then.
Thanks a lot for the quick follow up.
- Jelle
On Jun 14, 2016 9:57 AM, "Ludovic Courtès" <ludo <at> gnu.org> wrote:

> Jelle Licht <jlicht <at> fsfe.org> skribis:
>
> > Ludovic Courtès <ludo <at> gnu.org> writes:
> >
> >> Jelle Licht <jlicht <at> fsfe.org> skribis:
> >>
> >>> Ludovic Courtès <ludo <at> gnu.org> writes:
> >>>> Hello!
> >>>>
> >>>> Jelle Licht <jlicht <at> fsfe.org> skribis:
> >>>>
> >>>>> It seems that the patch-shebang functionality does not deal
> gracefully
> >>>>> with symlinks: it just overwrites them!
> >>>>>
> >>>>> After struggling somewhat with getting the recently packaged node
> 6.0.0
> >>>>> to behave, I found out that `patch-shebang' in (guix build
> >>>>> gnu-build-system) does not work properly on symlinks.
> >>>>
> >>>> There’s ‘patch-shebangs’ (plural) in this file, but it explicitly
> >>>> touches only regular files (see ‘list-of-files’).
> >>>>
> >>>
> >>> It seems I made a mistake when writing the bug report; I am talking
> >>> about the `patch-shebang' defined in (guix build utils). My apologies.
> >>>
> >>> Also, seeing as my experience with the stat utility and similarly
> styled
> >>> programming libraries was lacking, I decided to play around with the
> >>> definition of `list-of-files': It actually does include symlinks, as
> >>> (stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
> >>> Looking into this a bit more, it seems that calling `stat' gives the
> >>> exact same results on both the linked-to-file and the symlink to that
> >>> file.
> >>>
> >>> For the particular problem I ran into to be fixed, it is imperative
> that
> >>> `list-of-files' of `patch-shebangs' includes the symlink; it does after
> >>> all need to be patched. The way this patching currently happens just
> >>> clobbers symlinks.
> >>
> >> My bad, indeed, ‘list-of-files’ should use ‘lstat’ instead of ‘stat’.
> >
> > This would be one way of fixing this bug. I'd rather see that
> > `patch-shebang' in (guix build utils) checks for symlinks, and if so,
> > patches the actual file instead of the symlink. This is the approach I
> > currently use in my tree to use node 6.0. Would there be any downside to
> > this approach?
>
> Both would work, but I think the “spirit” is that symlinks are supposed
> to be transparent, and tools/procedures that operate on files shouldn’t
> try to do anything smart about symlinks.  Thus I have a slight
> preference for pushing the smartness to the edges.  WDYT?
>
> Thanks,
> Ludo’.
>

[Message part 2 (text/html, inline)]

Message #28 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 23723 <at> debbugs.gnu.org, Jelle Licht <jlicht <at> fsfe.org>
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Thu, 16 Jun 2016 07:15:28 +0200

Hi Ludo,

are you sure that's correct?

Compare:

On Sun, 12 Jun 2016 12:29:52 +0200
ludo <at> gnu.org (Ludovic Courtès) wrote:

> +            (filter (lambda (file)
> +                      (eq? 'regular (lstat file)))
                                     ^^^^^
to

> -                            (let ((s (stat (string-append dir "/" f))))
> +                            (let ((s (lstat (string-append dir "/" f))))
>                                (eq? 'regular (stat:type s)))))
                                                ^^^^^^^^^
.

I think the first is missing a call to stat:type .

Message #31 received at 23723 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 23723 <at> debbugs.gnu.org, Jelle Licht <jlicht <at> fsfe.org>
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Thu, 16 Jun 2016 10:16:13 +0200

Danny Milosavljevic <dannym <at> scratchpost.org> skribis:

> are you sure that's correct?
>
> Compare:
>
> On Sun, 12 Jun 2016 12:29:52 +0200
> ludo <at> gnu.org (Ludovic Courtès) wrote:
>
>> +            (filter (lambda (file)
>> +                      (eq? 'regular (lstat file)))
>                                      ^^^^^
> to
>
>> -                            (let ((s (stat (string-append dir "/" f))))
>> +                            (let ((s (lstat (string-append dir "/" f))))
>>                                (eq? 'regular (stat:type s)))))
>                                                 ^^^^^^^^^
> .
>
> I think the first is missing a call to stat:type .

Good catch!  I’ll adjust it when I commit to ‘core-updates-next’.

Thank you!

Ludo’.

Message #36 received at 23723-done <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 23723-done <at> debbugs.gnu.org
Subject: Re: bug#23723: patch-shebang phase breaks symlinks
Date: Mon, 12 Sep 2016 21:37:55 +0200

ludo <at> gnu.org (Ludovic Courtès) skribis:

> Jelle Licht <jlicht <at> fsfe.org> skribis:

[...]

>> Also, seeing as my experience with the stat utility and similarly styled
>> programming libraries was lacking, I decided to play around with the
>> definition of `list-of-files': It actually does include symlinks, as
>> (stat:type (stat "some-symlinked-file")) gives us a plain old 'regular.
>> Looking into this a bit more, it seems that calling `stat' gives the
>> exact same results on both the linked-to-file and the symlink to that
>> file.
>>
>> For the particular problem I ran into to be fixed, it is imperative that
>> `list-of-files' of `patch-shebangs' includes the symlink; it does after
>> all need to be patched. The way this patching currently happens just
>> clobbers symlinks.
>
> My bad, indeed, ‘list-of-files’ should use ‘lstat’ instead of ‘stat’.

This was fixed some time ago in core-updates by commit
c13a9feb5b64fd819eaed38a17da0284bbe2b8d9; closing this bug!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.