diff mbox series

[bug#51838,v6,05/41] guix: node-build-system: Add 'delete-dependencies' helper function.

Message ID 01d25375840e1611224f1407431dd03508f5b07a.camel@gmail.com
State Accepted
Headers show
Series None | expand

Commit Message

Liliana Marie Prikler Jan. 6, 2022, 4:50 p.m. UTC
Hi Leo,

Am Mittwoch, dem 05.01.2022 um 15:02 -0500 schrieb Leo Famulari:
> On Wed, Jan 05, 2022 at 02:08:30PM -0500, Philip McGrath wrote:
> > I would consider it very regrettable if this patch series were to
> > continue to be blocked by stylistic considerations in the
> > implementation of
> > unexported helper functions.
> 
> Agreed.
> 
> Is there a concrete problem with these patches? Or will they work as
> specified for Guix packagers?
> 
> Let's remember that the primary goal of code review is to bring a
> contribution into the codebase.
I'm currently in the process of applying my own checklist, see
<https://issues.guix.gnu.org/issue/51838/raw/352>

Each of my fixup commits is a change on top of Philip's corresponding
commit (or at least I hope so, I haven't squashed them yet) and delete-
dependencies.patch shows the savings in the rest of the series.  I so
far successfully built node-irc and node-serialport with these changes
applied on top.

It'll take some time to check the rest of the toplevel packages and
thereafter I'll squash and cleanup commit messages.

So from my position, everything is about to be done.  Should I resend
this as v8 for a complete check that I'm not doing anything harmful or
should I go ahead and commit once I'm done?

Cheers

Comments

Leo Famulari Jan. 6, 2022, 5:28 p.m. UTC | #1
On Thu, Jan 06, 2022 at 05:50:08PM +0100, Liliana Marie Prikler wrote:
> So from my position, everything is about to be done.  Should I resend
> this as v8 for a complete check that I'm not doing anything harmful or
> should I go ahead and commit once I'm done?

I think we should wait to see what Philip thinks about the revisions.

I don't have specific feedback about the patches. Instead, I wanted to
give a nudge towards resolution of the "ticket", which I think is
important in terms of how contributors feel about their work with Guix.
diff mbox series

Patch

diff --git a/gnu/packages/node-xyz.scm b/gnu/packages/node-xyz.scm
index 1f51c0d636..990b26a689 100644
--- a/gnu/packages/node-xyz.scm
+++ b/gnu/packages/node-xyz.scm
@@ -297,9 +297,7 @@  (define-public node-semver
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("tap")))))))
+             (delete-dependencies '("^tap$")))))
        ;; FIXME: Tests depend on node-tap
        #:tests? #f))
     (home-page "https://github.com/npm/node-semver")
@@ -329,9 +327,7 @@  (define-public node-wrappy
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("tap")))))))))
+             (delete-dependencies '("^tap$")))))))
     (home-page "https://github.com/npm/wrappy")
     (synopsis "Callback wrapping utility")
     (description "@code{wrappy} is a utility for Node.js to wrap callbacks.")
@@ -356,9 +352,7 @@  (define-public node-once
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("tap")))))))
+             (delete-dependencies '("^tap$")))))
        ;; FIXME: Tests depend on node-tap
        #:tests? #f))
     (inputs
@@ -391,9 +385,7 @@  (define-public node-inherits
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("tap")))))))
+             (delete-dependencies '("^tap$")))))
        ;; FIXME: Tests depend on node-tap
        #:tests? #f))
     (home-page
@@ -425,10 +417,7 @@  (define-public node-safe-buffer
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta  '("tape"
-                                                  "standard")))))))
+             (delete-dependencies '("^tape$" "^standard$")))))
        #:tests? #f))
     (home-page
      "https://github.com/feross/safe-buffer")
@@ -458,12 +447,8 @@  (define-public node-string-decoder
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      '("tap"
-                                        "core-util-is"
-                                        "babel-polyfill")))))))
+             (delete-dependencies
+              '("^tap$" "^core-util-is$" "^babel-polyfill$")))))
        ;; FIXME: Tests depend on node-tap
        #:tests? #f))
     (inputs
@@ -497,29 +482,22 @@  (define-public node-readable-stream
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("@babel/cli"
-                                        "@babel/core"
-                                        "@babel/polyfill"
-                                        "@babel/preset-env"
-                                        "airtap"
-                                        "assert"
-                                        "bl"
-                                        "deep-strict-equal"
-                                        "events.once"
-                                        "glob"
-                                        "gunzip-maybe"
-                                        "hyperquest"
-                                        "lolex"
-                                        "nyc"
-                                        "pump"
-                                        "rimraf"
-                                        "tap"
-                                        "tape"
-                                        "tar-fs"
-                                        "util-promisify")))))))
+             (delete-dependencies `("^@babel/"
+                                    "^airtap$"
+                                    "^assert$"
+                                    "^bl$"
+                                    "^deep-strict-equal$"
+                                    "^events\\.once$"
+                                    "^glob$"
+                                    "^gunzip-maybe$"
+                                    "^hyperquest$"
+                                    "^lolex$"
+                                    "^nyc$"
+                                    "^pump$"
+                                    "^rimraf$"
+                                    "^tape?$"
+                                    "^tar-fs$"
+                                    "^util-promisify$")))))
        #:tests? #f))
     (inputs
      (list node-util-deprecate node-string-decoder node-inherits))
@@ -554,10 +532,7 @@  (define-public node-irc-colors
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("istanbul"
-                                                 "vows")))))))
+             (delete-dependencies `("^istanbul$" "^vows$")))))
        #:tests? #f))
     (home-page "https://github.com/fent/irc-colors.js")
     (synopsis "Node.js module providing color and formatting for IRC")
@@ -586,12 +561,8 @@  (define-public node-irc
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("ansi-color"
-                                                 "faucet"
-                                                 "jscs"
-                                                 "tape")))))))
+             (delete-dependencies
+              `("^ansi-color$" "^faucet$" "^jscs$" "^tape$")))))
        #:tests? #f))
     (inputs
      (list node-irc-colors))
@@ -620,17 +591,14 @@  (define-public node-nan
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies
-                  pkg-meta
-                  '("bindings"
-                    "commander"
-                    "glob"
-                    "request"
-                    "node-gyp" ;; would be needed for tests
-                    "tap"
-                    "xtend")))))))
+             (delete-dependencies
+              '("^bindings$"
+                "^commander$"
+                "^glob$"
+                "^request$"
+                "^node-gyp$" ;; would be needed for tests
+                "^tap$"
+                "^xtend$")))))
        ;; tests need tap and other dependencies
        #:tests? #f))
     (inputs
@@ -673,21 +641,11 @@  (define-public node-addon-api
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("benchmark"
-                                        "bindings"
-                                        "clang-format"
-                                        "eslint"
-                                        "eslint-config-semistandard"
-                                        "eslint-config-standard"
-                                        "eslint-plugin-import"
-                                        "eslint-plugin-node"
-                                        "eslint-plugin-promise"
-                                        "fs-extra"
-                                        "path"
-                                        "pre-commit"))))))
+             (delete-dependencies
+              `("^benchmark$" "^bindings$" "^clang-format$"
+                "^eslint"
+                "^fs-extra$" "^path$" "^pre-commit$"))
+             ))
          (add-after 'unpack 'skip-js-tests
            ;; We can't run the js-based tests,
            ;; but we can still do the C++ parts
@@ -767,29 +725,26 @@  (define-public node-sqlite3
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies
-                  pkg-meta
-                  `(;; Normally, this is "built" using @mapbox/node-pre-gyp,
-                    ;; which publishes or downloads pre-built binaries or
-                    ;; falls back to building from source.  Here, we patch out
-                    ;; all of that and just build directly.  It might be
-                    ;; better to patch a version of @mapbox/node-pre-gyp that
-                    ;; always builds from source, as Debian does, but there
-                    ;; are a number of dependencies that need to be packaged
-                    ;; or removed.
-                    "@mapbox/node-pre-gyp"
-                    "node-pre-gyp" ;; deprecated name still used in some places
-                    "aws-sdk"
-                    "@mapbox/cloudfriend"
-                    ;; Confusingly, this is only a dependency because of
-                    ;; @mapbox/node-pre-gyp: with that removed,
-                    ;; npm will use its own copy:
-                    "node-gyp"
-                    ;; These we'd like, we just don't have them yet:
-                    "eslint"
-                    "mocha"))))))
+             (delete-dependencies
+              `(;; Normally, this is "built" using @mapbox/node-pre-gyp,
+                ;; which publishes or downloads pre-built binaries or
+                ;; falls back to building from source.  Here, we patch out
+                ;; all of that and just build directly.  It might be
+                ;; better to patch a version of @mapbox/node-pre-gyp that
+                ;; always builds from source, as Debian does, but there
+                ;; are a number of dependencies that need to be packaged
+                ;; or removed.
+                "^@mapbox/node-pre-gyp$"
+                "^node-pre-gyp$" ;; deprecated name still used in some places
+                "^aws-sdk$"
+                "^@mapbox/cloudfriend$"
+                ;; Confusingly, this is only a dependency because of
+                ;; @mapbox/node-pre-gyp: with that removed,
+                ;; npm will use its own copy:
+                "^node-gyp$"
+                ;; These we'd like, we just don't have them yet:
+                "^eslint$"
+                "^mocha$"))))
          (add-before 'configure 'npm-config-sqlite
            ;; We need this step even if we do replace @mapbox/node-pre-gyp
            ;; because the package expects to build its bundled sqlite
@@ -885,24 +840,12 @@  (define-public node-file-uri-to-path
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("@types/mocha"
-                                        "@types/node"
-                                        "@typescript-eslint/eslint-plugin"
-                                        "@typescript-eslint/parser"
-                                        "cpy-cli"
-                                        "eslint"
-                                        "eslint-config-airbnb"
-                                        "eslint-config-prettier"
-                                        "eslint-import-resolver-typescript"
-                                        "eslint-plugin-import"
-                                        "eslint-plugin-jsx-a11y"
-                                        "eslint-plugin-react"
-                                        "mocha"
-                                        "rimraf"
-                                        "typescript"))))))
+             (delete-dependencies
+              `("^@types/mocha$" "^@types/node$"
+                "typescript"
+                "eslint"
+                "^cpy-cli$"
+                "^mocha$" "^rimraf$"))))
          (replace 'build
            (lambda* (#:key inputs native-inputs #:allow-other-keys)
              (copy-recursively "src" "dist")
@@ -995,14 +938,12 @@  (define-public node-ms
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("eslint"
-                                                 "expect.js"
-                                                 "husky"
-                                                 "lint-staged"
-                                                 "mocha"
-                                                 "prettier")))))))
+             (delete-dependencies `("^eslint$"
+                                    "^expect\\.js$"
+                                    "^husky$"
+                                    "^lint-staged$"
+                                    "^mocha$"
+                                    "^prettier$")))))
        #:tests? #f))
     (home-page "https://github.com/vercel/ms")
     (synopsis "Tiny millisecond conversion utility")
@@ -1045,20 +986,15 @@  (define-public node-debug
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("brfs"
-                                                 "browserify"
-                                                 "coveralls"
-                                                 "istanbul"
-                                                 "karma"
-                                                 "karma-browserify"
-                                                 "karma-chrome-launcher"
-                                                 "karma-mocha"
-                                                 "mocha"
-                                                 "mocha-lcov-reporter"
-                                                 "xo"
-                                                 "supports-color")))))))
+             (delete-dependencies
+              `("^brfs$"
+                "^browserify$"
+                "^coveralls$"
+                "^istanbul$"
+                "^karma"
+                "^mocha"
+                "^xo$"
+                "^supports-color$")))))
        #:tests? #f))
     (home-page "https://github.com/debug-js/debug")
     (synopsis "Lightweight debugging utility for Node.js and the browser")
@@ -1173,12 +1109,10 @@  (define-public node-serialport-bindings
              (chdir "packages/bindings")))
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("prebuild-install"
-                                                 ;; devDependencies
-                                                 "@serialport/binding-mock"
-                                                 "node-abi"))))))
+             (delete-dependencies `("^prebuild-install$"
+                                    ;; devDependencies
+                                    "^@serialport/binding-mock$"
+                                    "^node-abi$"))))
          (add-after 'chdir 'avoid-prebuild-install
            (lambda args
              (with-atomic-json-file-replacement "package.json"
@@ -1330,11 +1264,8 @@  (define-public node-serialport-stream
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `(;; devDependencies
-                                        "@serialport/binding-mock"))))))
+             (delete-dependencies `(;; devDependencies
+                                    "^@serialport/binding-mock$"))))
          (add-after 'unpack 'chdir
            (lambda args
              (chdir "packages/stream"))))
@@ -1370,10 +1301,8 @@  (define-public node-serialport
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("@serialport/binding-mock"))))))
+             (delete-dependencies `("^@serialport/binding-mock$"))
+             ))
          (add-after 'unpack 'chdir
            (lambda args
              (chdir "packages/serialport"))))
diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index 5a4401e779..49e6323a9d 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -348,9 +348,7 @@  (define-public node-semver-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("tap")))))))))
+             (delete-dependencies '("^tap$")))))))
     (home-page "https://github.com/npm/node-semver")
     (properties '((hidden? . #t)))
     (synopsis "Parses semantic versions strings")
@@ -381,13 +379,11 @@  (define-public node-ms-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta '("eslint"
-                                                 "expect.js"
-                                                 "husky"
-                                                 "lint-staged"
-                                                 "mocha")))))))))
+             (delete-dependencies '("^eslint$"
+                                    "^expect\\.js$"
+                                    "^husky$"
+                                    "^lint-staged$"
+                                    "^mocha$")))))))
     (home-page "https://github.com/zeit/ms#readme")
     (properties '((hidden? . #t)))
     (synopsis "Tiny millisecond conversion utility")
@@ -417,10 +413,7 @@  (define-public node-binary-search-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("chai"
-                                                 "mocha")))))))))
+             (delete-dependencies `("^(chai\|mocha)$")))))))
     (home-page "https://github.com/darkskyapp/binary-search#readme")
     (properties '((hidden? . #t)))
     (synopsis "Tiny binary search function with comparators")
@@ -449,20 +442,13 @@  (define-public node-debug-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("brfs"
-                                        "browserify"
-                                        "coveralls"
-                                        "istanbul"
-                                        "karma"
-                                        "karma-browserify"
-                                        "karma-chrome-launcher"
-                                        "karma-mocha"
-                                        "mocha"
-                                        "mocha-lcov-reporter"
-                                        "xo")))))))))
+             (delete-dependencies `("^brfs$"
+                                    "^browserify$"
+                                    "^coveralls$"
+                                    "^istanbul$"
+                                    "^karma"
+                                    "^mocha"
+                                    "^xo$")))))))
     (inputs (list node-ms-bootstrap))
     (home-page "https://github.com/visionmedia/debug#readme")
     (properties '((hidden? . #t)))
@@ -517,15 +503,12 @@  (define-public node-llparse-builder-bootstrap
        #:phases
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
-           (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("@types/mocha"
-                                                 "@types/node"
-                                                 "mocha"
-                                                 "ts-node"
-                                                 "tslint"
-                                                 "typescript"))))))
+           (lambda _
+             (delete-dependencies `("^@types/node$"
+                                    "mocha$"
+                                    "^ts-node$"
+                                    "^tslint$"
+                                    "^typescript$"))))
          (replace 'build
            (lambda* (#:key inputs #:allow-other-keys)
              (let ((esbuild (search-input-file inputs "/bin/esbuild")))
@@ -581,15 +564,13 @@  (define-public node-llparse-frontend-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta `("@types/debug"
-                                                 "@types/mocha"
-                                                 "@types/node"
-                                                 "mocha"
-                                                 "ts-node"
-                                                 "tslint"
-                                                 "typescript"))))))
+             (delete-dependencies `("^@types/debug$"
+                                    "^@types/mocha$"
+                                    "^@types/node$"
+                                    "^mocha$"
+                                    "^ts-node$"
+                                    "^tslint$"
+                                    "^typescript$"))))
          (replace 'build
            (lambda* (#:key inputs #:allow-other-keys)
              (let ((esbuild (search-input-file inputs "/bin/esbuild")))
@@ -644,18 +625,15 @@  (define-public node-llparse-bootstrap
        (modify-phases %standard-phases
          (add-after 'patch-dependencies 'delete-dependencies
            (lambda args
-             (with-atomic-json-file-replacement "package.json"
-               (lambda (pkg-meta)
-                 (delete-dependencies pkg-meta
-                                      `("@types/debug"
-                                        "@types/mocha"
-                                        "@types/node"
-                                        "esm"
-                                        "llparse-test-fixture"
-                                        "mocha"
-                                        "ts-node"
-                                        "tslint"
-                                        "typescript"))))))
+             (delete-dependencies `("^@types/debug$"
+                                    "^@types/mocha$"
+                                    "^@types/node$"
+                                    "^esm$"
+                                    "^llparse-test-fixture$"
+                                    "^mocha$"
+                                    "^ts-node$"
+                                    "^tslint$"
+                                    "^typescript$"))))
          (replace 'build
            (lambda* (#:key inputs #:allow-other-keys)
              (let ((esbuild (search-input-file inputs "/bin/esbuild")))