patch-shebang phase breaks symlinks

  • Done
  • quality assurance status badge
Details
3 participants
  • Danny Milosavljevic
  • Jelle Licht
  • Ludovic Courtès
Owner
unassigned
Submitted by
Jelle Licht
Severity
normal

Debbugs page

Jelle Licht wrote 9 years ago
(address . bug-guix@gnu.org)
87fusowopk.fsf@fsfe.org
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
Ludovic Courtès wrote 9 years ago
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 23723@debbugs.gnu.org)
87r3c51agw.fsf@gnu.org
Hello!

Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (7 lines)
> 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?

Toggle quote (14 lines)
> 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’.
Jelle Licht wrote 9 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
87r3c33l79.fsf@fsfe.org
Hi

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (15 lines)
> Hello!
>
> Jelle Licht <jlicht@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.

Toggle quote (20 lines)
> 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.

Toggle quote (5 lines)
>
> Thanks!
>
> Ludo’.

Looking forward to your thoughts on the matter

- Jelle
Ludovic Courtès wrote 9 years ago
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 23723@debbugs.gnu.org)
8737oiog2n.fsf@gnu.org
Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (32 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>> Hello!
>>
>> Jelle Licht <jlicht@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’.
Toggle diff (24 lines)
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)))))
'())))
Ludovic Courtès wrote 9 years ago
control message for bug #23723
(address . control@debbugs.gnu.org)
87eg82jgmc.fsf@gnu.org
tags 23723 patch
Jelle Licht wrote 9 years ago
Re: bug#23723: patch-shebang phase breaks symlinks
(name . Ludovic Courtès)(address . ludo@gnu.org)
87bn3427iu.fsf@fsfe.org
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (36 lines)
> Jelle Licht <jlicht@fsfe.org> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>> Hello!
>>>
>>> Jelle Licht <jlicht@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?

Toggle quote (8 lines)
>
> 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
Ludovic Courtès wrote 9 years ago
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 23723@debbugs.gnu.org)
878ty8jj92.fsf@gnu.org
Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (44 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Jelle Licht <jlicht@fsfe.org> skribis:
>>
>>> Ludovic Courtès <ludo@gnu.org> writes:
>>>> Hello!
>>>>
>>>> Jelle Licht <jlicht@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’.
Jelle Licht wrote 9 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 23723@debbugs.gnu.org)
CAPsKtfLw5NrxvsgDxuFX1L0Y0C=buEvRSLNFZd5SBHeoeyF=cg@mail.gmail.com
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@gnu.org> wrote:

Toggle quote (58 lines)
> Jelle Licht <jlicht@fsfe.org> skribis:
>
> > Ludovic Courtès <ludo@gnu.org> writes:
> >
> >> Jelle Licht <jlicht@fsfe.org> skribis:
> >>
> >>> Ludovic Courtès <ludo@gnu.org> writes:
> >>>> Hello!
> >>>>
> >>>> Jelle Licht <jlicht@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’.
>
Attachment: file
Danny Milosavljevic wrote 9 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
20160616071528.1e56e75b@scratchpost.org
Hi Ludo,

are you sure that's correct?

Compare:

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

Toggle quote (2 lines)
> + (filter (lambda (file)
> + (eq? 'regular (lstat file)))
^^^^^
to

Toggle quote (3 lines)
> - (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 .
Ludovic Courtès wrote 9 years ago
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
8760t9k0qa.fsf@gnu.org
Danny Milosavljevic <dannym@scratchpost.org> skribis:

Toggle quote (20 lines)
> are you sure that's correct?
>
> Compare:
>
> On Sun, 12 Jun 2016 12:29:52 +0200
> ludo@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’.
Ludovic Courtès wrote 8 years ago
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 23723-done@debbugs.gnu.org)
87lgywzz70.fsf@gnu.org
ludo@gnu.org (Ludovic Courtès) skribis:

Toggle quote (2 lines)
> Jelle Licht <jlicht@fsfe.org> skribis:

[...]

Toggle quote (15 lines)
>> 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’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 23723@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 23723
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help