Improved build tools (preprocessor & postprocessor)

[Rob--W]

I found many defects in the preprocessor, so I've fixed them (and added unit tests to give confidence that the features work as intended). See the commit messages for a detailled account of what was changed, and why.

In particular, nested preprocessor conditions work as expected now:

//#if FALSE
//#else
//#if CHROME
//// This should be shown, but it was not before the patch.
//#endif
//#endif

The previous can also be shortened with the new #elif command:

//#if FALSE
//#elif CHROME
//// This should be shown, but it was not before the patch.
//#endif

[Snuffleupagus]

Review progress:

• Improve getWorkerSrcFiles (builder.js)
• make.js: Less greedy comment stripping
• Fix preprocessor: nesting, error & tests

[Snuffleupagus]

When running the added unit-tests (which are really nice to have, btw), I'm getting the following output:

Assertion failed for undefined-define.js
--------------------------------------------------
EXPECTED:
//Error: Could not evaluate "notdefined" at c:\Users\Jonas\Git\pdfjs\external\builder\fixtures\undefined-define.js:2
//ReferenceError: evalmachine.<anonymous>:1
//notdefined
//^
//notdefined is not defined
--------------------------------------------------
ACTUAL
//Error: Could not evaluate "notdefined" at c:\Users\Jonas\Git\pdfjs\external\builder\fixtures\undefined-define.js:2
//ReferenceError: notdefined is not defined
--------------------------------------------------

All tests completed without errors.

Is this result expected? I'm running the tests on Windows, if that matters.

[Rob--W]

@Snuffleupagus The test failure is not expected, what Node version are you using? I'm using 0.12.5.

The exact format of the error message is an implementation detail of Node, so a slightly different error is not too surprising.

[Snuffleupagus]

I was using version 0.10.26. However, I've just updated to version 0.12.7 and now the tests pass as expected!
If there's a (simple) way to make this more robust with respect to different Node versions, that would be nice, but that is not a requirement as far as I'm concerned.

[Rob--W]

@Snuffleupagus It's possible, by setting displayErrors to false - https://nodejs.org/api/vm.html#vm_vm_runinthiscontext_code_options.

I've updated the PR with all nits.

diff --git a/external/builder/builder.js b/external/builder/builder.js
index cee1700..d3b6ed8 100644
--- a/external/builder/builder.js
+++ b/external/builder/builder.js
@@ -58,10 +58,10 @@ function preprocess(inFilename, outFilename, defines) {
       throw new Error('No JavaScript expression given at ' + loc());
     }
     try {
-      return vm.runInNewContext(code, defines);
+      return vm.runInNewContext(code, defines, {displayErrors: false});
     } catch (e) {
       throw new Error('Could not evaluate "' + code + '" at ' + loc() + '\n' +
-          e.name + ': ' + e.message);
+                      e.name + ': ' + e.message);
     }
   }
   function include(file) {
@@ -125,7 +125,7 @@ function preprocess(inFilename, outFilename, defines) {
             state = evaluateCondition(m[2]) ? STATE_IF_TRUE : STATE_IF_FALSE;
           } else if (state === STATE_ELSE_TRUE || state === STATE_ELSE_FALSE) {
             throw new Error('Found #elif after #else at ' + loc());
-          } else if (state !== STATE_IF_FALSE) {
+          } else {
             throw new Error('Found #elif without matching #if at ' + loc());
           }
           break;
@@ -172,7 +172,7 @@ function preprocess(inFilename, outFilename, defines) {
   }
   if (state !== STATE_NONE || stack.length !== 0) {
     throw new Error('Missing #endif in preprocessor for ' +
-        fs.realpathSync(inFilename));
+                    fs.realpathSync(inFilename));
   }
   if (typeof outFilename !== 'function') {
     fs.writeFileSync(outFilename, out);
@@ -319,7 +319,7 @@ function getWorkerSrcFiles(filePath) {
     files = JSON.parse(files);
   } catch (e) {
     throw new Error('Failed to parse otherFiles in ' + filePath + ' as JSON, ' +
-        e);
+                    e);
   }

   var srcFiles = files.filter(function(name) {
diff --git a/external/builder/fixtures/undefined-define-expected.js b/external/builder/fixtures/undefined-define-expected.js
index a85fcf9..67bfbca 100644
--- a/external/builder/fixtures/undefined-define-expected.js
+++ b/external/builder/fixtures/undefined-define-expected.js
@@ -1,5 +1,2 @@
 //Error: Could not evaluate "notdefined" at __filename:2
-//ReferenceError: evalmachine.<anonymous>:1
-//notdefined
-//^
-//notdefined is not defined
+//ReferenceError: notdefined is not defined
diff --git a/make.js b/make.js
index 208047c..b35b68e 100644
--- a/make.js
+++ b/make.js
@@ -605,9 +605,9 @@ function stripCommentHeaders(content, filename) {
   // Strip out all the vim/license headers.
   var notEndOfComment = '(?:[^*]|\\*(?!/))+';
   var reg = new RegExp(
-      '\n/\\* -\\*- Mode' + notEndOfComment + '\\*/\\s*' +
-      '(?:/\\*' + notEndOfComment + '\\*/\\s*|//(?!#).*\n\\s*)+' +
-      '\'use strict\';', 'g');
+    '\n/\\* -\\*- Mode' + notEndOfComment + '\\*/\\s*' +
+    '(?:/\\*' + notEndOfComment + '\\*/\\s*|//(?!#).*\n\\s*)+' +
+    '\'use strict\';', 'g');
   content = content.replace(reg, '');
   return content;
 }

[Snuffleupagus]

Really good, thanks for the patches!

[timvandermeij]

Nice work on the unit tests, too! It is great that this patch makes the build tools not only more complete, but also testable.