Merge pull request #533 from ethereum/solcjs-base-path-stripping-matching-solc

[solcjs] Base path stripping matching `solc`

Author: cameel

solcjs

@@ -4,6 +4,7 @@
 var originalUncaughtExceptionListeners = process.listeners("uncaughtException");
 
 var fs = require('fs-extra');
+var os = require('os');
 var path = require('path');
 var solc = require('./index.js');
 var smtchecker = require('./smtchecker.js');
@@ -34,26 +35,48 @@ function abort (msg) {
   process.exit(1);
 }
 
-function readFileCallback(path) {
-  path = program.basePath + '/' + path;
-  if (fs.existsSync(path)) {
+function readFileCallback(sourcePath) {
+  if (program.basePath)
+    sourcePath = program.basePath + '/' + sourcePath;
+  if (fs.existsSync(sourcePath)) {
     try {
-      return { 'contents': fs.readFileSync(path).toString('utf8') }
+      return { 'contents': fs.readFileSync(sourcePath).toString('utf8') }
     } catch (e) {
-      return { error: 'Error reading ' + path + ': ' + e };
+      return { error: 'Error reading ' + sourcePath + ': ' + e };
     }
   } else
-    return { error: 'File not found at ' + path}
+    return { error: 'File not found at ' + sourcePath}
 }
-function stripBasePath(path) {
-  if (program.basePath && path.startsWith(program.basePath))
-    return path.slice(program.basePath.length);
-  else
-    return path;
+
+function withUnixPathSeparators(filePath) {
+  if (os.platform !== 'win32')
+    // On UNIX-like systems forward slashes in paths are just a part of the file name.
+    return filePath;
+
+  return filePath.replace(/\\/g, "/");
+}
+
+function stripBasePath(sourcePath) {
+  const absoluteBasePath = (program.basePath ? path.resolve(program.basePath) : path.resolve('.'));
+
+  // Compared to base path stripping logic in solc this is much simpler because path.resolve()
+  // handles symlinks correctly (does not resolve them except in work dir) and strips .. segments
+  // from paths going beyond root (e.g. `/../../a/b/c` -> `/a/b/c/`). It's simpler also because it
+  // ignores less important corner cases: drive letters are not stripped from absolute paths on
+  // Windows and UNC paths are not handled in a special way (at least on Linux). Finally, it has
+  // very little test coverage so there might be more differences that we are just not aware of.
+  const absoluteSourcePath = path.resolve(sourcePath);
+  const relativeSourcePath = path.relative(absoluteBasePath, absoluteSourcePath);
+
+  if (relativeSourcePath.startsWith('../'))
+    // Path can't be made relative without stepping outside of base path so return absolute one.
+    return withUnixPathSeparators(absoluteSourcePath);
+
+  return withUnixPathSeparators(relativeSourcePath);
 }
 
 var callbacks = undefined
-if (program.basePath)
+if (program.basePath || !program.standardJson)
   callbacks = {'import': readFileCallback};
 
 if (program.standardJson) {
@@ -167,4 +190,4 @@ originalUncaughtExceptionListeners.forEach(function (listener) {
 
 if (hasError) {
   process.exit(1);
-}
+}

test/cli.js

@@ -1,5 +1,6 @@
 const tape = require('tape');
 const spawn = require('tape-spawn');
+const path = require('path');
 const pkg = require('../package.json');
 
 tape('CLI', function (t) {
@@ -62,14 +63,56 @@ tape('CLI', function (t) {
     spt.end();
   });
 
-  t.test('no-base-path', function (st) {
-    var spt = spawn(st, './solcjs --bin test/resources/importA.sol');
-    spt.stderr.match(/not found: File import callback not supported/);
+  t.test('no base path', function (st) {
+    var spt = spawn(
+      st,
+      './solcjs --bin ' +
+        'test/resources/importA.sol ' +
+        './test/resources//importA.sol ' +
+        path.resolve('test/resources/importA.sol')
+    );
+    spt.stderr.empty();
+    spt.succeeds();
+    spt.end();
+  });
+
+  t.test('relative base path', function (st) {
+    // NOTE: This and other base path tests rely on the relative ./importB.sol import in importA.sol.
+    // If base path is not stripped correctly from all source paths below, they will not be found
+    // by the import callback when it appends the base path back.
+    var spt = spawn(
+      st,
+      './solcjs --bin --base-path test/resources ' +
+        'test/resources/importA.sol ' +
+        './test/resources//importA.sol ' +
+        path.resolve('test/resources/importA.sol')
+    );
+    spt.stderr.empty();
+    spt.succeeds();
+    spt.end();
+  });
+
+  t.test('relative non canonical base path', function (st) {
+    var spt = spawn(
+      st,
+      './solcjs --bin --base-path ./test/resources ' +
+        'test/resources/importA.sol ' +
+        './test/resources//importA.sol ' +
+        path.resolve('test/resources/importA.sol')
+    );
+    spt.stderr.empty();
+    spt.succeeds();
     spt.end();
   });
 
-  t.test('base-path', function (st) {
-    var spt = spawn(st, './solcjs --bin --base-path test/resources test/resources/importA.sol');
+  t.test('absolute base path', function (st) {
+    var spt = spawn(
+      st,
+      './solcjs --bin --base-path ' + path.resolve('test/resources') + ' ' +
+        'test/resources/importA.sol ' +
+        './test/resources//importA.sol ' +
+        path.resolve('test/resources/importA.sol')
+    );
     spt.stderr.empty();
     spt.succeeds();
     spt.end();