test: build test add-ons like third-party add-ons

Until now we built add-ons by pointing node-gyp at the src/ directory.
We've had at least one DOA release where add-ons were broken because of
a header dependency issue that wasn't caught because we build our test
add-ons in a non-standard way.

This commit does the following:

* Use tools/install.py to install the headers to test/addons/include.
* Add a script to build everything in test/addons.
* Remove the pile-up of hacks from the Makefile.

Refs: nodejs#11628

Makefile

@@ -66,18 +66,13 @@ endif
 # to check for changes.
 .PHONY: $(NODE_EXE) $(NODE_G_EXE)
 
-# The -r/-L check stops it recreating the link if it is already in place,
-# otherwise $(NODE_EXE) being a .PHONY target means it is always re-run.
-# Without the check there is a race condition between the link being deleted
-# and recreated which can break the addons build when running test-ci
-# See comments on the build-addons target for some more info
 $(NODE_EXE): config.gypi out/Makefile
 	$(MAKE) -C out BUILDTYPE=Release V=$(V)
-	if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Release/$(NODE_EXE) $@; fi
+	ln -fs out/Release/$(NODE_EXE) $@
 
 $(NODE_G_EXE): config.gypi out/Makefile
 	$(MAKE) -C out BUILDTYPE=Debug V=$(V)
-	if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Debug/$(NODE_EXE) $@; fi
+	ln -fs out/Debug/$(NODE_EXE) $@
 
 out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
               deps/zlib/zlib.gyp deps/v8/gypfiles/toolchain.gypi \
@@ -205,62 +200,20 @@ test-valgrind: all
 	$(PYTHON) tools/test.py --mode=release --valgrind sequential parallel message
 
 # Implicitly depends on $(NODE_EXE).  We don't depend on it explicitly because
-# it always triggers a rebuild due to it being a .PHONY rule.  See the comment
-# near the build-addons rule for more background.
+# it always triggers a rebuild due to it being a .PHONY rule.  The parent make
+# cannot know if the subprocess touched anything so it pessimistically assumes
+# that binding.node is out of date and needs a rebuild.  Just goes to show that
+# recursive make really is harmful...
+# TODO(bnoordhuis) Force rebuild after gyp update.
 test/gc/build/Release/binding.node: test/gc/binding.cc test/gc/binding.gyp
 	$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
 		--python="$(PYTHON)" \
 		--directory="$(shell pwd)/test/gc" \
 		--nodedir="$(shell pwd)"
 
-# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
-DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md
-
-ifeq ($(OSTYPE),aix)
-DOCBUILDSTAMP_PREREQS := $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
-endif
-
-test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
-	$(RM) -r test/addons/??_*/
-	$(NODE) $<
-	touch $@
-
-ADDONS_BINDING_GYPS := \
-	$(filter-out test/addons/??_*/binding.gyp, \
-		$(wildcard test/addons/*/binding.gyp))
-
-ADDONS_BINDING_SOURCES := \
-	$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
-	$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))
-
-# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
-# Depends on node-gyp package.json so that build-addons is (re)executed when
-# node-gyp is updated as part of an npm update.
-test/addons/.buildstamp: config.gypi \
-	deps/npm/node_modules/node-gyp/package.json \
-	$(ADDONS_BINDING_GYPS) $(ADDONS_BINDING_SOURCES) \
-	deps/uv/include/*.h deps/v8/include/*.h \
-	src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
-	test/addons/.docbuildstamp
-#	Cannot use $(wildcard test/addons/*/) here, it's evaluated before
-#	embedded addons have been generated from the documentation.
-	@for dirname in test/addons/*/; do \
-		printf "\nBuilding addon $$PWD/$$dirname\n" ; \
-		env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
-		        --loglevel=$(LOGLEVEL) rebuild \
-			--python="$(PYTHON)" \
-			--directory="$$PWD/$$dirname" \
-			--nodedir="$$PWD" || exit 1 ; \
-	done
-	touch $@
-
-# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
-# directly because it calls make recursively.  The parent make cannot know
-# if the subprocess touched anything so it pessimistically assumes that
-# .buildstamp and .docbuildstamp are out of date and need a rebuild.
-# Just goes to show that recursive make really is harmful...
-# TODO(bnoordhuis) Force rebuild after gyp update.
-build-addons: $(NODE_EXE) test/addons/.buildstamp
+.PHONY: build-addons
+build-addons: $(NODE_EXE)
+	./$< tools/build-addons.js
 
 ifeq ($(OSTYPE),$(filter $(OSTYPE),darwin aix))
   XARGS = xargs
@@ -287,7 +240,7 @@ CI_JS_SUITES := doctool inspector known_issues message parallel pseudo-tty seque
 
 # Build and test addons without building anything else
 test-ci-native: LOGLEVEL := info
-test-ci-native: | test/addons/.buildstamp
+test-ci-native: | build-addons
 	$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
 		--mode=release --flaky-tests=$(FLAKY_TESTS) \
 		$(TEST_CI_ARGS) $(CI_NATIVE_SUITES)
@@ -361,7 +314,7 @@ test-addons: test-build
 test-addons-clean:
 	$(RM) -rf test/addons/??_*/
 	$(RM) -rf test/addons/*/build
-	$(RM) test/addons/.buildstamp test/addons/.docbuildstamp
+	$(RM) -rf test/addons/include/
 
 test-timers:
 	$(MAKE) --directory=tools faketime

test/addons/openssl-binding/binding.gyp

@@ -5,7 +5,6 @@
       'conditions': [
          ['node_use_openssl=="true"', {
            'sources': ['binding.cc'],
-           'include_dirs': ['../../../deps/openssl/openssl/include'],
          }]
       ]
     },

tools/build-addons.js

@@ -0,0 +1,55 @@
+'use strict';
+
+const fs = require('fs');
+const { spawnSync } = require('child_process');
+const { resolve } = require('path');
+
+const kTopLevelDirectory = resolve(__dirname, '..');
+const kAddonsDirectory = resolve(kTopLevelDirectory, 'test/addons');
+
+const kPython = process.env.PYTHON || 'python';
+const kNodeGyp =
+    resolve(kTopLevelDirectory, 'deps/npm/node_modules/node-gyp/bin/node-gyp');
+
+process.chdir(kTopLevelDirectory);
+
+// Copy headers to test/addons/include.  install.py preserves timestamps.
+{
+  const args = [ 'tools/install.py', 'install', kAddonsDirectory, '/' ];
+  const env = Object.assign({}, process.env);
+  env.HEADERS_ONLY = 'yes, please';  // Ask nicely.
+  env.LOGLEVEL = 'WARNING';
+
+  const options = { env, stdio: 'inherit' };
+  spawnSync(kPython, args, options);
+}
+
+// Scrape embedded add-ons from doc/api/addons.md.
+require('./doc/addon-verify.js');
+
+// Regenerate build files and rebuild if necessary.
+for (const basedir of fs.readdirSync(kAddonsDirectory)) {
+  const path = resolve(kAddonsDirectory, basedir);
+  if (!fs.statSync(path).isDirectory()) continue;
+
+  const gypfile = resolve(path, 'binding.gyp');
+  if (!fs.existsSync(gypfile)) continue;
+
+  const args = [ kNodeGyp,
+                 '--directory=' + path,
+                 '--loglevel=silent',
+                 '--nodedir=' + kAddonsDirectory,
+                 '--python=' + kPython ];
+  const env = Object.assign({}, process.env);
+  env.MAKEFLAGS = '-j1';
+
+  const options = { env, stdio: 'inherit' };
+  {
+    const proc = spawnSync(process.execPath, args.concat('configure'), options);
+    if (proc.status !== 0) process.exit(1);
+  }
+  {
+    const proc = spawnSync(process.execPath, args.concat('build'), options);
+    if (proc.status !== 0) process.exit(1);
+  }
+}

tools/doc/addon-verify.js

@@ -1,5 +1,6 @@
 'use strict';
 
+const assert = require('assert');
 const fs = require('fs');
 const path = require('path');
 const marked = require('marked');
@@ -20,13 +21,8 @@ tokens.push({ type: 'heading' });
 for (var i = 0; i < tokens.length; i++) {
   var token = tokens[i];
   if (token.type === 'heading' && token.text) {
-    const blockName = token.text;
-    if (files && Object.keys(files).length !== 0) {
-      verifyFiles(files,
-                  blockName,
-                  console.log.bind(null, 'wrote'),
-                  function(err) { if (err) throw err; });
-    }
+    if (files && Object.keys(files).length !== 0)
+      verifyFiles(files, token.text);
     files = {};
   } else if (token.type === 'code') {
     var match = token.text.match(/^\/\/\s+(.*\.(?:cc|h|js))[\r\n]/);
@@ -36,17 +32,7 @@ for (var i = 0; i < tokens.length; i++) {
   }
 }
 
-function once(fn) {
-  var once = false;
-  return function() {
-    if (once)
-      return;
-    once = true;
-    fn.apply(this, arguments);
-  };
-}
-
-function verifyFiles(files, blockName, onprogress, ondone) {
+function verifyFiles(files, blockName) {
   // must have a .cc and a .js to be a valid test
   if (!Object.keys(files).some((name) => /\.cc$/.test(name)) ||
       !Object.keys(files).some((name) => /\.js$/.test(name))) {
@@ -91,22 +77,24 @@ ${files[name].replace('Release', "' + common.buildType + '")}
     })
   });
 
-  fs.mkdir(dir, function() {
-    // Ignore errors
+  try {
+    fs.mkdirSync(dir);
+  } catch (e) {
+    assert.strictEqual(e.code, 'EEXIST');
+  }
 
-    var done = once(ondone);
-    var waiting = files.length;
-    files.forEach(function(file) {
-      fs.writeFile(file.path, file.content, function(err) {
-        if (err)
-          return done(err);
+  for (const file of files) {
+    try {
+      var content = fs.readFileSync(file.path);
+    } catch (e) {
+      assert.strictEqual(e.code, 'ENOENT');
+    }
 
-        if (onprogress)
-          onprogress(file.path);
+    // Only update when file content has changed to prevent unneeded rebuilds.
+    if (content && content.toString() === file.content)
+      continue;
 
-        if (--waiting === 0)
-          done();
-      });
-    });
-  });
+    fs.writeFileSync(file.path, file.content);
+    console.log('wrote', file.path);
+  }
 }

tools/install.py

@@ -2,13 +2,18 @@
 
 import errno
 import json
+import logging
 import os
 import re
 import shutil
 import sys
 from getmoduleversion import get_version
 
+logging.basicConfig(format='%(message)s',
+                    level=os.environ.get('LOGLEVEL', 'INFO'))
+
 # set at init time
+headers_only = os.environ.get('HEADERS_ONLY')
 node_prefix = '/usr/local' # PREFIX variable from Makefile
 install_path = None # base target directory (DESTDIR + PREFIX from Makefile)
 target_defaults = None
@@ -31,7 +36,7 @@ def try_unlink(path):
     if e.errno != errno.ENOENT: raise
 
 def try_symlink(source_path, link_path):
-  print 'symlinking %s -> %s' % (source_path, link_path)
+  logging.info('symlinking %s -> %s', source_path, link_path)
   try_unlink(link_path)
   os.symlink(source_path, link_path)
 
@@ -61,14 +66,15 @@ def mkpaths(path, dst):
 
 def try_copy(path, dst):
   source_path, target_path = mkpaths(path, dst)
-  print 'installing %s' % target_path
+  logging.info('installing %s', target_path)
   try_mkdir_r(os.path.dirname(target_path))
-  try_unlink(target_path) # prevent ETXTBSY errors
+  if not headers_only:
+    try_unlink(target_path)  # Prevent ETXTBSY errors.
   return shutil.copy2(source_path, target_path)
 
 def try_remove(path, dst):
   source_path, target_path = mkpaths(path, dst)
-  print 'removing %s' % target_path
+  logging.info('removing %s', target_path)
   try_unlink(target_path)
   try_rmdir_r(os.path.dirname(target_path))
 
@@ -200,7 +206,7 @@ def run(args):
 
   cmd = args[1] if len(args) > 1 else 'install'
 
-  if os.environ.get('HEADERS_ONLY'):
+  if headers_only:
     if cmd == 'install': return headers(install)
     if cmd == 'uninstall': return headers(uninstall)
   else:

vcbuild.bat

@@ -320,17 +320,8 @@ for /d %%F in (test\addons\??_*) do (
   rd /s /q %%F
 )
 :: generate
-"%node_exe%" tools\doc\addon-verify.js
+"%node_exe%" tools\build-addons.js
 if %errorlevel% neq 0 exit /b %errorlevel%
-:: building addons
-SetLocal EnableDelayedExpansion
-for /d %%F in (test\addons\*) do (
-  "%node_exe%" deps\npm\node_modules\node-gyp\bin\node-gyp rebuild ^
-    --directory="%%F" ^
-    --nodedir="%cd%"
-  if !errorlevel! neq 0 exit /b !errorlevel!
-)
-EndLocal
 goto run-tests
 
 :run-tests