Merge pull request #21927 from atom-community/incompatible-native-detection

fix: require .node files directly to detect incompatible native modules

.gitignore

@@ -16,5 +16,6 @@ node_modules
 docs/output
 docs/includes
 spec/fixtures/evil-files/
+!spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/
 out/
 /electron/

spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/main.js

@@ -0,0 +1,6 @@
+const condition = false;
+
+if (condition) {
+    const { native } = require("./node_modules/native-module");
+    native(condition);
+}

spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/main.js

@@ -0,0 +1,7 @@
+exports.native = function loadNative(condition) {
+    if (condition) {
+        return require('../build/Release/native.node');
+    } else {
+        return null;
+    }
+}

spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "native-module",
+  "main": "./main.js"
+}

spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "package-with-incompatible-native-module",
+  "version": "1.0.0",
+  "main": "./main.js"
+}

spec/package-spec.js

@@ -45,6 +45,20 @@ describe('Package', function() {
       );
     });
 
+    it('detects the package as incompatible even if .node file is loaded conditionally', function() {
+      const packagePath = atom.project
+        .getDirectories()[0]
+        .resolve(
+          'packages/package-with-incompatible-native-module-loaded-conditionally'
+        );
+      const pack = buildPackage(packagePath);
+      expect(pack.isCompatible()).toBe(false);
+      expect(pack.incompatibleModules[0].name).toBe('native-module');
+      expect(pack.incompatibleModules[0].path).toBe(
+        path.join(packagePath, 'node_modules', 'native-module')
+      );
+    });
+
     it("utilizes _atomModuleCache if present to determine the package's native dependencies", function() {
       let packagePath = atom.project
         .getDirectories()[0]

src/package.js

@@ -1126,46 +1126,60 @@ module.exports = class Package {
   // Does the given module path contain native code?
   isNativeModule(modulePath) {
     try {
-      return (
-        fs.listSync(path.join(modulePath, 'build', 'Release'), ['.node'])
-          .length > 0
-      );
+      return this.getModulePathNodeFiles(modulePath).length > 0;
     } catch (error) {
       return false;
     }
   }
 
-  // Get an array of all the native modules that this package depends on.
+  // get the list of `.node` files for the given module path
+  getModulePathNodeFiles(modulePath) {
+    try {
+      const modulePathNodeFiles = fs.listSync(
+        path.join(modulePath, 'build', 'Release'),
+        ['.node']
+      );
+      return modulePathNodeFiles;
+    } catch (error) {
+      return [];
+    }
+  }
+
+  // Get a Map of all the native modules => the `.node` files that this package depends on.
   //
   // First try to get this information from
   // @metadata._atomModuleCache.extensions. If @metadata._atomModuleCache doesn't
   // exist, recurse through all dependencies.
-  getNativeModuleDependencyPaths() {
-    const nativeModulePaths = [];
+  getNativeModuleDependencyPathsMap() {
+    const nativeModulePaths = new Map();
 
     if (this.metadata._atomModuleCache) {
+      const nodeFilePaths = [];
       const relativeNativeModuleBindingPaths =
         (this.metadata._atomModuleCache.extensions &&
           this.metadata._atomModuleCache.extensions['.node']) ||
         [];
       for (let relativeNativeModuleBindingPath of relativeNativeModuleBindingPaths) {
-        const nativeModulePath = path.join(
+        const nodeFilePath = path.join(
           this.path,
           relativeNativeModuleBindingPath,
           '..',
           '..',
           '..'
         );
-        nativeModulePaths.push(nativeModulePath);
+        nodeFilePaths.push(nodeFilePath);
       }
+      nativeModulePaths.set(this.path, nodeFilePaths);
       return nativeModulePaths;
     }
 
     var traversePath = nodeModulesPath => {
       try {
         for (let modulePath of fs.listSync(nodeModulesPath)) {
-          if (this.isNativeModule(modulePath))
-            nativeModulePaths.push(modulePath);
+          const modulePathNodeFiles = this.getModulePathNodeFiles(modulePath);
+          if (modulePathNodeFiles) {
+            nativeModulePaths.set(modulePath, modulePathNodeFiles);
+          }
           traversePath(path.join(modulePath, 'node_modules'));
         }
       } catch (error) {}
@@ -1176,6 +1190,12 @@ module.exports = class Package {
     return nativeModulePaths;
   }
 
+  // Get an array of all the native modules that this package depends on.
+  // See `getNativeModuleDependencyPathsMap` for more information
+  getNativeModuleDependencyPaths() {
+    return [...this.getNativeModuleDependencyPathsMap().keys()];
+  }
+
   /*
   Section: Native Module Compatibility
   */
@@ -1277,8 +1297,7 @@ module.exports = class Package {
   }
 
   // Get the incompatible native modules that this package depends on.
-  // This recurses through all dependencies and requires all modules that
-  // contain a `.node` file.
+  // This recurses through all dependencies and requires all `.node` files.
   //
   // This information is cached in local storage on a per package/version basis
   // to minimize the impact on startup time.
@@ -1293,9 +1312,13 @@ module.exports = class Package {
     }
 
     const incompatibleNativeModules = [];
-    for (let nativeModulePath of this.getNativeModuleDependencyPaths()) {
+    const nativeModulePaths = this.getNativeModuleDependencyPathsMap();
+    for (const [nativeModulePath, nodeFilesPaths] of nativeModulePaths) {
       try {
-        require(nativeModulePath);
+        // require each .node file
+        for (const nodeFilePath of nodeFilesPaths) {
+          require(nodeFilePath);
+        }
       } catch (error) {
         let version;
         try {