From 2f354c9323cc9af6d0830ac3db8adc9699087a6e Mon Sep 17 00:00:00 2001 From: emizzle Date: Tue, 29 Jan 2019 16:10:01 +1100 Subject: [PATCH] fix(@embark/solc): Import remapping update Changes include: - Add unit tests for recursively remapping imports - Handle plugin contracts - Allow `prepareForCompilation` to be called from `File`. Allows external compilers, like `embark-solc` to call this. - Add flattened remappings to file getting prepared for compilation - Return remapped contract content when file type is http - Add check allowing files to be remapped/prepared multiple times --- packages/embark/src/lib/core/file.ts | 24 ++-- .../embark/src/lib/modules/pipeline/index.js | 2 +- .../embark/src/lib/modules/solidity/index.js | 3 +- .../embark/src/lib/modules/solidity/solcP.js | 13 --- .../src/lib/utils/solidity/remapImports.ts | 60 +++++++--- packages/embark/src/test/config.js | 14 ++- packages/embark/src/test/file.js | 42 +++++++ .../src/test/modules/solidity/remapImports.js | 105 ++++++++++++++++++ 8 files changed, 217 insertions(+), 46 deletions(-) create mode 100644 packages/embark/src/test/file.js create mode 100644 packages/embark/src/test/modules/solidity/remapImports.js diff --git a/packages/embark/src/lib/core/file.ts b/packages/embark/src/lib/core/file.ts index 0ab58f56a..210814760 100644 --- a/packages/embark/src/lib/core/file.ts +++ b/packages/embark/src/lib/core/file.ts @@ -1,4 +1,5 @@ import * as path from "path"; +import { ImportRemapping, prepareForCompilation } from "../utils/solidity/remapImports"; const fs = require("./fs.js"); const utils = require("../utils/utils"); @@ -10,11 +11,6 @@ export enum Types { http = "http", } -interface ImportRemapping { - prefix: string; - target: string; -} - export class File { public type: Types; public externalUrl: string = ""; @@ -30,22 +26,34 @@ export class File { constructor(options: any) { this.type = options.type; - this.basedir = options.basedir; + this.basedir = options.basedir || ""; this.resolver = options.resolver; this.pluginPath = options.pluginPath ? options.pluginPath : ""; this.storageConfig = options.storageConfig; this.providerUrl = ""; this.originalPath = options.originalPath || ""; - if (this.type === Types.http) { + if (this.type === Types.custom && this.pluginPath) { + this.path = path.join(this.pluginPath, options.path).replace(fs.dappPath(), ""); + if (this.path.startsWith("/")) { + this.path = this.path.substring(1); + } + } else if (this.type === Types.http) { const external = utils.getExternalContractUrl(options.externalUrl, this.providerUrl); this.externalUrl = external.url; - this.path = external.filePath; + this.path = fs.dappPath(external.filePath); } else { this.path = options.path.replace(/\\/g, "/"); } } + public async prepareForCompilation(isCoverage = false) { + if (!this.path.endsWith(".sol")) { + return Promise.reject("this method is only supported for Solidity files"); + } + return prepareForCompilation(this, isCoverage); + } + public get content(): Promise { return new Promise((resolve) => { switch (this.type) { diff --git a/packages/embark/src/lib/modules/pipeline/index.js b/packages/embark/src/lib/modules/pipeline/index.js index 9a6a96536..b2a3912fd 100644 --- a/packages/embark/src/lib/modules/pipeline/index.js +++ b/packages/embark/src/lib/modules/pipeline/index.js @@ -292,7 +292,7 @@ class Pipeline { self.logger.trace("reading " + file.path); file.content.then((fileContent) => { self.runPlugins(file, fileContent, fileCb); - }); + }).catch(fileCb); }, function (err, contentFiles) { if (err) { diff --git a/packages/embark/src/lib/modules/solidity/index.js b/packages/embark/src/lib/modules/solidity/index.js index eed2c7a84..b64d9ba8d 100644 --- a/packages/embark/src/lib/modules/solidity/index.js +++ b/packages/embark/src/lib/modules/solidity/index.js @@ -1,6 +1,5 @@ let async = require('../../utils/async_extend.js'); let SolcW = require('./solcW.js'); -const remapImports = require('../../utils/solidity/remapImports'); class Solidity { @@ -177,7 +176,7 @@ class Solidity { originalFilepath[filename] = file.path; - remapImports.prepareForCompilation(file, options.isCoverage) + file.prepareForCompilation(options.isCoverage) .then(fileContent => { input[file.path] = {content: fileContent.replace(/\r\n/g, '\n')}; fileCb(); diff --git a/packages/embark/src/lib/modules/solidity/solcP.js b/packages/embark/src/lib/modules/solidity/solcP.js index 38d048ba1..09b0909ef 100644 --- a/packages/embark/src/lib/modules/solidity/solcP.js +++ b/packages/embark/src/lib/modules/solidity/solcP.js @@ -1,8 +1,5 @@ const fs = require('fs-extra'); -const path = require('path'); const semver = require('semver'); -const constants = require('../../constants'); -const Utils = require('../../utils/utils'); const ProcessWrapper = require('../../core/processes/processWrapper'); const PluginManager = require('live-plugin-manager-git-fix').PluginManager; import LongRunningProcessTimer from '../../utils/longRunningProcessTimer'; @@ -17,19 +14,9 @@ class SolcProcess extends ProcessWrapper { } findImports(filename) { - if (filename.startsWith('http') || filename.startsWith('git')) { - const fileObj = Utils.getExternalContractUrl(filename, this._providerUrl); - filename = fileObj.filePath; - } if (fs.existsSync(filename)) { return {contents: fs.readFileSync(filename).toString()}; } - if (fs.existsSync(path.join('./node_modules/', filename))) { - return {contents: fs.readFileSync(path.join('./node_modules/', filename)).toString()}; - } - if (fs.existsSync(path.join(constants.httpContractsDirectory, filename))) { - return {contents: fs.readFileSync(path.join(constants.httpContractsDirectory, filename)).toString()}; - } return {error: 'File not found'}; } diff --git a/packages/embark/src/lib/utils/solidity/remapImports.ts b/packages/embark/src/lib/utils/solidity/remapImports.ts index 2522e855c..3bff514ef 100644 --- a/packages/embark/src/lib/utils/solidity/remapImports.ts +++ b/packages/embark/src/lib/utils/solidity/remapImports.ts @@ -8,10 +8,14 @@ const fs = require("../../core/fs"); const FIND_IMPORTS_REGEX = /^import[\s]*(['"])(.*)\1;/gm; const FIND_FILE_REGEX = /import[\s]*(['"])(.*)\1;/; +export interface ImportRemapping { + prefix: string; + target: string; +} + interface RemapImport { path: string; - searchValue: string; - replaceValue: string; + remapping: ImportRemapping; } const getImports = (source: string) => { @@ -33,14 +37,10 @@ const prepareInitialFile = async (file: File) => { } const destination = fs.dappPath(".embark", file.path); - if (file.type === Types.dappFile) { + if (file.type === Types.dappFile || file.type === Types.custom) { fs.copySync(fs.dappPath(file.path), destination); } - if (file.type === Types.custom) { - fs.writeFileSync(destination); - } - file.path = destination; }; @@ -101,8 +101,9 @@ const rescursivelyFindRemapImports = async (file: File, filesProcessed: string[] for (const importPath of imports) { const newFile = buildNewFile(file, importPath); - file.importRemappings.push({prefix: importPath, target: newFile.path}); - remapImports.push({path: file.path, searchValue: importPath, replaceValue: newFile.path}); + const remapping = { prefix: importPath, target: newFile.path }; + file.importRemappings.push(remapping); + remapImports.push({ path: file.path, remapping }); remapImports = remapImports.concat( await rescursivelyFindRemapImports(newFile, filesProcessed), ); @@ -124,30 +125,55 @@ const isHttp = (input: string) => { }; const replaceImports = (remapImports: RemapImport[]) => { - const byPath: {[path: string]: [{searchValue: string, replaceValue: string}]} = groupBy(remapImports, "path"); + const byPath: { [path: string]: [{ remapping: ImportRemapping }] } = groupBy(remapImports, "path"); Object.keys(byPath).forEach((p) => { let source = fs.readFileSync(p, "utf-8"); - byPath[p].forEach(({searchValue, replaceValue}) => { - source = source.replace(`import "${searchValue}"`, `import "${replaceValue}"`); + byPath[p].forEach(({ remapping }) => { + source = source.replace(`import "${remapping.prefix}"`, `import "${remapping.target}"`); }); fs.writeFileSync(p, source); }); }; +const addRemappingsToFile = (file: File, remapImports: RemapImport[]) => { + const byPath: { [path: string]: [{ remapping: ImportRemapping }] } = groupBy(remapImports, "path"); + const paths = Object.keys(byPath); + if (paths) { + file.importRemappings = []; // clear as we already have the first remapping added + paths.forEach((p) => { + const [...remappings] = byPath[p].map((importRemapping) => importRemapping.remapping); + file.importRemappings = file.importRemappings.concat(remappings); + }); + } +}; + const resolve = (input: string) => { try { - return require.resolve(input, {paths: [fs.dappPath("node_modules")]}); + const result = require.resolve(input, { paths: [fs.dappPath("node_modules"), fs.embarkPath("node_modules")] }); + return result; } catch (e) { return ""; } }; export const prepareForCompilation = async (file: File, isCoverage = false) => { - await prepareInitialFile(file); - const remapImports = await rescursivelyFindRemapImports(file); - replaceImports(remapImports); + if (!file.isPrepared) { + await prepareInitialFile(file); + const remapImports = await rescursivelyFindRemapImports(file); + replaceImports(remapImports); + // add all remappings to top-level file + addRemappingsToFile(file, remapImports); + + // set flag to prevent copying, remapping, and changing of paths again + file.isPrepared = true; + } + let content; + if (file.type === Types.http) { + content = (await fs.readFile(file.path)).toString(); + } else { + content = await file.content; + } - const content = await file.content; if (!isCoverage) { return content; } diff --git a/packages/embark/src/test/config.js b/packages/embark/src/test/config.js index fb25051fa..616d8d7f0 100644 --- a/packages/embark/src/test/config.js +++ b/packages/embark/src/test/config.js @@ -4,6 +4,7 @@ const Plugins = require('../lib/core/plugins.js'); const assert = require('assert'); const TestLogger = require('../lib/utils/test_logger'); const Events = require('../lib/core/events'); +const fs = require('../lib/core/fs'); describe('embark.Config', function () { let config = new Config({ @@ -190,12 +191,13 @@ describe('embark.Config', function () { const expected = [ { "type": "http", - "externalUrl": "https://raw.githubusercontent.com/embark-framework/embark/master/test_dapps/test_app/app/contracts/simple_storage.sol", - "path": ".embark/contracts/embark-framework/embark/master/test_dapps/test_app/app/contracts/simple_storage.sol", - "originalPath": ".embark/contracts/embark-framework/embark/master/test_dapps/test_app/app/contracts/simple_storage.sol", + "externalUrl": "https://raw.githubusercontent.com/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol", + "path": fs.dappPath(".embark/contracts/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol"), + "originalPath": ".embark/contracts/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol", "pluginPath": '', "basedir": "", "importRemappings": [], + "isPrepared": false, "resolver": undefined, "storageConfig": undefined, "providerUrl": "" @@ -203,23 +205,25 @@ describe('embark.Config', function () { { "type": "http", "externalUrl": "https://raw.githubusercontent.com/status-im/contracts/master/contracts/identity/ERC725.sol", - "path": ".embark/contracts/status-im/contracts/master/contracts/identity/ERC725.sol", + "path": fs.dappPath(".embark/contracts/status-im/contracts/master/contracts/identity/ERC725.sol"), "originalPath": ".embark/contracts/status-im/contracts/master/contracts/identity/ERC725.sol", "pluginPath": '', "basedir": "", "importRemappings": [], + "isPrepared": false, "resolver": undefined, "storageConfig": undefined, "providerUrl": "" }, { "externalUrl": "https://swarm-gateways.net/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63", - "path": ".embark/contracts/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63", + "path": fs.dappPath(".embark/contracts/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63"), "originalPath": ".embark/contracts/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63", "type": "http", "pluginPath": '', "basedir": "", "importRemappings": [], + "isPrepared": false, "resolver": undefined, "storageConfig": undefined, "providerUrl": "" diff --git a/packages/embark/src/test/file.js b/packages/embark/src/test/file.js new file mode 100644 index 000000000..aa2b7c05d --- /dev/null +++ b/packages/embark/src/test/file.js @@ -0,0 +1,42 @@ +/*globals describe, it, before*/ +const {File, Types} = require("../lib/core/file"); +const path = require("path"); +const {expect, assert} = require("chai"); +const fs = require("../lib/core/fs"); +const fsNode = require("fs"); + +describe('embark.File', function () { + describe('Read file contents', function () { + it('should be able to download a file when type is "http"', async () => { + const file = new File({externalUrl: 'https://raw.githubusercontent.com/embark-framework/embark/master/test_apps/test_app/app/contracts/simple_storage.sol', type: Types.http}); + const content = await file.content; + + const contentFromFileSystem = fsNode.readFileSync(path.join(fs.embarkPath(), "../../", "test_dapps/packages/test_app/app/contracts/simple_storage.sol")).toString(); + expect(content).to.equal(contentFromFileSystem); + }); + + it('should be able to read a file when type is "dappFile"', async () => { + const file = new File({path: fs.dappPath('contracts/recursive_test_0.sol'), type: Types.dappFile}); + const content = await file.content; + + const contentFromFileSystem = fs.readFileSync(fs.dappPath("contracts/recursive_test_0.sol")).toString(); + expect(content).to.equal(contentFromFileSystem); + }); + + it('should be able to execute a resolver when type is "custom"', async () => { + const file = new File({path: fs.dappPath('contracts/recursive_test_0.sol'), type: Types.custom, resolver: (callback) => { + callback("test"); + }}); + expect(await file.content).to.equal("test"); + }); + + it('should be able to read a file when type is "embarkInternal"', async () => { + const file = new File({path: 'test/contracts/recursive_test_0.sol', type: Types.embarkInternal}); + const content = await file.content; + + const contentFromFileSystem = fs.readFileSync(fs.dappPath("contracts/recursive_test_0.sol")).toString(); + expect(content).to.equal(contentFromFileSystem); + }); + + }); +}); diff --git a/packages/embark/src/test/modules/solidity/remapImports.js b/packages/embark/src/test/modules/solidity/remapImports.js new file mode 100644 index 000000000..95ec57020 --- /dev/null +++ b/packages/embark/src/test/modules/solidity/remapImports.js @@ -0,0 +1,105 @@ +/*globals describe, it, before*/ +const {File, Types} = require("../../../lib/core/file"); +const path = require("path"); +const remapImports = require("../../../lib/utils/solidity/remapImports"); +const {expect} = require("chai"); +const fs = require("../../../lib/core/fs"); +const fsNode = require("fs"); + +let file, content; + +describe('embark.RemapImports', function () { + describe('Import remappings from local file', function () { + before('do the remappings', async () => { + file = new File({path: 'contracts/recursive_test_0.sol', type: Types.dappFile}); + content = await remapImports.prepareForCompilation(file); + }); + + it("should find and add remappings for all recursive imports", (done) => { + expect(file.importRemappings[0]).to.deep.equal({ + prefix: "./recursive_test_1.sol", + target: fs.dappPath(".embark/contracts/recursive_test_1.sol") + }); + expect(file.importRemappings[1]).to.deep.equal({ + prefix: "./recursive_test_2.sol", + target: fs.dappPath(".embark/contracts/recursive_test_2.sol") + }); + expect(file.importRemappings[2]).to.deep.equal({ + prefix: "embark-test-contract-0/recursive_test_3.sol", + target: fs.dappPath(".embark/node_modules/embark-test-contract-0/recursive_test_3.sol") + }); + expect(file.importRemappings[3]).to.deep.equal({ + prefix: "embark-test-contract-1/recursive_test_4.sol", + target: fs.dappPath(".embark/node_modules/embark-test-contract-1/recursive_test_4.sol") + }); + done(); + }); + + it("should update the contract content to use the remapped imports", function (done) { + expect(content).to.not.contain("./recursive_test_1.sol"); + expect(content).to.contain(".embark/contracts/recursive_test_1.sol"); + + let contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/recursive_test_0.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("./recursive_test_1.sol"); + expect(contractFromFilesystem).to.contain(".embark/contracts/recursive_test_1.sol"); + + contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/recursive_test_1.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("./recursive_test_2.sol"); + expect(contractFromFilesystem).to.contain(".embark/contracts/recursive_test_2.sol"); + + contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/recursive_test_2.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("import \"embark-test-contract-0/recursive_test_3.sol\""); + expect(contractFromFilesystem).to.contain(`import "${fs.dappPath(".embark/node_modules/embark-test-contract-0/recursive_test_3.sol")}"`); + + contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/node_modules/embark-test-contract-0/recursive_test_3.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("import \"embark-test-contract-1/recursive_test_4.sol\""); + expect(contractFromFilesystem).to.contain(`import "${fs.dappPath(".embark/node_modules/embark-test-contract-1/recursive_test_4.sol")}"`); + + done(); + }); + + }); + + describe('Import remappings from external URL', function () { + before('do the external HTTP contract remappings', async () => { + file = new File({externalUrl: 'https://github.com/embark-framework/embark/blob/master/src/test/contracts/recursive_test_0.sol', type: Types.http}); + content = await remapImports.prepareForCompilation(file); + }); + + it("should find and add remappings for all recursive imports", (done) => { + expect(file.importRemappings[0]).to.deep.equal({ + prefix: "./recursive_test_1.sol", + target: fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_1.sol") + }); + expect(file.importRemappings[1]).to.deep.equal({ + prefix: "./recursive_test_2.sol", + target: fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_2.sol") + }); + expect(file.importRemappings[2]).to.deep.equal({ + prefix: "embark-test-contract-0/recursive_test_3.sol", + target: fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/embark-test-contract-0/recursive_test_3.sol") + }); + done(); + }); + + it("should update the contract content to use the remapped imports", function (done) { + expect(content).to.not.contain("./recursive_test_1.sol"); + expect(content).to.contain(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_1.sol"); + + let contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_0.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("./recursive_test_1.sol"); + expect(contractFromFilesystem).to.contain(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_1.sol"); + + contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_1.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("./recursive_test_2.sol"); + expect(contractFromFilesystem).to.contain(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_2.sol"); + + contractFromFilesystem = fsNode.readFileSync(fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/recursive_test_2.sol")).toString(); + expect(contractFromFilesystem).to.not.contain("import \"embark-test-contract-0/recursive_test_3.sol\""); + expect(contractFromFilesystem).to.contain(`import "${fs.dappPath(".embark/contracts/embark-framework/embark/master/src/test/contracts/embark-test-contract-0/recursive_test_3.sol")}"`); + + done(); + }); + }); + +});