From debbugs-submit-bounces@debbugs.gnu.org Sun Sep 26 18:02:56 2021 Received: (at 49946) by debbugs.gnu.org; 26 Sep 2021 22:02:56 +0000 Received: from localhost ([127.0.0.1]:38927 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mUcEa-0004UH-7Z for submit@debbugs.gnu.org; Sun, 26 Sep 2021 18:02:56 -0400 Received: from mail-qk1-f171.google.com ([209.85.222.171]:33669) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mUcEY-0004U4-Cl for 49946@debbugs.gnu.org; Sun, 26 Sep 2021 18:02:55 -0400 Received: by mail-qk1-f171.google.com with SMTP id d207so35052735qkg.0 for <49946@debbugs.gnu.org>; Sun, 26 Sep 2021 15:02:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philipmcgrath.com; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=QjLw4Mn90chDV10lYjLz2lOQVsQu0l/I9vkZk8FXYwc=; b=Ox1HjIJeRKq8qAQAA/Cz2cBrPq4zJ5LDoBBPhUes+YozhS7u6UK47Vnks4YIVXfLHB wn5nnhn+Gb57bvkUTvWnp+nzt65gWGQd4VHZFBB7+7O3+k2CbJa09I8XfKpEQAgc4p9E 0776V7jzfJJfq8pvap76+zSV9OH6ZEJ+hOBfywRRdVuWuJySaQDxlO7erqg4lKWGPVON Hf0AYJHsbwIbZj20H1I3ICp6XU4HCYOIUJiHUeIsBHqQABIHAya8NIRRLtwDUnVLE1yX MHMpVFamNE1WIHp/lzDReFTrtuGy5xSBU0Br5XqfdrrNx+XV78kn/7oFxlYSXRF1q1au hF8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QjLw4Mn90chDV10lYjLz2lOQVsQu0l/I9vkZk8FXYwc=; b=BXTtoA7oj8YEZU2uRKOV88boJeQUUkoYqYHeQDe8OMuuMRboZYHEx/e/YtZaVd1Gsv UWC+Dqux+E9Z/Avr6xqOAdJ6PJee3m4ZlS7I1T3maO/lmJZCplU06NvGN75X46rEY/BA F+DD/WCSQ45DtO7PdWe1VABKTrO9VQt4sg4gqZOZ3MyXyOfuL8Tb/NX1rN3SbVSoJuSw EeVrtvbIShqtGzsao17ntx2ay97fs5OEgpNX2snweJ+6Gdj2mpUgyZFnd1ljy6YaPCN2 ydC0jFXo8gpiiKX06xTaDQaJKqePrPRZjhaH1RsWawlmI9ltO6v2BLMrzykkyYudMfRv il7A== X-Gm-Message-State: AOAM531c6mpwwrCV6a5q1lKjTFwWq0PypgK2SBAqmQtbZAvsn0Shdqdi mvNIgfUMlUkQy+Nhfk0rK0xvtj1t1dOWq9ZS X-Google-Smtp-Source: ABdhPJyy+0EyoMXraDZXdutiYuQeHCjROva80csaHoGC59/5rNIk+lMfKaJgt3n9GDxAsDSK3P5hdQ== X-Received: by 2002:ae9:ed45:: with SMTP id c66mr21729605qkg.336.1632693768505; Sun, 26 Sep 2021 15:02:48 -0700 (PDT) Received: from [192.168.45.37] (c-73-125-89-242.hsd1.fl.comcast.net. [73.125.89.242]) by smtp.gmail.com with ESMTPSA id w7sm317578qtc.29.2021.09.26.15.02.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 26 Sep 2021 15:02:48 -0700 (PDT) Subject: Re: [bug#49946] [PATCH 08/31] gnu: node: Patch /usr/bin/env in node-gyp. To: Pierre Langlois References: <87h7fztt60.fsf@gmx.com> <20210808233354.6745-1-pierre.langlois@gmx.com> <20210808233354.6745-8-pierre.langlois@gmx.com> <42e10baddb6afe308f67c3240bf5da8159e6f118.camel@telenet.be> <87o88gq5p5.fsf@gmx.com> From: Philip McGrath Message-ID: Date: Sun, 26 Sep 2021 18:02:47 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <87o88gq5p5.fsf@gmx.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Score: 0.6 (/) X-Debbugs-Envelope-To: 49946 Cc: 49946@debbugs.gnu.org, Maxime Devos X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.4 (/) Hi Pierre, On 9/25/21 6:24 AM, Pierre Langlois wrote: > Philip McGrath writes: >> Since the shebang line for node-gyp is specifically "#!/usr/bin/env node", I >> wonder if it should use the node built by this package, rather than a dynamic >> node. > > Yeah we could do that, although I generally prefer to follow whatever > the script already does, there could be a good reason for them to use > `env' no I think it might be better to use `patch-shebang` from `(guix build utils)` rather than `substitute*` these by hand, and it seems that `patch-shebang` removed the indirection through `env`. My guess is most of these cases are to accommodate the fact that `node` and `python` are often installed to places other than `/usr/bin`. >> I'd guess node-gyp may not be the only one with shebangs that ought to >> be patched. > > Yeah there could be others, although normally the patching phase from > the gnu build system should have taken care of most of them, hopefully > all, I'm not sure why it didn't work for /usr/bin/env though. > > I would suggest we patch things as we encounter them, did you find > anymore issues when working on your package? Looking at `gnu-build-system`, it seems that the `'patch-shebangs` phase only operates on files installed in the "/bin" and "/sbin" subdirectories of the package's outputs. That restriction doesn't make sense to me in general: for instance, what about "/libexec"? For Node specifically, this misses a lot of stuff under "/lib/node_modules" and "/lib/node_modules/npm/node_modules". I think I more general fix could subsume the `'patch-npm-shebang` and `'patch-node-shebang` phases in building Node, too. > For instance, while working on a newer version of one of the packages in > this series, I saw we may need to patch GYP's python reference as well, > like so: > > (substitute* "deps/npm/node_modules/node-gyp/gyp/gyp_main.py" > (("#!/usr/bin/env python") > (string-append "#!" (assoc-ref inputs "python") "/bin/python3"))) > > Only for node 14+. The reason seems to be that gyp still refers to > "python", but python2 is no longer a dependency for newer nodes. And it > seems GYP is perfectly happy with python3, and the shebang is fixed > upstream so a never node will be fine: > https://github.com/nodejs/node-gyp/pull/2355/files I think in some places (but perhaps not enough places) Guix uses `python-wrapper` to work around this ... > > Maybe updating node would be better than this fix though. I'm not totally clear on whether the upstream fix is in 14.17.6 LTS, but, if so, that seems great! > >> More generally, I see that there are 355 directories installed under >> "lib/node_modules/npm/node_modules" (which corresponds to the "deps" >> path above). Most of them don't seem to be available as Guix packages that could >> be depended upon by other Guix node packages. > > Yeah that's tricky, ideally we should remove all the node_modules deps > and package them separately, I wonder if anybody tried to do that > already. I would suspect it to be quite a lot of work, sometimes > unbundling stops being worth and when it's hard to maintain dependencies > manually. > > Hopefully we can get there one day though! I don't want to deter anybody > from trying :-), I might give it a go on a raindy day. Since these are developed and released with Node, and apparently we can build them as part of the Node build process, I was thinking we could just make packages that point to these versions we're already building. It might be good to hear from someone who develops with node/npm, though ... I just use it to install software that I can't find packaged elsewhere. > >> On 8/8/21 6:29 PM, Pierre Langlois wrote: >> >>> ... `node-gyp' needs >> >>> node headers to compile against, packaged as a tarball, which it tries >> >>> to download. Instead, we can run a `node-gyp --tarball <> configure' >> >>> step to manually provide the tarball, which we can package separately >> >>> for any given node version. >> >> There is also a --nodedir option, which I found could work with something like: >> >> (string-append "--nodedir=" (assoc-ref inputs "node")) >> >> That seems like it might be better, though I don't know all the considerations >> for cross-compilation and such. > > Oh that's a good idea, I didn't really like having to download the > headers separately from the main package, especially given we run > snippet on the source to remove bundled dependencies. > > Trying this out this approach does work, but I needed to: > > - Create a union directory with both node and libuv. The node package > only has headers for V8/node, but we also need libuv, so doing > something like this works: > > (union-build node-sources > (list (assoc-ref inputs "node") > (assoc-ref inputs "libuv")) > #:create-all-directories? #t > #:log-port (%make-void-port "w")) I found it worked to just add libuv as an input of packages built with node-gyp. I hadn't tried to change `node-build-system`, but I think that would be the place to do it. > > - For some reason, --nodedir didn't really "configure" gyp to use that > node directory, I think it's meant to be passed everytime you run > any gyp command. Instead I found that you can use and environment > variable: > > (setenv "npm_config_nodedir" node-sources) That seems right. I believe there's a similar "npm_config_python" for the Python executable to use. Alternatively, I think it's possible to configure these in $PREFIX/etc/npmrc: > > And that works for the packages in this series! That'll be much better > than before, I'll do it this way. > > Thanks again for taking a look, I'll see if I can send updated patches > sometimes this weekend. Glad it was useful! For patching the shebangs, here's a variant of node-lts that worked for me, though I think it would be even better to combine it with the existing phases: ``` (define-public patched-node (let ((node node-lts)) (package (inherit node) (arguments (substitute-keyword-arguments (package-arguments node) ((#:phases standard-phases) `(modify-phases ,standard-phases (add-after 'patch-npm-shebang 'patch-more-shebangs (lambda* (#:key inputs outputs #:allow-other-keys) (define (append-map f lst) (apply append (map f lst))) ;; from patch-shebangs (define bin-directories ;;(match-lambda ;; ((_ . dir) (lambda (pr) (let ((dir (cdr pr))) (list (string-append dir "/bin") (string-append dir "/sbin"))))) (define output-bindirs (append-map bin-directories outputs)) (define input-bindirs ;; Shebangs should refer to binaries of the target system---i.e., from ;; "inputs", not from "native-inputs". (append-map bin-directories inputs)) (define path (append output-bindirs input-bindirs)) (with-directory-excursion (string-append (assoc-ref outputs "out") "/lib/node_modules/npm/node_modules") (for-each ;;(cut patch-shebang <> path) (lambda (file) (patch-shebang file path)) ;; from patch-generated-file-shebangs (find-files "." (lambda (file stat) (and (eq? 'regular (stat:type stat)) (not (zero? (logand (stat:mode stat) #o100))))) #:stat lstat)))))))))))) ``` -Philip