Cleanup Message Generation (#726)

This PR closes #732 improves upon #712 by handling other whitespaces besides normal spaces in a more robust way. Also cleans up parts of the message/etc. generation process (using async functions).

Fix#732

Author: felixdivo

README.md

@@ -249,6 +249,7 @@ Special thanks to the people who contribute.
 - [Alex Mikhalev](https://github.com/amikhalev)
 - [Wayne Parrott](https://github.com/wayneparrott)
 - [Matt Richard](https://github.com/mattrichard)
+- [Felix Divo](https://github.com/felixdivo)
 
 ## License
 

rosidl_gen/idl_generator.js

@@ -26,7 +26,7 @@ const dots = dot.process({
   path: path.join(__dirname, '../rosidl_gen/templates'),
 });
 
-function removeExtraSpaceLines(str) {
+function removeEmptyLines(str) {
   return str.replace(/^\s*\n/gm, '');
 }
 
@@ -44,7 +44,7 @@ function generateServiceJSStruct(serviceInfo, dir) {
     '__' +
     serviceInfo.interfaceName +
     '.js';
-  const generatedCode = removeExtraSpaceLines(
+  const generatedCode = removeEmptyLines(
     dots.service({ serviceInfo: serviceInfo })
   );
   return writeGeneratedCode(dir, fileName, generatedCode);
@@ -68,7 +68,7 @@ function generateMessageJSStructFromSpec(messageInfo, dir, spec) {
     spec.msgName +
     '.js';
 
-  const generatedCode = removeExtraSpaceLines(
+  const generatedCode = removeEmptyLines(
     dots.message({
       messageInfo: messageInfo,
       spec: spec,
@@ -207,12 +207,13 @@ async function generateActionJSStruct(actionInfo, dir) {
     '__' +
     actionInfo.subFolder +
     '__' +
-    actionInfo.interfaceName;
-  const generatedCode = removeExtraSpaceLines(
+    actionInfo.interfaceName +
+    '.js';
+  const generatedCode = removeEmptyLines(
     dots.action({ actionInfo: actionInfo })
   );
-  dir = path.join(dir, `${actionInfo.pkgName}`);
-  const action = writeGeneratedCode(dir, fileName + '.js', generatedCode);
+  dir = path.join(dir, actionInfo.pkgName);
+  const action = writeGeneratedCode(dir, fileName, generatedCode);
 
   await Promise.all([
     goalMsg,
@@ -230,17 +231,15 @@ async function generateActionJSStruct(actionInfo, dir) {
 }
 
 async function generateJSStructFromIDL(pkg, dir) {
-  const results = [];
-  pkg.messages.forEach(messageInfo => {
-    results.push(generateMessageJSStruct(messageInfo, dir));
-  });
-  pkg.services.forEach(serviceInfo => {
-    results.push(generateServiceJSStruct(serviceInfo, dir));
-  });
-  pkg.actions.forEach(actionInfo => {
-    results.push(generateActionJSStruct(actionInfo, dir));
-  });
-  await Promise.all(results);
+  await Promise.all([
+    ...pkg.messages.map((messageInfo) =>
+      generateMessageJSStruct(messageInfo, dir)
+    ),
+    ...pkg.services.map((serviceInfo) =>
+      generateServiceJSStruct(serviceInfo, dir)
+    ),
+    ...pkg.actions.map((actionInfo) => generateActionJSStruct(actionInfo, dir)),
+  ]);
 }
 
 module.exports = generateJSStructFromIDL;

rosidl_gen/index.js

@@ -25,17 +25,14 @@ const installedPackagePaths = process.env.AMENT_PREFIX_PATH.split(
 );
 
 async function generateInPath(path) {
-  const results = [];
   const pkgs = await packages.findPackagesInDirectory(path);
-  pkgs.forEach(pkg => {
-    results.push(generateJSStructFromIDL(pkg, generatedRoot));
-  });
-  await Promise.all(results);
+  const pkgsInfo = Array.from(pkgs.values());
+  await Promise.all(
+    pkgsInfo.map((pkgInfo) => generateJSStructFromIDL(pkgInfo, generatedRoot))
+  );
 }
 
 async function generateAll(forcedGenerating) {
-  const results = [];
-
   // If we want to create the JavaScript files compulsively (|forcedGenerating| equals to true)
   // or the JavaScript files have not been created (|exist| equals to false),
   // all the JavaScript files will be created.
@@ -45,10 +42,9 @@ async function generateAll(forcedGenerating) {
       path.join(__dirname, 'generator.json'),
       path.join(generatedRoot, 'generator.json')
     );
-    installedPackagePaths.forEach(path => {
-      results.push(generateInPath(path));
-    });
-    await Promise.all(results);
+    await Promise.all(
+      installedPackagePaths.map((path) => generateInPath(path))
+    );
   }
 }
 

rosidl_gen/packages.js

@@ -70,13 +70,18 @@ function addInterfaceInfo(info, type, pkgMap) {
   pkg[type].push(info);
 }
 
-function findPackagesInDirectory(dir) {
+/**
+ * Collects all packages in a directory.
+ * @param {string} dir - the directory to search in
+ * @return {Promise<Map<string, object>>} A mapping from the package name to some info about it.
+ */
+async function findPackagesInDirectory(dir) {
   return new Promise((resolve, reject) => {
     let amentExecuted = true;
 
     // If there is a folder named 'share' under the root path, we consider that
     // the ament build tool has been executed and |amentExecuted| will be true.
-    fs.access(path.join(dir, 'share'), err => {
+    fs.access(path.join(dir, 'share'), (err) => {
       if (err) {
         amentExecuted = false;
       }
@@ -88,12 +93,7 @@ function findPackagesInDirectory(dir) {
         if (path.extname(file.name) === '.msg') {
           // Some .msg files were generated prior to 0.3.2 for .action files,
           // which has been disabled. So these files should be ignored here.
-          if (
-            path
-              .dirname(root)
-              .split(path.sep)
-              .pop() !== 'action'
-          ) {
+          if (path.dirname(root).split(path.sep).pop() !== 'action') {
             addInterfaceInfo(
               grabInterfaceInfo(path.join(root, file.name), amentExecuted),
               'messages',
@@ -112,6 +112,8 @@ function findPackagesInDirectory(dir) {
             'actions',
             pkgMap
           );
+        } else {
+          // we ignore all other files
         }
         next();
       });

rosidl_parser/parser.py

@@ -49,7 +49,7 @@ def get_json_object_from_msg_spec_object(msg_spec_object):
 
 if __name__ == '__main__':
     if len(sys.argv) < 3:
-        print ('Wrong number of argments')
+        print('Wrong number of argments')
         sys.exit(1)
     try:
         parser_method = getattr(parser, sys.argv[1])
@@ -69,9 +69,9 @@ def get_json_object_from_msg_spec_object(msg_spec_object):
         else:
             assert False, "unknown method '%s'" % sys.argv[1]
 
-        print (json.dumps(json_obj))
+        print(json.dumps(json_obj))
         sys.exit(0)
 
     except Exception as e:
-        print (str(e), file=sys.stderr)
+        print(str(e), file=sys.stderr)
         sys.exit(1)

rosidl_parser/py_utils.js

@@ -16,13 +16,16 @@
 
 const os = require('os');
 
-let pyUtils = {
-  getPython(py) {
+module.exports = {
+  /**
+   * Get the python executable on any platform suitable to be handed to *child_process.execFile*.
+   * @param {string} py either "python2" or "python3"
+   * @return {[string, string[]]} A command to execute directly and additional arguments as an array.
+   */
+  getPythonExecutable(py) {
     if (os.type() === 'Windows_NT') {
-      py = py === 'python' ? 'py -2' : 'py -3';
+      return ['py', [py === 'python' ? '-2' : '-3']];
     }
-    return py;
+    return [py, []];
   },
 };
-
-module.exports = pyUtils;

rosidl_parser/rosidl_parser.js

@@ -14,56 +14,52 @@
 
 'use strict';
 
-const os = require('os');
-const exec = require('child_process').exec;
-const pyUtils = require('./py_utils');
+const path = require('path');
+const execFile = require('child_process').execFile;
 
-const pythonExe = pyUtils.getPython('python3');
+const pythonExecutable = require('./py_utils').getPythonExecutable('python3');
 
-let rosidlParser = {
+const rosidlParser = {
   parseMessageFile(packageName, filePath) {
-    return this._parseFile(
-      'parse_message_file',
-      packageName,
-      filePath.replace(/\s/g, '\\ ')
-    );
+    return this._parseFile('parse_message_file', packageName, filePath);
   },
 
   parseServiceFile(packageName, filePath) {
-    return this._parseFile(
-      'parse_service_file',
-      packageName,
-      filePath.replace(/\s/g, '\\ ')
-    );
+    return this._parseFile('parse_service_file', packageName, filePath);
   },
 
   parseActionFile(packageName, filePath) {
-    return this._parseFile(
-      'parse_action_file',
-      packageName,
-      filePath.replace(/\s/g, '\\ ')
-    );
+    return this._parseFile('parse_action_file', packageName, filePath);
   },
 
-  _parseFile(...args) {
+  _parseFile(command, packageName, filePath) {
     return new Promise((resolve, reject) => {
-      exec(this._assembleCommand(args), (err, stdout, stderr) => {
-        if (err) {
-          reject(new Error(stderr));
-        } else {
-          resolve(JSON.parse(stdout));
+      const args = [
+        path.join(__dirname, 'parser.py'),
+        command,
+        packageName,
+        filePath,
+      ];
+      const [pythonExecutableFile, pythonExecutableArgs] = pythonExecutable;
+      execFile(
+        pythonExecutableFile,
+        pythonExecutableArgs.concat(args),
+        (err, stdout, stderr) => {
+          if (err) {
+            reject(
+              new Error(
+                `There was an error executing python with arguments "${JSON.stringify(
+                  args
+                )}": "${err}"; stderr was: ${stderr}`
+              )
+            );
+          } else {
+            resolve(JSON.parse(stdout));
+          }
         }
-      });
+      );
     });
   },
-
-  _assembleCommand(args) {
-    let command = `${pythonExe} ${__dirname}/parser.py`;
-    args.forEach((arg) => {
-      command += ' ' + arg;
-    });
-    return command;
-  },
 };
 
 module.exports = rosidlParser;