From 7d8124f339aeffb7b444f460b581d13668f953c3 Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 20:28:18 +0200 Subject: [PATCH 1/8] language/node: set npm loglevel to max -ddd to match the loglevel used in npm_debug.log (previous --verbose loglevel would match -dd) --- Library/Homebrew/language/node.rb | 4 ++-- Library/Homebrew/test/language/node_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 01d41041be..6aaeacbea5 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -42,7 +42,7 @@ module Language # npm install args for global style module format installed into libexec %W[ - --verbose + -ddd --global --prefix=#{libexec} #{Dir.pwd}/#{pack} @@ -52,7 +52,7 @@ module Language def self.local_npm_install_args setup_npm_environment # npm install args for local style module format - ["--verbose"] + ["-ddd"] end end end diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index b2114bc4fd..4116c85810 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -41,6 +41,6 @@ describe Language::Node do specify "#local_npm_install_args" do resp = subject.local_npm_install_args - expect(resp).to include("--verbose") + expect(resp).to include("-ddd") end end From 02113e2714aae818ee2c43e203137b3cd0b61ced Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 20:35:48 +0200 Subject: [PATCH 2/8] language/node: build native addons from source By telling node-pre-gyp and prebuild to don't pull prebuild binaries and instead build them from source. This still may not work for some custom third party scripts for pulling prebuild binaries. --- Library/Homebrew/language/node.rb | 6 +++++- Library/Homebrew/test/language/node_spec.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 6aaeacbea5..98d376766b 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -44,6 +44,7 @@ module Language %W[ -ddd --global + --build-from-source --prefix=#{libexec} #{Dir.pwd}/#{pack} ] @@ -52,7 +53,10 @@ module Language def self.local_npm_install_args setup_npm_environment # npm install args for local style module format - ["-ddd"] + %w[ + -ddd + --build-from-source + ] end end end diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index 4116c85810..55bb1574e1 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -41,6 +41,6 @@ describe Language::Node do specify "#local_npm_install_args" do resp = subject.local_npm_install_args - expect(resp).to include("-ddd") + expect(resp).to include("-ddd", "--build-from-source") end end From 495520a1f9d55712a6a0d3eae6123fda60dbaeae Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 21:33:12 +0200 Subject: [PATCH 3/8] language/node: make packname detection more robust This fixes some edge cases where verbose output from a prepublish script could break our npm pack package name detection code by only using the last line of the output printed by npm itself containing the desired package name. --- Library/Homebrew/language/node.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 98d376766b..86874e385c 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -11,8 +11,10 @@ module Language # directory, consequently breaking that assumption. We require a tarball # because npm install creates a "real" installation when fed a tarball. output = Utils.popen_read("npm pack").chomp - raise "npm failed to pack #{Dir.pwd}" unless $CHILD_STATUS.exitstatus.zero? - output + if !$CHILD_STATUS.exitstatus.zero? || output.lines.empty? + raise "npm failed to pack #{Dir.pwd}" + end + output.lines.last.chomp end def self.setup_npm_environment From fe39dbb78c2d62111358bfa00b6219ef43db7a4f Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 21:34:05 +0200 Subject: [PATCH 4/8] language/node: log verbose npm pack output This makes npm pack to log verbose debug output to the console to simplify debugging npm pack failures. Refs: https://github.com/Homebrew/brew/pull/2820#discussion_r123890729 Prevously Utils.popen_read swallowed all debug output. --- Library/Homebrew/language/node.rb | 4 +++- Library/Homebrew/test/language/node_spec.rb | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 86874e385c..02885d62fe 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -10,7 +10,9 @@ module Language # fed to `npm install` only symlinks are created linking back to that # directory, consequently breaking that assumption. We require a tarball # because npm install creates a "real" installation when fed a tarball. - output = Utils.popen_read("npm pack").chomp + pack_cmd = "npm pack -ddd" + ohai pack_cmd + output = `#{pack_cmd}` if !$CHILD_STATUS.exitstatus.zero? || output.lines.empty? raise "npm failed to pack #{Dir.pwd}" end diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index 55bb1574e1..b822c2350d 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -24,18 +24,28 @@ describe Language::Node do describe "#std_npm_install_args" do npm_install_arg = "libexec" + npm_pack_cmd = "npm pack -ddd" it "raises error with non zero exitstatus" do + allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("error msg") + allow_any_instance_of(Process::Status).to receive(:exitstatus).and_return(42) + allow_any_instance_of(nil::NilClass).to receive(:exitstatus).and_return(42) + expect { subject.std_npm_install_args(npm_install_arg) }.to \ + raise_error("npm failed to pack #{Dir.pwd}") + end + + it "raises error with empty npm pack output" do + allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("") expect { subject.std_npm_install_args(npm_install_arg) }.to \ raise_error("npm failed to pack #{Dir.pwd}") end it "does not raise error with a zero exitstatus" do - allow(Utils).to receive(:popen_read).with("npm pack").and_return("pack") + allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("pack.tgz") allow_any_instance_of(Process::Status).to receive(:exitstatus).and_return(0) allow_any_instance_of(nil::NilClass).to receive(:exitstatus).and_return(0) resp = subject.std_npm_install_args(npm_install_arg) - expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack") + expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack.tgz") end end From 466fe9841a9af915fe78c1fd8648a745610f942d Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Thu, 29 Jun 2017 20:24:15 +0200 Subject: [PATCH 5/8] language/node: npm pack ignore prepublish scripts This tells npm pack to don't run prepublish scripts at all. I think this is the best default because: * most modules don't have a prepublish script at all and aren't affected by this change * most prepublish scripts are calling devDeps, which would fail in our case, because (dev)Deps aren't installed at npm pack time until #2820 gets resolved * we favor npm registry tarball for formula downloads, which are already prepublished, so we would in the best case needlessly run prepublish a second time and in the worst case it would fail (because a clean step is required before running prepublish a second time in a row) * This change does the right thing for >99% of all the packages and would only affect packages with prepublish scripts downloaded from a non-npm registry tarball (like github tarballs) and with a prepublish script wich does no't require any devDep (unlike for cross platform) --- Library/Homebrew/language/node.rb | 2 +- Library/Homebrew/test/language/node_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 02885d62fe..03fa9e5220 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -10,7 +10,7 @@ module Language # fed to `npm install` only symlinks are created linking back to that # directory, consequently breaking that assumption. We require a tarball # because npm install creates a "real" installation when fed a tarball. - pack_cmd = "npm pack -ddd" + pack_cmd = "npm pack -ddd --ignore-scripts" ohai pack_cmd output = `#{pack_cmd}` if !$CHILD_STATUS.exitstatus.zero? || output.lines.empty? diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index b822c2350d..e4ed9ee1f4 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -24,7 +24,7 @@ describe Language::Node do describe "#std_npm_install_args" do npm_install_arg = "libexec" - npm_pack_cmd = "npm pack -ddd" + npm_pack_cmd = "npm pack -ddd --ignore-scripts" it "raises error with non zero exitstatus" do allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("error msg") From 6baea2543aef21d996f04a3eff96185c4a6feba1 Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 21:47:47 +0200 Subject: [PATCH 6/8] language/node: set cache config via argument instead of writing a .npmrc file, which simplifies the code. npm_cache_config is still preserved for backwarts compatiblility and usage int he kibana@n formulas in core. --- Library/Homebrew/language/node.rb | 16 +++++++--------- Library/Homebrew/test/language/node_spec.rb | 19 +++++++++---------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 03fa9e5220..0d02a2ad01 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -1,7 +1,7 @@ module Language module Node def self.npm_cache_config - "cache=#{HOMEBREW_CACHE}/npm_cache\n" + "cache=#{HOMEBREW_CACHE}/npm_cache" end def self.pack_for_installation @@ -20,13 +20,9 @@ module Language end def self.setup_npm_environment - npmrc = Pathname.new("#{ENV["HOME"]}/.npmrc") - # only run setup_npm_environment once per formula - return if npmrc.exist? - # explicitly set npm's cache path to HOMEBREW_CACHE/npm_cache to fix - # issues caused by overriding $HOME (long build times, high disk usage) - # https://github.com/Homebrew/brew/pull/37#issuecomment-208840366 - npmrc.write npm_cache_config + # guard that this is only run once + return if @env_set + @env_set = true # explicitly use our npm and node-gyp executables instead of the user # managed ones in HOMEBREW_PREFIX/lib/node_modules which might be broken begin @@ -49,6 +45,7 @@ module Language -ddd --global --build-from-source + --#{npm_cache_config} --prefix=#{libexec} #{Dir.pwd}/#{pack} ] @@ -57,9 +54,10 @@ module Language def self.local_npm_install_args setup_npm_environment # npm install args for local style module format - %w[ + %W[ -ddd --build-from-source + --#{npm_cache_config} ] end end diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index e4ed9ee1f4..a8f95fe9a0 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -2,22 +2,21 @@ require "language/node" describe Language::Node do describe "#setup_npm_environment" do - it "does nothing when npmrc exists" do - expect(subject.setup_npm_environment).to be_nil - end - - it "calls prepend_path when node formula exists and npmrc does not exist" do + it "calls prepend_path when node formula exists only during the first call" do node = formula "node" do url "node-test" end stub_formula_loader(node) - allow_any_instance_of(Pathname).to receive(:exist?).and_return(false) expect(ENV).to receive(:prepend_path) - subject.setup_npm_environment + subject.instance_variable_set(:@env_set, false) + expect(subject.setup_npm_environment).to be_nil + + expect(subject.instance_variable_get(:@env_set)).to eq(true) + expect(ENV).not_to receive(:prepend_path) + expect(subject.setup_npm_environment).to be_nil end - it "does not call prepend_path when node formula does not exist but npmrc exists" do - allow_any_instance_of(Pathname).to receive(:exist?).and_return(false) + it "does not call prepend_path when node formula does not exist" do expect(subject.setup_npm_environment).to be_nil end end @@ -51,6 +50,6 @@ describe Language::Node do specify "#local_npm_install_args" do resp = subject.local_npm_install_args - expect(resp).to include("-ddd", "--build-from-source") + expect(resp).to include("-ddd", "--build-from-source", "--cache=#{HOMEBREW_CACHE}/npm_cache") end end From 7910e4a5b4eabe77481ba29a13090367363285f8 Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Mon, 26 Jun 2017 23:22:11 +0200 Subject: [PATCH 7/8] language/node update documentation --- docs/Node-for-Formula-Authors.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/Node-for-Formula-Authors.md b/docs/Node-for-Formula-Authors.md index b740367537..a3d0624f2d 100644 --- a/docs/Node-for-Formula-Authors.md +++ b/docs/Node-for-Formula-Authors.md @@ -30,7 +30,7 @@ Node modules which are compatible with the latest Node version should declare a depends_on "node" ``` -If your formula requires being executed with an older Node version you must vendor this older Node version as done in the [`kibana` formula](https://github.com/Homebrew/homebrew-core/blob/c6202f91a129e2f994d904f299a308cc6fbd58e5/Formula/kibana.rb). +If your formula requires being executed with an older Node version you should use one of the versioned node formulae (e.g. `node@6`). ### Special requirements for native addons @@ -47,8 +47,8 @@ Also note that such a formula would only be compatible with the same Node major Node modules should be installed to `libexec`. This prevents the Node modules from contaminating the global `node_modules`, which is important so that npm doesn't try to manage Homebrew-installed Node modules. In the following we distinguish between two types of Node modules using formulae: -* formulae for standard Node modules compatible with npm's global module format which should use [`std_npm_install_args`](#installing-global-style-modules-with-std_npm_install_args-to-libexec) (like [`azure-cli`](https://github.com/Homebrew/homebrew-core/blob/d93fe9ba3bcc9071b699c8da4e7d733518d3337e/Formula/azure-cli.rb) or [`autocode`](https://github.com/Homebrew/homebrew-core/blob/1a670a6269e1e07f86683c2d164977c9bd8a3fb6/Formula/autocode.rb)) and -* formulae where the `npm install` step is only one of multiple not exclusively Node related install steps (not compatible with npm's global module format) which have to use [`local_npm_install_args`](#installing-module-dependencies-locally-with-local_npm_install_args) (like [`elixirscript`](https://github.com/Homebrew/homebrew-core/blob/ec1e40d37e81af63122a354f0101c377f6a4e66d/Formula/elixirscript.rb) or [`kibana`](https://github.com/Homebrew/homebrew-core/blob/c6202f91a129e2f994d904f299a308cc6fbd58e5/Formula/kibana.rb)) +* formulae for standard Node modules compatible with npm's global module format which should use [`std_npm_install_args`](#installing-global-style-modules-with-std_npm_install_args-to-libexec) (like [`azure-cli`](https://github.com/Homebrew/homebrew-core/blob/0f3b27d252b8112c744e0460d871cfe1def6b993/Formula/azure-cli.rb) or [`webpack`](https://github.com/Homebrew/homebrew-core/blob/6282879973d569962e63da7c81ac4623e1a8336b/Formula/webpack.rb)) and +* formulae where the `npm install` call is not the only required install step (e.g. need to compile non-JavaScript sources too) have to use [`local_npm_install_args`](#installing-module-dependencies-locally-with-local_npm_install_args) (like [`elixirscript`](https://github.com/Homebrew/homebrew-core/blob/4bb491b7b246830aed57b97348a17e9401374978/Formula/elixirscript.rb) Both methods have in common that they are setting the correct environment for using npm inside Homebrew and are returning the arguments for invoking `npm install` for their specific use cases. This includes fixing an important edge case with the npm cache (caused by Homebrew's redirection of `HOME` during the build and test process) by using our own custom `npm_cache` inside `HOMEBREW_CACHE`, which would otherwise result in very long build times and high disk space usage. @@ -72,6 +72,8 @@ This will install your Node module in npm's global module style with a custom pr bin.install_symlink Dir["#{libexec}/bin/*"] ``` +**Note:** Because of a required workaround for `npm@5` calling `npm pack` we currently don't support installing modules (from non-npm registry tarballs), which require a prepublish step (e.g. for transpiling sources). See [Homebrew/brew#2820](https://github.com/Homebrew/brew/pull/2820) for more information. + ### Installing module dependencies locally with `local_npm_install_args` In your formula's `install` method, do any installation steps which need to be done before the `npm install` step and then `cd` to the top level of the included Node module. Then, use `system` with `Language::Node.local_npm_install_args` to invoke `npm install` like: From 5e00c277ce45d6af497ec135faf54571c340e704 Mon Sep 17 00:00:00 2001 From: Christian Moritz Date: Thu, 29 Jun 2017 20:49:29 +0200 Subject: [PATCH 8/8] partly revert log verbose npm pack output commit to fix issues with shwoing npm debug output even on non-verbose install runs. --- Library/Homebrew/language/node.rb | 5 ++--- Library/Homebrew/test/language/node_spec.rb | 10 ++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/language/node.rb b/Library/Homebrew/language/node.rb index 0d02a2ad01..eaadc54fc7 100644 --- a/Library/Homebrew/language/node.rb +++ b/Library/Homebrew/language/node.rb @@ -10,9 +10,8 @@ module Language # fed to `npm install` only symlinks are created linking back to that # directory, consequently breaking that assumption. We require a tarball # because npm install creates a "real" installation when fed a tarball. - pack_cmd = "npm pack -ddd --ignore-scripts" - ohai pack_cmd - output = `#{pack_cmd}` + pack_cmd = "npm pack --ignore-scripts" + output = Utils.popen_read(pack_cmd) if !$CHILD_STATUS.exitstatus.zero? || output.lines.empty? raise "npm failed to pack #{Dir.pwd}" end diff --git a/Library/Homebrew/test/language/node_spec.rb b/Library/Homebrew/test/language/node_spec.rb index a8f95fe9a0..5ddbde9446 100644 --- a/Library/Homebrew/test/language/node_spec.rb +++ b/Library/Homebrew/test/language/node_spec.rb @@ -23,10 +23,10 @@ describe Language::Node do describe "#std_npm_install_args" do npm_install_arg = "libexec" - npm_pack_cmd = "npm pack -ddd --ignore-scripts" + npm_pack_cmd = "npm pack --ignore-scripts" it "raises error with non zero exitstatus" do - allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("error msg") + allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return("error msg") allow_any_instance_of(Process::Status).to receive(:exitstatus).and_return(42) allow_any_instance_of(nil::NilClass).to receive(:exitstatus).and_return(42) expect { subject.std_npm_install_args(npm_install_arg) }.to \ @@ -34,13 +34,15 @@ describe Language::Node do end it "raises error with empty npm pack output" do - allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("") + allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return("") + allow_any_instance_of(Process::Status).to receive(:exitstatus).and_return(0) + allow_any_instance_of(nil::NilClass).to receive(:exitstatus).and_return(0) expect { subject.std_npm_install_args(npm_install_arg) }.to \ raise_error("npm failed to pack #{Dir.pwd}") end it "does not raise error with a zero exitstatus" do - allow(Language::Node).to receive(:`).with(npm_pack_cmd).and_return("pack.tgz") + allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return("pack.tgz") allow_any_instance_of(Process::Status).to receive(:exitstatus).and_return(0) allow_any_instance_of(nil::NilClass).to receive(:exitstatus).and_return(0) resp = subject.std_npm_install_args(npm_install_arg)