From ad2aaa8c3c2a28b07c550aa99187cbd147a65962 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Sat, 6 Mar 2021 00:54:40 -0600 Subject: [PATCH 1/2] fix: require .node files directly to detect incompatible native modules - This fixes the incompatible native module detection for the packages that require their .node files lazily - Speeds up the performance of detection by directly require .node files instead of requiring the package --- src/package.js | 53 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/src/package.js b/src/package.js index fd4b92789..d7c490e3c 100644 --- a/src/package.js +++ b/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 { From 0f04fb2a74468688c3db7e8255dfe4493026e516 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Sat, 6 Mar 2021 20:00:57 -0600 Subject: [PATCH 2/2] test: add test for conditionally loaded .node files --- .gitignore | 1 + .../main.js | 6 ++++++ .../native-module/build/Release/native.node | 0 .../node_modules/native-module/main.js | 7 +++++++ .../node_modules/native-module/package.json | 4 ++++ .../package.json | 5 +++++ spec/package-spec.js | 14 ++++++++++++++ 7 files changed, 37 insertions(+) create mode 100644 spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/main.js create mode 100644 spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/build/Release/native.node create mode 100644 spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/main.js create mode 100644 spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/package.json create mode 100644 spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/package.json diff --git a/.gitignore b/.gitignore index b21d0cdf9..c743195e3 100644 --- a/.gitignore +++ b/.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/ diff --git a/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/main.js b/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/main.js new file mode 100644 index 000000000..3b6530bc1 --- /dev/null +++ b/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); +} diff --git a/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/build/Release/native.node b/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/build/Release/native.node new file mode 100644 index 000000000..e69de29bb diff --git a/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/main.js b/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/main.js new file mode 100644 index 000000000..bde22006e --- /dev/null +++ b/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; + } +} diff --git a/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/package.json b/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/node_modules/native-module/package.json new file mode 100644 index 000000000..cac262cba --- /dev/null +++ b/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" +} diff --git a/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/package.json b/spec/fixtures/packages/package-with-incompatible-native-module-loaded-conditionally/package.json new file mode 100644 index 000000000..857bc7221 --- /dev/null +++ b/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" +} diff --git a/spec/package-spec.js b/spec/package-spec.js index 59025efaa..a31c2141c 100644 --- a/spec/package-spec.js +++ b/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]