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
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’). 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’.
Hi 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. > 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
[-- Attachment #1: Type: text/plain, Size: 1704 bytes --] 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’. 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’. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 1101 bytes --] 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 <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? > > 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
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 #1: Type: text/plain, Size: 2633 bytes --] 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: > 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 #2: Type: text/html, Size: 3624 bytes --]
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: > + (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 .
Danny Milosavljevic <dannym@scratchpost.org> skribis:
> 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’.
ludo@gnu.org (Ludovic Courtès) skribis: > Jelle Licht <jlicht@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’.