Review for buildNpmPackage #2

yorickvP · 2018-09-26T17:16:16Z

No description provided.

mkaito · 2018-09-26T17:46:04Z

pkgs/buildNpmPackage/default.nix

+    echo making cache
+    node ${./mkcache.js} ${npm-cache-input lock}
+    echo installing
+    npm ci --cache=./npm-cache --offline --script-shell=${shellWrap}/bin/npm-shell-wrap.sh


I was about to tell you to use full command names instead of abbreviations in scripts, but it turns out npm ci is the full command name. Cue traditional npm-inspired facepalm.

pkgs/buildNpmPackage/mkcache.js

lukateras

I will take another pass a bit later.

lukateras · 2018-09-26T17:17:57Z

pkgs/default.nix

@@ -16,6 +16,8 @@ in
    rev = "76dc0f06d21f6063cb7b7d2291b8623da24affa9";
  }) {};

+  buildNpmPackage = callPackage ./buildNpmPackage {};


Would be nicer if this were a separate repo, I don't think it's right to commit JS code into a closure which is supposed to mostly point to stuff.

lukateras · 2018-09-26T17:19:04Z

pkgs/buildNpmPackage/default.nix

+let
+  inherit (builtins) fromJSON toJSON readFile split head elemAt foldl';
+
+  deps-to-fetches = base: deps: builtins.foldl' (a: b: a // (dep-to-fetch b)) base (builtins.attrValues deps);


builtins.foldl' -> foldl', builtins.attrValues -> attrValues, deps-to-fetches -> depsToFetches

lukateras · 2018-09-26T17:19:09Z

pkgs/buildNpmPackage/default.nix

@@ -0,0 +1,58 @@
+{ stdenvNoCC, writeShellScriptBin, writeText, stdenv, fetchurl, makeWrapper, nodejs-10_x }:
+let
+  inherit (builtins) fromJSON toJSON readFile split head elemAt foldl';


with lib;

lukateras · 2018-09-26T17:20:52Z

pkgs/buildNpmPackage/default.nix

+in
+args @ { lockfile, src, buildInputs ? [], ... }:
+let lock = fromJSON (readFile lockfile); in
+stdenv.mkDerivation ({


stdenv.mkDerivation ({ /* ... */ }) -> stdenv.mkDerivation { /* ... */ }

this is stdenv.mkDerivation ({ ... } // { ... } // { ... })

lukateras · 2018-09-26T17:21:12Z

pkgs/buildNpmPackage/default.nix

+  '';
+in
+args @ { lockfile, src, buildInputs ? [], ... }:
+let lock = fromJSON (readFile lockfile); in


let lock = fromJSON (readFile lockFile); in

lukateras · 2018-09-26T17:27:15Z

pkgs/buildNpmPackage/default.nix

+    "${resolved}" = fetchurl {
+      url = resolved;
+      "${hashtype}" = hash;
+    };


This should rather return nameValuePair.

lukateras · 2018-09-26T17:28:27Z

pkgs/buildNpmPackage/default.nix

+  };
+
+  dep-to-fetch = args @ { resolved ? null, dependencies ? {}, ... }:
+    deps-to-fetches (if isNull resolved then {} else dep-own-fetch args) dependencies;


fromDeps (optionalAttrs (resolved != null) (dep2thing args)) dependencies

lukateras · 2018-09-26T17:28:58Z

pkgs/buildNpmPackage/mkcache.js

+module.paths.push(path.join(process.argv[0], "../../lib/node_modules/npm/node_modules"))
+const pacote = require('pacote')
+
+function forall_deps(pkg, fn) {


traverseDeps, walkDeps? forall typically doesn't presume tree traversal.

lukateras · 2018-09-26T17:29:22Z

pkgs/buildNpmPackage/default.nix

+  buildPhase = ''
+    echo making cache
+    node ${./mkcache.js} ${npm-cache-input lock}
+    echo installing


If this is install, everything below it should go into installPhase instead.

lukateras · 2018-09-26T17:30:08Z

pkgs/buildNpmPackage/default.nix

+  XDG_CONFIG_DIRS = ".";
+  NO_UPDATE_NOTIFIER = true;
+  buildPhase = ''
+    echo making cache


Derivations shouldn't print that, it's phases job.

lukateras

Overall, this does not follow Nixpkgs style guidelines, and variable names could be better.

installJavascript check

yorickvP · 2018-10-08T14:31:35Z

@yegortimoshenko Please re-review

…

On Thu, Sep 27, 2018 at 4:02 PM Yegor Timoshenko ***@***.***> wrote: ***@***.**** commented on this pull request. Overall, this does not follow Nixpkgs style guidelines, and variable names could be better. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnfpAsNwMqh_eBV7OFD_tpwIE791I8pks5ufNprgaJpZM4W7H0t> .

pkgs/buildNpmPackage/mkcache.js

obfusk · 2018-10-12T13:04:07Z

pkgs/buildNpmPackage/default.nix

+    exec bash "$@"
+  '';
+in
+args @ { lockfile, src, buildInputs ? [], npmFlags ? [], ... }:


Where is lockfile set?

pkgs/buildNpmPackage/mkcache.js

obfusk · 2018-10-12T13:12:49Z

pkgs/buildNpmPackage/default.nix

+  inherit (builtins) fromJSON toJSON split;
+
+  depsToFetches = deps: concatMap depToFetch (attrValues deps);
+  depFetchOwn = { resolved, integrity, ... }: let


Aren't you also getting non-production dependencies here?

obfusk · 2018-10-12T13:14:42Z

pkgs/buildNpmPackage/default.nix

+  # unpack the .tgz into output directory and add npm wrapper
+  installPhase = ''
+    mkdir -p $out/bin
+    tar xzvf ./${lock.name}-${lock.version}.tgz -C $out --strip-components=1


You could use ${name} (= "${lock.name}-${lock.version}") here.

obfusk · 2018-10-22T18:05:33Z

I suggest closing this in favor of a PR that uses serokell/nix-npm-buildpackage#2 when that's done.

Lucus16 · 2018-10-26T15:56:13Z

Closing in favor of #5

yorickvP added 2 commits September 26, 2018 01:38

add buildNpmPackage

33ca36a

buildNpmPackage: add makeWrapper

b11dd41

mkaito reviewed Sep 26, 2018

View reviewed changes

pkgs/buildNpmPackage/mkcache.js Show resolved Hide resolved

lukateras suggested changes Sep 27, 2018

View reviewed changes

lukateras reviewed Sep 27, 2018

View reviewed changes

yorickvP added 3 commits September 30, 2018 10:08

buildNpmPackage: change after code review

b418dd5

buildNpmPackage: change eval to runHook

d8cbeab

buildNpmPackage: remove environment variables (because nix-shell), fix

0aec71f

installJavascript check

obfusk reviewed Oct 12, 2018

View reviewed changes

pkgs/buildNpmPackage/mkcache.js Show resolved Hide resolved

obfusk reviewed Oct 12, 2018

View reviewed changes

pkgs/buildNpmPackage/mkcache.js Show resolved Hide resolved

obfusk reviewed Oct 12, 2018

View reviewed changes

Lucus16 closed this Oct 26, 2018

serokell-bot deleted the yorickvp-review branch October 26, 2018 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review for buildNpmPackage #2

Review for buildNpmPackage #2

yorickvP commented Sep 26, 2018

mkaito Sep 26, 2018

lukateras left a comment

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

yorickvP Oct 1, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras Sep 26, 2018

lukateras left a comment

yorickvP commented Oct 8, 2018 via email

obfusk Oct 12, 2018

obfusk Oct 12, 2018

obfusk Oct 12, 2018 •

edited

Loading

obfusk commented Oct 22, 2018 •

edited

Loading

Lucus16 commented Oct 26, 2018

Review for buildNpmPackage #2

Review for buildNpmPackage #2

Conversation

yorickvP commented Sep 26, 2018

Choose a reason for hiding this comment

lukateras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras left a comment

Choose a reason for hiding this comment

yorickvP commented Oct 8, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obfusk Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

obfusk commented Oct 22, 2018 • edited Loading

Lucus16 commented Oct 26, 2018

obfusk Oct 12, 2018 •

edited

Loading

obfusk commented Oct 22, 2018 •

edited

Loading