npm has irreproducible install behavior

npm has irreproducible install behavior

[Ricardo Wurmus]

Hi Guix,

after a few days of frustrating investigation I found a bug in one of
the libraries used by npm:

  https://github.com/npm/pacote/issues/285

The result is that “npm install” will not install *all* files dependent
on whether a file is deduplicated in the store.  This causes
irreproducible output and build failures down the line on different
systems depending on the state of the file system.

We should patch this ourselves.  We can either tell node-tar not to mark
up hardlinks with the “Link” type, or we can patch pacote to not skip
files that have the “Link” type.

I’ve tested this little addition to the build phases of node-lts on an
affected system:

--8<---------------cut here---------------start------------->8---
           (add-after 'install 'do-not-ignore-links
             (lambda* (#:key outputs #:allow-other-keys)
               (with-directory-excursion
                   (string-append (assoc-ref outputs "out")
                                  "/lib/node_modules/npm/node_modules")
                 (substitute* "pacote/lib/fetcher.js"
                              (("\\/Link\\$\\/.test\\(entry.type\\)") "false")
                              (("\\/File\\$\\/.test\\(entry.type\\)")
                               "/(File|Link)$/.test(entry.type)"))
--8<---------------cut here---------------end--------------->8---

What do you think?

-- 
Ricardo

Re: npm has irreproducible install behavior

[Jelle Licht]

Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Guix,
>
> after a few days of frustrating investigation I found a bug in one of
> the libraries used by npm:
>
>   https://github.com/npm/pacote/issues/285

Props on finding the root cause, I hope it didn't cost you much sanity :).

> The result is that “npm install” will not install *all* files dependent
> on whether a file is deduplicated in the store.  This causes
> irreproducible output and build failures down the line on different
> systems depending on the state of the file system.
>
> We should patch this ourselves.  We can either tell node-tar not to mark
> up hardlinks with the “Link” type, or we can patch pacote to not skip
> files that have the “Link” type.

The first option makes more sense to me at first glance. Wouldn't the
proposed solution change behavior w.r.t. softlinks as well? OTOH,
deciding where to address this particular issue and which color the
bikeshed should be seems like a job for upstream to figure out.

> I’ve tested this little addition to the build phases of node-lts on an
> affected system:
>
> --8<---------------cut here---------------start------------->8---
>            (add-after 'install 'do-not-ignore-links
>              (lambda* (#:key outputs #:allow-other-keys)
>                (with-directory-excursion
>                    (string-append (assoc-ref outputs "out")
>                                   "/lib/node_modules/npm/node_modules")
>                  (substitute* "pacote/lib/fetcher.js"
>                               (("\\/Link\\$\\/.test\\(entry.type\\)") "false")
>                               (("\\/File\\$\\/.test\\(entry.type\\)")
>                                "/(File|Link)$/.test(entry.type)"))
> --8<---------------cut here---------------end--------------->8---

The proposed change seems fine to me, provided our QA shows a pretty
green button at some point. Could you send a patch that also includes a
comment referencing the upstream bug report?

Thanks,
- Jelle

Re: npm has irreproducible install behavior

[Ricardo Wurmus]

Hi Jelle,

>> I’ve tested this little addition to the build phases of node-lts on an
>> affected system:
>>
>> --8<---------------cut here---------------start------------->8---
>>            (add-after 'install 'do-not-ignore-links
>>              (lambda* (#:key outputs #:allow-other-keys)
>>                (with-directory-excursion
>>                    (string-append (assoc-ref outputs "out")
>>                                   "/lib/node_modules/npm/node_modules")
>>                  (substitute* "pacote/lib/fetcher.js"
>>                               (("\\/Link\\$\\/.test\\(entry.type\\)") "false")
>>                               (("\\/File\\$\\/.test\\(entry.type\\)")
>>                                "/(File|Link)$/.test(entry.type)"))
>> --8<---------------cut here---------------end--------------->8---
>
> The proposed change seems fine to me, provided our QA shows a pretty
> green button at some point. Could you send a patch that also includes a
> comment referencing the upstream bug report?

I changed it to patch node-tar instead, because the patch to fetcher.js
would cause *some* files to be copied to the wrong location.

I’ll send the updated patch to guix-patches today.

-- 
Ricardo

Re: npm has irreproducible install behavior

[Ricardo Wurmus]

Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Jelle,
>
>>> I’ve tested this little addition to the build phases of node-lts on an
>>> affected system:
>>>
>>> --8<---------------cut here---------------start------------->8---
>>>            (add-after 'install 'do-not-ignore-links
>>>              (lambda* (#:key outputs #:allow-other-keys)
>>>                (with-directory-excursion
>>>                    (string-append (assoc-ref outputs "out")
>>>                                   "/lib/node_modules/npm/node_modules")
>>>                  (substitute* "pacote/lib/fetcher.js"
>>>                               (("\\/Link\\$\\/.test\\(entry.type\\)") "false")
>>>                               (("\\/File\\$\\/.test\\(entry.type\\)")
>>>                                "/(File|Link)$/.test(entry.type)"))
>>> --8<---------------cut here---------------end--------------->8---
>>
>> The proposed change seems fine to me, provided our QA shows a pretty
>> green button at some point. Could you send a patch that also includes a
>> comment referencing the upstream bug report?
>
> I changed it to patch node-tar instead, because the patch to fetcher.js
> would cause *some* files to be copied to the wrong location.
>
> I’ll send the updated patch to guix-patches today.

Here’s the patch set:

  https://issues.guix.gnu.org/64592

The first patch is to clean up the snippet while we’re at it.  The
second changes the behavior of node-tar, so that files with hardlinks
are not treated any different from files without.

-- 
Ricardo