fix: Handle environment variables properly

basbossink · isaacs · commit 9c93ac39e95b · 2019-08-12T11:34:20.000-07:00
- Adapt the regular expression used to match the first line. - Create a simple module that can convert shell style variable declarations: NODE_PATH=./lib:$NODE_PATH to the equivalent batch syntax: @set=NODE_PATH=./lib:%NODE_PATH%. Furthermore the structure of the generated shim now looks like this: @SETLOCAL <variable declarations> <original content> @endlocal - Note that the new segments are only added to the file if there were any variable declarations. - The generated shell script carries over the captured variable declaration as is. - Add some extra tests to validate the behavior. - Remove some unnecessary white-space in the generated shim. PR-URL: #2 Fix: npm/npm#3380 Credit: @basbossink Close: #2 Reviewed-by: @isaacs
diff --git a/index.js b/index.js
@@ -15,7 +15,8 @@ var fs = require("graceful-fs")
 
 var mkdir = require("mkdirp")
   , path = require("path")
-  , shebangExpr = /^#\!\s*(?:\/usr\/bin\/env)?\s*([^ \t]+)(.*)$/
+  , toBatchSyntax = require("./lib/to-batch-syntax")
+  , shebangExpr = /^#\!\s*(?:\/usr\/bin\/env)?\s*([^ \t]+=[^ \t]+\s+)*\s*([^ \t]+)(.*)$/
 
 function cmdShimIfExists (from, to, cb) {
   fs.stat(from, function (er) {
@@ -63,22 +64,25 @@ function writeShim (from, to, cb) {
       if (er) return writeShim_(from, to, null, null, cb)
       var firstLine = data.trim().split(/\r*\n/)[0]
         , shebang = firstLine.match(shebangExpr)
-      if (!shebang) return writeShim_(from, to, null, null, cb)
-      var prog = shebang[1]
-        , args = shebang[2] || ""
-      return writeShim_(from, to, prog, args, cb)
+      if (!shebang) return writeShim_(from, to, null, null, null, cb)
+      var vars = shebang[1] || ""
+        , prog = shebang[2]
+        , args = shebang[3] || ""
+      return writeShim_(from, to, prog, args, vars, cb)
     })
   })
 }
 
-function writeShim_ (from, to, prog, args, cb) {
+
+function writeShim_ (from, to, prog, args, variables, cb) {
   var shTarget = path.relative(path.dirname(to), from)
     , target = shTarget.split("/").join("\\")
     , longProg
     , shProg = prog && prog.split("\\").join("/")
     , shLongProg
   shTarget = shTarget.split("\\").join("/")
   args = args || ""
+  variables = variables || ""
   if (!prog) {
     prog = "\"%~dp0\\" + target + "\""
     shProg = "\"$basedir/" + shTarget + "\""
@@ -101,13 +105,19 @@ function writeShim_ (from, to, prog, args, cb) {
   // )
   var cmd
   if (longProg) {
-    cmd = "@IF EXIST " + longProg + " (\r\n"
+    shLongProg = shLongProg.trim();
+    args = args.trim();
+    var variableDeclarationsAsBatch = toBatchSyntax.convertToSetCommands(variables) || "";
+    cmd = ((variableDeclarationsAsBatch.length > 0) ? ("@SETLOCAL\r\n"
+        + variableDeclarationsAsBatch) : "")
+        + "@IF EXIST " + longProg + " (\r\n"
         + "  " + longProg + " " + args + " " + target + " %*\r\n"
         + ") ELSE (\r\n"
         + "  @SETLOCAL\r\n"
         + "  @SET PATHEXT=%PATHEXT:;.JS;=;%\r\n"
         + "  " + prog + " " + args + " " + target + " %*\r\n"
         + ")"
+        + ((variableDeclarationsAsBatch.length > 0) ? "\r\n@ENDLOCAL" : "")
   } else {
     cmd = "@" + prog + " " + args + " " + target + " %*\r\n"
   }
@@ -141,10 +151,10 @@ function writeShim_ (from, to, prog, args, cb) {
 
     sh = sh
        + "if [ -x "+shLongProg+" ]; then\n"
-       + "  " + shLongProg + " " + args + " " + shTarget + " \"$@\"\n"
+       + "  " + variables + shLongProg + " " + args + " " + shTarget + " \"$@\"\n"
        + "  ret=$?\n"
        + "else \n"
-       + "  " + shProg + " " + args + " " + shTarget + " \"$@\"\n"
+       + "  " + variables + shProg + " " + args + " " + shTarget + " \"$@\"\n"
        + "  ret=$?\n"
        + "fi\n"
        + "exit $ret\n"
diff --git a/lib/to-batch-syntax.js b/lib/to-batch-syntax.js
@@ -0,0 +1,52 @@
+exports.replaceDollarWithPercentPair = replaceDollarWithPercentPair
+exports.convertToSetCommand = convertToSetCommand
+exports.convertToSetCommands = convertToSetCommands
+
+function convertToSetCommand(key, value) {
+    var line = ""
+    key = key || ""
+    key = key.trim()
+    value = value || ""
+    value = value.trim()
+    if(key && value && value.length > 0) {
+        line = "@SET " + key + "=" + replaceDollarWithPercentPair(value) + "\r\n"
+    }
+    return line
+}
+
+function extractVariableValuePairs(declarations) {
+    var pairs = {}
+    declarations.map(function(declaration) {
+        var split = declaration.split("=")
+        pairs[split[0]]=split[1]
+    })
+    return pairs
+}
+
+function convertToSetCommands(variableString) {
+    var variableValuePairs = extractVariableValuePairs(variableString.split(" "))
+    var variableDeclarationsAsBatch = ""
+    Object.keys(variableValuePairs).forEach(function (key) {
+        variableDeclarationsAsBatch += convertToSetCommand(key, variableValuePairs[key])
+    })
+    return variableDeclarationsAsBatch
+}
+
+function replaceDollarWithPercentPair(value) {
+    var dollarExpressions = /\$\{?([^\$@#\?\- \t{}:]+)\}?/g
+    var result = ""
+    var startIndex = 0
+    value = value || ""
+    do {
+        var match = dollarExpressions.exec(value)
+        if(match) {
+            var betweenMatches = value.substring(startIndex, match.index) || ""
+            result +=  betweenMatches + "%" + match[1] + "%"
+            startIndex = dollarExpressions.lastIndex
+        }
+    } while (dollarExpressions.lastIndex > 0)
+    result += value.substr(startIndex)
+    return result
+}
+
+
diff --git a/test/00-setup.js b/test/00-setup.js
@@ -8,6 +8,7 @@ var froms = {
   'from.exe': 'exe',
   'from.env': '#!/usr/bin/env node\nconsole.log(/hi/)\n',
   'from.env.args': '#!/usr/bin/env node --expose_gc\ngc()\n',
+  'from.env.variables': '#!/usr/bin/env NODE_PATH=./lib:$NODE_PATH node',
   'from.sh': '#!/usr/bin/sh\necho hi\n',
   'from.sh.args': '#!/usr/bin/sh -x\necho hi\n'
 }
diff --git a/test/basic.js b/test/basic.js
@@ -76,26 +76,67 @@ test('env shebang with args', function (t) {
             "\nesac"+
             "\n"+
             "\nif [ -x \"$basedir/node\" ]; then"+
-            "\n  \"$basedir/node\"  --expose_gc \"$basedir/from.env.args\" \"$@\""+
+            "\n  \"$basedir/node\" --expose_gc \"$basedir/from.env.args\" \"$@\""+
             "\n  ret=$?"+
             "\nelse "+
-            "\n  node  --expose_gc \"$basedir/from.env.args\" \"$@\""+
+            "\n  node --expose_gc \"$basedir/from.env.args\" \"$@\""+
             "\n  ret=$?"+
             "\nfi"+
             "\nexit $ret"+
             "\n")
     t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
             "@IF EXIST \"%~dp0\\node.exe\" (\r"+
-            "\n  \"%~dp0\\node.exe\"  --expose_gc \"%~dp0\\from.env.args\" %*\r"+
+            "\n  \"%~dp0\\node.exe\" --expose_gc \"%~dp0\\from.env.args\" %*\r"+
             "\n) ELSE (\r"+
             "\n  @SETLOCAL\r"+
             "\n  @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
-            "\n  node  --expose_gc \"%~dp0\\from.env.args\" %*\r"+
+            "\n  node --expose_gc \"%~dp0\\from.env.args\" %*\r"+
             "\n)")
     t.end()
   })
 })
 
+test('env shebang with variables', function (t) {
+  var from = path.resolve(fixtures, 'from.env.variables')
+  var to = path.resolve(fixtures, 'env.variables.shim')
+  cmdShim(from, to, function(er) {
+    if (er)
+      throw er
+    console.error('%j', fs.readFileSync(to, 'utf8'))
+    console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))
+
+    t.equal(fs.readFileSync(to, 'utf8'),
+            "#!/bin/sh"+
+            "\nbasedir=$(dirname \"$(echo \"$0\" | sed -e 's,\\\\,/,g')\")" +
+            "\n"+
+            "\ncase `uname` in"+
+            "\n    *CYGWIN*) basedir=`cygpath -w \"$basedir\"`;;"+
+            "\nesac"+
+            "\n"+
+            "\nif [ -x \"$basedir/node\" ]; then"+
+            "\n  NODE_PATH=./lib:$NODE_PATH \"$basedir/node\"  \"$basedir/from.env.variables\" \"$@\""+
+            "\n  ret=$?"+
+            "\nelse "+
+            "\n  NODE_PATH=./lib:$NODE_PATH node  \"$basedir/from.env.variables\" \"$@\""+
+            "\n  ret=$?"+
+            "\nfi"+
+            "\nexit $ret"+
+            "\n")
+    t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
+            "@SETLOCAL\r"+
+            "\n@SET NODE_PATH=./lib:%NODE_PATH%\r"+
+            "\n@IF EXIST \"%~dp0\\node.exe\" (\r"+
+            "\n  \"%~dp0\\node.exe\"  \"%~dp0\\from.env.variables\" %*\r"+
+            "\n) ELSE (\r"+
+            "\n  @SETLOCAL\r" +
+            "\n  @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
+            "\n  node  \"%~dp0\\from.env.variables\" %*\r"+
+            "\n)\r"+
+      "\n@ENDLOCAL")
+    t.end()
+  })
+})
+
 test('explicit shebang', function (t) {
   var from = path.resolve(fixtures, 'from.sh')
   var to = path.resolve(fixtures, 'sh.shim')
@@ -153,22 +194,22 @@ test('explicit shebang with args', function (t) {
             "\nesac" +
             "\n" +
             "\nif [ -x \"$basedir//usr/bin/sh\" ]; then" +
-            "\n  \"$basedir//usr/bin/sh\"  -x \"$basedir/from.sh.args\" \"$@\"" +
+            "\n  \"$basedir//usr/bin/sh\" -x \"$basedir/from.sh.args\" \"$@\"" +
             "\n  ret=$?" +
             "\nelse " +
-            "\n  /usr/bin/sh  -x \"$basedir/from.sh.args\" \"$@\"" +
+            "\n  /usr/bin/sh -x \"$basedir/from.sh.args\" \"$@\"" +
             "\n  ret=$?" +
             "\nfi" +
             "\nexit $ret" +
             "\n")
 
     t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
             "@IF EXIST \"%~dp0\\/usr/bin/sh.exe\" (\r" +
-            "\n  \"%~dp0\\/usr/bin/sh.exe\"  -x \"%~dp0\\from.sh.args\" %*\r" +
+            "\n  \"%~dp0\\/usr/bin/sh.exe\" -x \"%~dp0\\from.sh.args\" %*\r" +
             "\n) ELSE (\r" +
             "\n  @SETLOCAL\r"+
             "\n  @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
-            "\n  /usr/bin/sh  -x \"%~dp0\\from.sh.args\" %*\r" +
+            "\n  /usr/bin/sh -x \"%~dp0\\from.sh.args\" %*\r" +
             "\n)")
     t.end()
   })
diff --git a/test/to-batch-syntax-tests.js b/test/to-batch-syntax-tests.js
@@ -0,0 +1,25 @@
+var test = require('tap').test
+var toBatchSyntax = require('../lib/to-batch-syntax')
+
+test('replace $ expressions with % pair', function (t) {
+    var assertReplacement = function(string, expected) {
+        t.equal(toBatchSyntax.replaceDollarWithPercentPair(string), expected)
+    }
+    assertReplacement("$A", "%A%")
+    assertReplacement("$A:$B", "%A%:%B%")
+    assertReplacement("$A bla", "%A% bla")
+    assertReplacement("${A}bla", "%A%bla")
+    assertReplacement("$A $bla bla", "%A% %bla% bla")
+    assertReplacement("${A}bla ${bla}bla", "%A%bla %bla%bla")
+    assertReplacement("./lib:$NODE_PATH", "./lib:%NODE_PATH%")
+    t.end()
+})
+
+test('convert variable declaration to set command', function(t) {
+    t.equal(toBatchSyntax.convertToSetCommand("A",".lib:$A "), "@SET A=.lib:%A%\r\n")
+    t.equal(toBatchSyntax.convertToSetCommand("", ""), "")
+    t.equal(toBatchSyntax.convertToSetCommand(" ", ""), "")
+    t.equal(toBatchSyntax.convertToSetCommand(" ", " "), "")
+    t.equal(toBatchSyntax.convertToSetCommand(" ou", " ou "), "@SET ou=ou\r\n")
+    t.end()
+})
