deps: upgrade npm to 6.10.1

[isaacs]

Full release notes: https://npm.community/t/release-npm-6-10-1/8790

Notable changes:

• Fixes an EISDIR error in the cache
• Adds support for Visual Studio 2019 by upgrading node-gyp to 5.0.2
• Strip git environment variables when running git subcommands, so that
  running npm from within an existing git process does not get borked.

[Trott]

Rubber-stamp LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24394/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24467/

[Trott]

addon test failures on macOS are persistent. Many of the addon tests use stuff in deps/npm....

[richardlau]

addon test failures on macOS are persistent. Many of the addon tests use stuff in deps/npm....

... and uses node-gyp, which was updated in this PR inside npm from version 3.8.0 to 5.0.2. Given that it's macOS only, the prime suspect would be nodejs/node-gyp@2761afb (nodejs/node-gyp#1689).

[joaocgreis]

ARM64 Windows support landed as a floating patch in #28604. This PR already contains all of that, so this can simply overwrite what is currently in master.

[Trott]

addon test failures on macOS are persistent. Many of the addon tests use stuff in deps/npm....

... and uses node-gyp, which was updated in this PR inside npm from version 3.8.0 to 5.0.2. Given that it's macOS only, the prime suspect would be nodejs/node-gyp@2761afb (nodejs/node-gyp#1689).

^^^^ @bnoordhuis @nodejs/platform-macos @nodejs/node-gyp

[rvagg]

Well the only thing that could be impacting it from those two commits is the addon.gypi changes: nodejs/node-gyp@2761afb#diff-df3ec83827f1a390820c4225e8cbcc9a
Someone want to try applying the reverse patch and running it through CI?

diff --git a/addon.gypi b/addon.gypi
index e8b95c5..e1c4b24 100644
--- a/addon.gypi
+++ b/addon.gypi
@@ -90,10 +90,14 @@

     'conditions': [
       [ 'OS=="mac"', {
+        'cflags': [
+          '-fvisibility=hidden'
+        ],
         'defines': [
           '_DARWIN_USE_64_BIT_INODE=1'
         ],
         'xcode_settings': {
+          'GCC_SYMBOLS_PRIVATE_EXTERN': 'YES', # -fvisibility=hidden
           'DYLIB_INSTALL_NAME_BASE': '@rpath'
         },
       }],

[bnoordhuis]

It's the node-gyp change. Those add-on tests are actually defective but it slipped under the radar so far. The fix is trivial:

diff --git a/test/addons/dlopen-ping-pong/binding.cc b/test/addons/dlopen-ping-pong/binding.cc
index 6a6c297b52..8ad6a675b6 100644
--- a/test/addons/dlopen-ping-pong/binding.cc
+++ b/test/addons/dlopen-ping-pong/binding.cc
@@ -5,6 +5,7 @@
 
 #include <dlfcn.h>
 
+__attribute__((visibility("default")))
 extern "C" const char* dlopen_pong(void) {
   return "pong";
 }
diff --git a/test/addons/dlopen-ping-pong/ping.c b/test/addons/dlopen-ping-pong/ping.c
index bb0e6845d5..c76f40580c 100644
--- a/test/addons/dlopen-ping-pong/ping.c
+++ b/test/addons/dlopen-ping-pong/ping.c
@@ -2,6 +2,7 @@
 
 const char* dlopen_pong(void);
 
+__attribute__((visibility("default")))
 const char* dlopen_ping(void) {
   return dlopen_pong();
 }
diff --git a/test/addons/openssl-client-cert-engine/testengine.cc b/test/addons/openssl-client-cert-engine/testengine.cc
index 078695a056..b0acf23c06 100644
--- a/test/addons/openssl-client-cert-engine/testengine.cc
+++ b/test/addons/openssl-client-cert-engine/testengine.cc
@@ -19,6 +19,12 @@
 #define AGENT_KEY           "test/fixtures/keys/agent1-key.pem"
 #define AGENT_CERT          "test/fixtures/keys/agent1-cert.pem"
 
+#ifdef _WIN32
+# define DEFAULT_VISIBILITY
+#else
+# define DEFAULT_VISIBILITY __attribute__((visibility("default")))
+#endif
+
 namespace {
 
 int EngineInit(ENGINE* engine) {
@@ -93,8 +99,8 @@ int bind_fn(ENGINE* engine, const char* id) {
 }
 
 extern "C" {
-  IMPLEMENT_DYNAMIC_CHECK_FN();
-  IMPLEMENT_DYNAMIC_BIND_FN(bind_fn);
+  DEFAULT_VISIBILITY IMPLEMENT_DYNAMIC_CHECK_FN();
+  DEFAULT_VISIBILITY IMPLEMENT_DYNAMIC_BIND_FN(bind_fn);
 }
 
 }  // anonymous namespace

It does mean the new version of node-gyp might break third-party code that worked by accident before, and I suppose that means it has to be reverted because it landed in a patch release...

[rvagg]

FWIW I submitted a job just after I posted that last comment with that patch undone and it's passing: https://ci.nodejs.org/job/node-test-commit/30143/

Let's get that revert done and a release out tomorrow.

@bnoordhuis will you take care of getting your patch into here so we can do that change in a node-gyp v6?

[rvagg]

[email protected] is out and fixes this by reverting the change to addon.gypi, we'll push out a v6 sometime soon because we have at least one other semver-major queued.

For now, any thoughts on how to handle this? Do we wait for an new npm with it or do we patch it on top of 6.10.1 for this PR?

[bnoordhuis]

Npm needs to do a new release anyway, otherwise people still get a broken install when they manually upgrade to 6.10.1.

[irega]

Npm needs to do a new release anyway, otherwise people still get a broken install when they manually upgrade to 6.10.1.

@bnoordhuis @rvagg I have started to update "npm-lifecycle" and "npm" with this PR. It is pending of approvement

[irega]

node-gyp 5.0.3 will be included in the next npm version (6.10.2). See 06772f3 made by @isaacs for more details

[irega]

npm version 6.10.2 was released by @isaacs should we create a new PR?

EDIT: PR created.

[isaacs]

Closing in favor of #28853