From patchwork Mon Sep 21 20:33:43 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Giacomo Leidi X-Patchwork-Id: 24244 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 447A527BBE8; Mon, 21 Sep 2020 21:34:32 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_ADSP_ALL, DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL, T_DKIM_INVALID,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 1040627BBE7 for ; Mon, 21 Sep 2020 21:34:31 +0100 (BST) Received: from localhost ([::1]:44456 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kKSW6-0006UR-2t for patchwork@mira.cbaines.net; Mon, 21 Sep 2020 16:34:30 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:38710) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKSVe-0006U9-8h for guix-patches@gnu.org; Mon, 21 Sep 2020 16:34:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:44920) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kKSVd-0006wZ-Vh for guix-patches@gnu.org; Mon, 21 Sep 2020 16:34:01 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kKSVd-00083Z-SS for guix-patches@gnu.org; Mon, 21 Sep 2020 16:34:01 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#41219] [PATCH 2/2] guix: Enforce package.json "files" directive. Resent-From: paul Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 21 Sep 2020 20:34:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41219 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: To: Jelle Licht , 41219@debbugs.gnu.org Received: via spool by 41219-submit@debbugs.gnu.org id=B41219.160072042930951 (code B ref 41219); Mon, 21 Sep 2020 20:34:01 +0000 Received: (at 41219) by debbugs.gnu.org; 21 Sep 2020 20:33:49 +0000 Received: from localhost ([127.0.0.1]:56466 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kKSVQ-000838-K2 for submit@debbugs.gnu.org; Mon, 21 Sep 2020 16:33:49 -0400 Received: from confino.investici.org ([212.103.72.250]:64213) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kKSVO-00082y-Ev for 41219@debbugs.gnu.org; Mon, 21 Sep 2020 16:33:48 -0400 Received: from mx1.investici.org (unknown [127.0.0.1]) by confino.investici.org (Postfix) with ESMTP id 4BwGP43nwRz10wD; Mon, 21 Sep 2020 20:33:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=autistici.org; s=stigmate; t=1600720424; bh=I36Zmvg5sArX0WdmjkzqbsKQRI0jnrjPlz0nY4059Ug=; h=Subject:To:References:From:Date:In-Reply-To:From; b=PejP3+OYnPJmSL7IEv2BNimkQYDIzxuLDteuc5gVKTSSxKTUlyG1ExkT9N9Ac4Mpo VUwm3gC1hQ3rXMcuUNipYVeL8OB49JBnhaO6I+8r3/m6+N7uwXzKUb99p4jTqq4XGL ArvMAzmwCmagbOyMlkL7l7wxjSZ7hFKngdBa8NxE= Received: from [212.103.72.250] (mx1.investici.org [212.103.72.250]) (Authenticated sender: goodoldpaul@autistici.org) by localhost (Postfix) with ESMTPSA id 4BwGP42zb0z10w6; Mon, 21 Sep 2020 20:33:44 +0000 (UTC) References: <20200512213131.28873-1-goodoldpaul@autistici.org> <20200512213131.28873-2-goodoldpaul@autistici.org> <875z88jkg9.fsf@jlicht.xyz> From: paul Message-ID: <7bea951c-c8c8-cca9-4bfe-8d8f5c83e2ab@autistici.org> Date: Mon, 21 Sep 2020 22:33:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Icedove/68.12.0 MIME-Version: 1.0 In-Reply-To: <875z88jkg9.fsf@jlicht.xyz> Content-Language: en-US X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: "Guix-patches" X-getmail-retrieved-from-mailbox: Patches Hi Jelle, On 9/20/20 9:51 PM, Jelle Licht wrote: > Hey Giacomo, > > Apologies for the delay! Better late than never, a review just for you. No problem really, I spent some time AFK this summer and didn't ping soon enough. > Perhaps 'pattern-list'? I keep reading this as patron-list. We could > also build the patterns here. Mapping over the pattern-list + 'default > patterns' here might also be a wee bit faster. Yeah I actually don't know why I avoided to type two more letters in the first place. I didn't build the patterns here because that would have required storing the match result in a separate variable binding and requiring to check twice if the binding was false (which is the way I went in the new patch. The only slight downside in the new patch is that if the match result is #f then patterns is # but is also provably never accessed. If you can think of a better way to solve this, please do tell me), but mapping first is still more efficient, so I changed it. > >> + (#f #f))) >> + (main (match (assoc-ref data "main") >> + ("" #f) >> + ((? string? main-module) main-module) >> + (#f #f))) >> + (install-dir (string-append target "/node_modules/" modulename)) >> + (install-files (lambda (files directory) > ^ > You only use install-dir here: you could hard-code it in the lambda. Definitely, I just fixed that. > >> + (for-each (lambda (file) >> + (install-file >> + file >> + (string-append directory "/" >> + (dirname file)))) >> + files)))) >> (mkdir-p target) >> - (copy-recursively "." (string-append target "/node_modules/" modulename)) >> - ;; Remove references to dependencies >> - (delete-file-recursively >> - (string-append target "/node_modules/" modulename "/node_modules")) >> + (if patterns >> + (install-files >> + (filter (lambda (file) >> + (any (lambda (pattern) >> + (glob-match? >> + (string->compiled-sglob pattern) >> + file)) >> + (append >> + patterns >> + '("package.json" >> + ;; These files get installed no >> + ;; matter the case or extension. >> + "[rR][eE][aA][dD][mM][eE]*" >> + "[cC][hH][aA][nN][gG][eE][sS]*" >> + "[cC][hH][aA][nN][gG][eE][lL][oO][gG]*" >> + "[hH][iI][sS][tT][oO][rR][yY]*" >> + "[nN][oO][tT][iI][cC][eE]*")))) >> + (map (lambda (path) >> + (string-drop path 2)) > ^ > If this is meant to drop the "./" prefix, you > should be able to leave it out. > >> + (find-files "."))) > `find-files' accepts an optional second argument called PRED, so you can > do that instead of the earlier 'filter'. Thanks, I didn't know. Fixed :). > >> + install-dir) >> + (begin >> + (copy-recursively "." install-dir) >> + ;; Remove references to dependencies >> + (delete-file-recursively >> + (string-append install-dir "/node_modules")))) >> + (if (and main >> + (not (file-exists? >> + (string-append >> + install-dir "/" (dirname main))))) >> + (install-files (list main) install-dir)) > ^ > > This should not be needed if we use the 'old' (=non-files) approach of > installing. Do you think it makes sense to pull it into the previous > block that only runs on using the 'files' directive? I put this because also the "main" field from package.json is also guaranteed to be installed by NPM, according to https://docs.npmjs.com/files/package.json#main . Thus if a developer populates the "files" field without including the main file in that list, but they do insert it in the "main" field the file should be installed. Does it make sense? > Thanks for you patience, and thanks again for working on this. > > HTH, > > - Jelle Thank you for your patience in reviewing this patch. I'm attaching an updated version of the second patch. Cheers, Giacomo From 329ad1227ee537a630b3823e8d37db4862e023d5 Mon Sep 17 00:00:00 2001 From: Giacomo Leidi Date: Mon, 21 Sep 2020 22:18:19 +0200 Subject: [PATCH 2/2] guix: Enforce package.json "files" directive. This fixes https://issues.guix.gnu.org/40710 by implementing support for the "files" directive from https://docs.npmjs.com/files/package.json#files . * guix/build/node-build-system.scm (install): Enforce package.json "files" directive. * guix/build-system/node.scm (%node-build-system-modules) (node-build)[modules]: Add (guix glob). --- guix/build-system/node.scm | 4 +- guix/build/node-build-system.scm | 67 +++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/guix/build-system/node.scm b/guix/build-system/node.scm index 05c24c47d5..05bc9f2087 100644 --- a/guix/build-system/node.scm +++ b/guix/build-system/node.scm @@ -42,6 +42,7 @@ registry." `((guix build node-build-system) (guix build json) (guix build union) + (guix glob) ,@%gnu-build-system-modules)) ;; TODO: Might be not needed (define (default-node) @@ -90,7 +91,8 @@ registry." (modules '((guix build node-build-system) (guix build json) (guix build union) - (guix build utils)))) + (guix build utils) + (guix glob)))) "Build SOURCE using NODE and INPUTS." (define builder `(begin diff --git a/guix/build/node-build-system.scm b/guix/build/node-build-system.scm index 7799f03595..6e11d1c142 100644 --- a/guix/build/node-build-system.scm +++ b/guix/build/node-build-system.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2015 David Thompson ;;; Copyright © 2016 Jelle Licht +;;; Copyright © 2020 Giacomo Leidi ;;; ;;; This file is part of GNU Guix. ;;; @@ -22,6 +23,7 @@ #:use-module (guix build json) #:use-module (guix build union) #:use-module (guix build utils) + #:use-module (guix glob) #:use-module (ice-9 match) #:use-module (ice-9 popen) #:use-module (ice-9 regex) @@ -110,18 +112,59 @@ the @file{bin} directory." (#f #f))) (dependencies (match (assoc-ref data "dependencies") (('@ deps ...) deps) - (#f #f)))) + (#f #f))) + (file-list (match (assoc-ref data "files") + (() #f) + ((? list? pattern-list) pattern-list) + (#f #f))) + (patterns + (when file-list + (map (lambda (pattern) + (string->compiled-sglob pattern)) + (append file-list + '("package.json" + ;; These files get installed no + ;; matter the case or extension. + "[rR][eE][aA][dD][mM][eE]*" + "[cC][hH][aA][nN][gG][eE][sS]*" + "[cC][hH][aA][nN][gG][eE][lL][oO][gG]*" + "[hH][iI][sS][tT][oO][rR][yY]*" + "[nN][oO][tT][iI][cC][eE]*"))))) + (main (match (assoc-ref data "main") + ("" #f) + ((? string? main-module) main-module) + (#f #f))) + (install-dir (string-append target "/node_modules/" modulename)) + (install-files (lambda (files) + (for-each (lambda (file) + (install-file + file + (string-append install-dir "/" + (dirname file)))) + files)))) (mkdir-p target) - (copy-recursively "." (string-append target "/node_modules/" modulename)) - ;; Remove references to dependencies - (delete-file-recursively - (string-append target "/node_modules/" modulename "/node_modules")) + (if file-list + (install-files + (find-files "." (lambda (file stat) + (any (lambda (pattern) + (glob-match? pattern + (string-drop file 2))) + patterns)))) + (begin + (copy-recursively "." install-dir) + ;; Remove references to dependencies + (delete-file-recursively + (string-append install-dir "/node_modules")))) + (if (and main + (not (file-exists? + (string-append + install-dir "/" (dirname main))))) + (install-files (list main))) (cond ((string? binary-configuration) (begin (mkdir-p binaries) - (symlink (string-append target "/node_modules/" modulename "/" - binary-configuration) + (symlink (string-append install-dir "/" binary-configuration) (string-append binaries "/" modulename)))) ((list? binary-configuration) (for-each @@ -130,21 +173,19 @@ the @file{bin} directory." ((key . value) (begin (mkdir-p (dirname (string-append binaries "/" key))) - (symlink (string-append target "/node_modules/" modulename "/" - value) + (symlink (string-append install-dir "/" value) (string-append binaries "/" key)))))) - binary-configuration))) + binary-configuration))) (when dependencies (mkdir-p - (string-append target "/node_modules/" modulename "/node_modules")) + (string-append install-dir "/node_modules")) (for-each (lambda (dependency) (let ((dependency (car dependency))) (symlink (string-append (assoc-ref inputs (string-append "node-" dependency)) "/lib/node_modules/" dependency) - (string-append target "/node_modules/" modulename - "/node_modules/" dependency)))) + (string-append install-dir "/node_modules/" dependency)))) dependencies)) #t)) -- 2.28.0