Rework review and release modes. Use ant debug. Fixes Bug#988644

README.md

@@ -77,7 +77,11 @@ and get a binary APK response. Try this:
 
 You can generate manifests at [testmanifest.com](http://testmanifest.com/).
 
-You can attach a debugger via two tabs in Chrome
+You can attach a debugger via two tabs in Chrome, if you do this instead of `npm start`
+
+    node scripts/development-server.js debug
+
+And then load in Chrome:
 
 * [Controller Node Inspector](http://localhost:8888/debug?port=5858)
 * [Generator Node Inspector](http://localhost:8889/debug?port=5859)

bin/controller

@@ -53,19 +53,27 @@ config.withConfig(function(config) {
 
     metrics.generateApkRequest(manifestUrl);
 
-    frontController(manifestUrl, appType, config, function(err, s3res) {
+    frontController(manifestUrl, appType, config, function(err, s3res, isStream) {
       if (err) {
         return _genApkFailed(res, err);
       }
       res.type("application/vnd.android.package-archive");
       res.status(200);
-      s3res.on('data', function(chunk) {
+
-        res.write(chunk);
+      isStream = isStream || false;
-      });
+
-      s3res.on('end', function() {
+      if (isStream) {
-        res.send();
+        res.write(s3res);
-        metrics.generationApkFinished(new Date() - start);
+        metrics.generationApkFinished(new Date() - start);        
-      });
+      } else {
+        s3res.on('data', function(chunk) {
+          res.write(chunk);
+        });
+        s3res.on('end', function() {
+          res.send();
+          metrics.generationApkFinished(new Date() - start);
+        });
+      }
     });
   });
 
@@ -95,7 +103,7 @@ config.withConfig(function(config) {
   if (['development', 'review'].indexOf(config.environment) !== -1) {
     app.post('/cli_build', function(req, res) {
       var start = new Date();
-      cliDevBuild(req.body, config, function(err, s3res) {
+      cliDevBuild(req.body, config, function(err, s3resOrBlob, isBase64Stream) {
         var status = 'okay';
         var msg = '';
         var bApk = null;
@@ -104,24 +112,37 @@ config.withConfig(function(config) {
           msg = err;
           log.error(err);
         } else {
-          var buf = [];
+
+          isBase64Stream = isBase64Stream || false;
+
+
           res.type("application/vnd.android.package-archive");
           res.status(200);
-          s3res.on('data', function(chunk) {
-            buf.push(chunk);
-          });
-          s3res.on('end', function() {
-            bApk = Buffer.concat(buf).toString('base64');
 
+          if (isBase64Stream) {
             res.set('Content-Type', 'application/json');
             res.send(JSON.stringify({
               status: status,
-              apk: bApk,
+              apk: s3resOrBlob,
               message: msg
             }));
-
+            metrics.generationApkFinished(new Date() - start);        
-            metrics.generationApkFinished(new Date() - start);
+          } else {
-          });
+            var buf = [];
+            s3resOrBlob.on('data', function(chunk) {
+              buf.push(chunk);
+            });
+            s3resOrBlob.on('end', function() {
+              bApk = Buffer.concat(buf).toString('base64');
+              res.set('Content-Type', 'application/json');
+              res.send(JSON.stringify({
+                status: status,
+                apk: bApk,
+                message: msg
+              }));
+              metrics.generationApkFinished(new Date() - start);
+            });
+          }
         }
       });
     });

bin/generator

@@ -9,6 +9,8 @@ process.env.PROCESS_TYPE = 'apk-generator';
 // TODO Bug#973259
 process.env.NODE_TLS_REJECT_UNAUTHORIZED = 0;
 
+var fs = require('fs');
+
 var express = require('express');
 var hawk = require('hawk');
 
@@ -23,6 +25,9 @@ var apkSystem = require('../lib/apk_system');
 var ApkGenerator = require('../lib/apk_generator').ApkGenerator;
 var metrics = require('../lib/metrics');
 
+var BLOB_TYPE = require('../lib/apk_generator').BLOB_TYPE;
+var S3_TYPE = require('../lib/apk_generator').S3_TYPE;
+
 config.withConfig(function(config) {
   var log = require('../lib/logging')(config);
   var generator = new ApkGenerator(config.buildDir,
@@ -69,7 +74,7 @@ config.withConfig(function(config) {
 
         if (! post.manifest.url) throw new Error('missing url');
         generator.generate(post.manifest, post.zip, post.loadDir,
-                           genCb(res, post.manifest.url, start, log));
+                           genCb(res, config, post.manifest.url, start, log));
       });
     } else if ('/system/signer' === req.path) {
       apkSystem.signer(req, res, log);
@@ -96,8 +101,9 @@ config.withConfig(function(config) {
               });
 });
 
-function genCb(res, manifesturl, start, log) {
+function genCb(res, config, manifesturl, start, log) {
-  return function (err, apkFileLocation) {
+  return function (err, apkFileLocation, cleanupFn) {
+    var locationType;
     res.set('Content-Type', 'application/json');
     if (err) {
       log.error(err.stack);
@@ -109,10 +115,34 @@ function genCb(res, manifesturl, start, log) {
     } else {
       log.info('Generation of APK complete [' + manifesturl + '] streaming back');
       metrics.buildingApkFinished(manifesturl, new Date() - start);
-      res.send({
+      var resBody = {
         status: "okay",
-        apkFileLocation: apkFileLocation
+        locationType: locationType,
-      });
+      };
+
+      // Be explicit to detect deployment bugs
+      if ('release' === config.environment) {
+        resBody.locationType = S3_TYPE;
+        resBody.apkFileLocation = apkFileLocation;
+        res.send(resBody);
+        cleanupFn();
+      } else {
+        fs.readFile(apkFileLocation, {encoding:'binary'}, function(err, data) {
+          if (err) {
+            log.error('Problem reading apk' + err.toString());
+            return res.send({
+              status: "error",
+              message: err
+            });
+          }
+          resBody.locationType = BLOB_TYPE;
+          resBody.blob = new Buffer(data, 'binary').toString('base64');
+          res.send(resBody);
+          // Must be called after we readFile for the apkLocation
+          cleanupFn();
+        });
+      }
+      
     }
   };
 }

int-test/integration-test.js

@@ -72,7 +72,7 @@ config.withConfig(function(config) {
         fs.readFile(path.join('decoded', 'AndroidManifest.xml'), opt, this);
       },
       function checkApplicationZip(err/*, xml*/) {
-        test.notOk(err, 'We read AndroidManifest.xml');
+        test.notOk(err, 'We read AndroidManifest.xml ' + err);
         // xml.indexOf('android:versionName="' + manifest.version) !== -1
         if ( !! manifest.package_path) {
           var that = this;
@@ -89,7 +89,7 @@ config.withConfig(function(config) {
         fs.readFile('decoded/res/raw/manifest.json', opt, this);
       },
       function compareManifest(err, raw) {
-        test.notOk(err, 'we could read the manifest');
+        test.notOk(err, 'we could read the manifest ' + err);
         var reason = 'res/raw/manifest.json matches http version';
         var m = JSON.parse(raw);
 
@@ -116,13 +116,13 @@ config.withConfig(function(config) {
         request(desreUrl, this);
       },
       function loadDesre(err, res, body) {
-        test.notOk(err, 'requested fdesre url');
+        test.notOk(err, 'requested fdesre url ' + err);
         test.equal(res.statusCode, 200);
         desreManifest = JSON.parse(new Buffer(body).toString('utf8'));
         request(deltronUrl, this);
       },
       function loadDeltron3030(err, res, body) {
-        test.notOk(err, 'requested deltron3030 manifest');
+        test.notOk(err, 'requested deltron3030 manifest ' + err);
         test.equal(res.statusCode, 200);
         deltronManifest = JSON.parse(new Buffer(body).toString('utf8'));
         test.end();
@@ -158,15 +158,15 @@ config.withConfig(function(config) {
         exec("file t", this);
       },
       function afterCurl1File(err, stdout, stderr) {
-        test.notOk(err, 'file t check');
+        test.notOk(err, 'file t check ' + err);
         testFile(test, 't', stdout, stderr, this);
       },
       function afterCurl1FileTest(err) {
-        test.notOk(err, 'file t output checked');
+        test.notOk(err, 'file t output checked ' + err);
         testApk(test, desreManifest, this);
       },
       function afterCurl1ApkTool(err) {
-        test.notOk(err, 'apktool 1 check');
+        test.notOk(err, 'apktool 1 check ' + err);
         test.end();
       }
     );
@@ -187,11 +187,11 @@ config.withConfig(function(config) {
         testFile(test, 't', stdout, stderr, this);
       },
       function afterCurl2FileTest(err) {
-        test.notOk(err, 'file t output checked');
+        test.notOk(err, 'file t output checked ' + err);
         testApk(test, deltronManifest, this);
       },
       function(err) {
-        test.notOk(err, 'apktool 2 check');
+        test.notOk(err, 'apktool 2 check ' + err);
         test.end();
       });
   });
@@ -246,7 +246,7 @@ config.withConfig(function(config) {
         alwaysUpdating(this);
       },
       function serverCallback(err, aServer) {
-        test.notOk(err, 'always updating server started');
+        test.notOk(err, 'always updating server started ' + err);
         server = aServer;
         serverPort = server.address().port;
         alwaysUpdatingManifest = 'http://localhost:' + serverPort + '/manifest.webapp';
@@ -255,7 +255,7 @@ config.withConfig(function(config) {
         request(alwaysUpdatingUrl, this);
       },
       function afterGet1(err, res/*, body*/) {
-        test.notOk(err, 'get request has no eror');
+        test.notOk(err, 'get request has no error ' + err);
         test.equal(200, res.statusCode, 'get request was 200');
         var that = this;
         conn = mysql.createConnection(config.mysql);
@@ -279,15 +279,13 @@ config.withConfig(function(config) {
         version = row.version;
         libraryVersion = row.library_version;
         var that = this;
-        console.log('Go to sleep');
         // We have a 1 second cache
         setTimeout(function() {
-          console.log('and sending request...');
           request(alwaysUpdatingUrl, that);
         }, 2000);
       },
       function afterCurl2(err, res/*, body*/) {
-        test.notOk(err, 'no error from request');
+        test.notOk(err, 'no error from request ' + err);
         test.equal(200, res.statusCode, 'request is 200');
         var that = this;
         conn = mysql.createConnection(config.mysql);
@@ -305,7 +303,7 @@ config.withConfig(function(config) {
         }
       },
       function afterDb2(err, row) {
-        test.notOk(err);
+        test.notOk(err, 'no error from db SELECT ' + err);
         test.equal(id, row.id, 'ID is stable across updates');
         test.ok(version < row.version, 'Our version number increments ' + version + ' ' + row.version);
         test.equal(libraryVersion, row.library_version, 'Our APK Library version is stable');
@@ -327,7 +325,7 @@ config.withConfig(function(config) {
       },
       function afterDbVersionsCheck(err, rows) {
         var that = this;
-        test.notOk(err);
+        test.notOk(err, 'No error after DB Vesion Check ' + err);
         test.ok(rows.length >= 3, "We've got atleast 3 manifest urls in there now...");
         var data = {
           installed: {
@@ -352,7 +350,7 @@ config.withConfig(function(config) {
         }, 2000);// Wait out INT_TESTING caching
       },
       function afterAppUpdateRequest(err, res, body) {
-        test.notOk(err, 'No error for request to app_updates');
+        test.notOk(err, 'No error for request to app_updates ' + err);
         var outdated = JSON.parse(body).outdated;
         test.equal(1, outdated.length, 'only 1 app is out of date got ' + outdated.length);
         test.equal(outdated[0], alwaysUpdatingManifest,

lib/apk_generator.js

@@ -30,7 +30,6 @@ function cleanup(config, projectBuilder, appBuildDir) {
   }
 }
 
-//TODO not sure this works with the module pattern...
 withConfig(function(config) {
   var log = require('../lib/logging')(config);
   _.extend(ApkGenerator.prototype, {
@@ -141,18 +140,22 @@ withConfig(function(config) {
 
           fs.mkdirRecursiveSync(self.keysDir);
           var packName = androidManifestProperties.packageName;
-          projectBuilder.build(self.keysDir, manifest.url, packName, config, function(err, s3publicUrl) {
+          projectBuilder.build(self.keysDir, manifest.url, packName, config, function(err, s3publicUrlOrFilepath) {
             if (err) {
               log.error(err);
               log.error(err.stack);
               return cb(err);
             }
+            // noCache is true for CLI
             if (noCache) {
-              return cb(null, s3publicUrl);
+              // No-op for cleanup
+              return cb(null, s3publicUrlOrFilepath, function() {});
             } else {
-              cleanup(config, projectBuilder, appBuildDir);
               if (cb) {
-                cb(err, s3publicUrl);
+                // TODO: We have to cleanup after we stream this file, make less awkward
+                cb(err, s3publicUrlOrFilepath, function() {
+                  cleanup(config, projectBuilder, appBuildDir);
+                });
               }
             }
           });
@@ -170,5 +173,7 @@ withConfig(function(config) {
 });
 
 module.exports = {
-  ApkGenerator: ApkGenerator
+  ApkGenerator: ApkGenerator,
+  BLOB_TYPE: 'blob',
+  S3_TYPE: 's3'
 };

lib/apk_hash.js

@@ -23,11 +23,8 @@ withConfig(function(config) {
 
     var manifestHash = lru.get(manifestUrl);
     if (undefined !== manifestHash) {
-      console.log('APK Hash hit', manifestHash);
       log.info('APK Hash cache hit [' + HASH_TTL + '] for ', manifestUrl);
       return cb(null, manifestHash);
-    } else {
-      console.log('APK Hash cache miss, using request');
     }
 
     // TODO: duplicate code with front_controller
@@ -53,7 +50,6 @@ withConfig(function(config) {
         return cb(err);
       }
       manifestHash = sha1(body);
-      console.log('APK Hash populating cache');
       log.info('APK Hash cache ' + HASH_TTL + ' miss for ', manifestUrl);
       lru.set(manifestUrl, manifestHash, HASH_TTL);
       cb(null, manifestHash);

lib/apk_project_builder.js

@@ -330,7 +330,13 @@ _.extend(ApkProject.prototype, {
       }
 
       var manifestHash = sha1(manifestUrl);
-      signApk(manifestHash, apkLocation, unsignedPath, releasePath, cb);
+      if ('release' === config.environment) {
+        signApk(manifestHash, apkLocation, unsignedPath, releasePath, cb);
+      } else {
+        // Stream file back to client
+        log.info('Streaming back ' + apkLocation);
+        cb(null, apkLocation);
+      }
     }
     buildWithAnt();
   },

lib/build_queue.js

@@ -40,7 +40,7 @@ module.exports = function(manifestUrl, config, log, fn) {
   } else {
     db.aquireBuildLock(sha1(manifestUrl), manifestUrl, config, function(err) {
       if (err) {
-        console.log('ERROR, unable to aquire lock', manifestUrl);
+        log.warn('Unable to aquire lock', manifestUrl);
         //wip[manifestUrl] = true;
         return waitForLock(manifestUrl, log);
       } else {

lib/cli_dev_build.js

@@ -10,6 +10,8 @@ var s3;
 
 var log;
 
+var BLOB_TYPE = require('./apk_generator').BLOB_TYPE;
+
 module.exports = function(appData, config, cb) {
   if (undefined === log) {
     log = require('../lib/logging')(config);
@@ -35,9 +37,11 @@ module.exports = function(appData, config, cb) {
   };
   // TODO  we need to control where apks are built on the filesystem
   generator(config, manifestParams, appData.packageZip, loaderDirname, log,
-            function genCb(err/*, s3PublicUrl*/) {
+            function genCb(err, locationType, s3PublicUrlOrBlob) {
               if (err) {
                 cb(err);
+              } else if (locationType === BLOB_TYPE) {                
+                cb(null, s3PublicUrlOrBlob, true);
               } else {
                 var s3key = androidifier.packageName(manifestUrl) + '.apk';
                 if(undefined === s3) {

lib/db.js

@@ -117,7 +117,6 @@ withConfig(function(config) {
       conn.connect();
       conn.query(CLEAR_CACHED_APK_SQL, params, function(err, rows) {
         conn.end();
-        console.log(rows);
         var affected = 0;
         if (rows && rows.affectedRows) {
           affected = rows.affectedRows;
@@ -136,7 +135,6 @@ withConfig(function(config) {
       conn.connect();
       conn.query(CLEAR_ALL_APKS_SQL, [], function(err, rows) {
         conn.end();
-        console.log(rows);
         var affected = 0;
         if (rows && rows.affectedRows) {
           affected = rows.affectedRows;

lib/front_controller.js

@@ -15,6 +15,8 @@ var metrics = require('./metrics');
 var owaDownloader = require('./owa_downloader');
 var s3; // Lazy loaded to avoid config issues with cli.js
 
+var BLOB_TYPE = require('../lib/apk_generator').BLOB_TYPE;
+
 module.exports = function(manifestUrl, appType, config, cb) {
   var log = require('../lib/logging')(config);
   if (typeof cb !== 'function') {
@@ -51,6 +53,10 @@ function loadApk(s3key, cb) {
   s3.getApk(s3key, cb);
 }
 
+function decodeApk(blob, cb) {
+  cb(null, new Buffer(blob, 'base64'), true);
+}
+
 function cacheMissGenerateAPK(manifestUrl, appType, config, log, cacheApkFn, cb, finishedCb) {
   var loaderDirname;
 
@@ -92,13 +98,16 @@ function cacheMissGenerateAPK(manifestUrl, appType, config, log, cacheApkFn, cb,
 
     generator(config, manifestParams, zip, loaderDirname, log, genCb);
 
-    function genCb(err, s3publicUrl) {
+    function genCb(err, locationType, s3publicUrlOrBlob) {
       if (err) {
         finishedCb();
         cb(err);
+      } else if (locationType === BLOB_TYPE) {
+        finishedCb();
+        decodeApk(s3publicUrlOrBlob, cb);
       } else {
-        log.info('generator finished, updating cache ' + s3publicUrl);
+        log.info('generator finished, updating cache ' + s3publicUrlOrBlob);
-        cacheApkFn(s3publicUrl, ourApkVersion, function(err) {
+        cacheApkFn(s3publicUrlOrBlob, ourApkVersion, function(err) {
           // Tell buildQueue we're finished
           finishedCb();
           if (err) {

lib/generator_client.js

@@ -4,6 +4,8 @@
 
 var request = require('request');
 
+var BLOB_TYPE = require('../lib/apk_generator').BLOB_TYPE;
+var S3_TYPE = require('../lib/apk_generator').S3_TYPE;
 /**
  * manifest has several properties
  *   url - original manifest url
@@ -19,7 +21,6 @@ module.exports = function(config, manifest, zip, loadDir, log, cb) {
     zip: zip,
     loadDir: loadDir
   });
-
   log.info('Requesting ' + config.generator_endpoint + '/build');
   request({
     url: config.generator_endpoint + '/build',
@@ -33,7 +34,6 @@ module.exports = function(config, manifest, zip, loadDir, log, cb) {
     }
   }, function(err, res, body) {
     if (err || 200 !== res.statusCode) {
-
       if (res && res.statusCode && body) {
         log.debug(res.statusCode + ' ' + body);
         log.debug('Are you trying a review server? endpoint was ' +
@@ -51,7 +51,26 @@ module.exports = function(config, manifest, zip, loadDir, log, cb) {
         log.error(e);
       }
       if ('okay' === data.status) {
-        cb(null, data.apkFileLocation);
+        var locationType;
+        if ('release' === config.environment) {
+          locationType = S3_TYPE;
+        } else {
+          locationType = BLOB_TYPE;
+        }
+        log.debug('data.locationType=', locationType, data.locationType);
+
+        // Assert we get the expected type of location
+        if (locationType === data.locationType) {
+          if (S3_TYPE === data.locationType) {
+            cb(null, data.locationType, data.apkFileLocation);
+          } else {
+            cb(null, data.locationType, data.blob);
+          }
+
+        } else {
+          cb(new Error('Deployment error, expected ' + locationType + ', but got ' +
+                       data.apkFileLocation));
+        }
       } else {
         cb(new Error('Error in generator - ' + body));
       }

scripts/development-server.js

@@ -1,3 +1,13 @@
+/*
+This script is either run via
+
+    npm start
+
+or
+
+    node scripts/development-server.js debug
+*/
+
 var path = require('path');
 var spawn = require('child_process').spawn;
 
@@ -42,8 +52,23 @@ function wireUp(child, prefix) {
   });
 }
 
-var cChild = spawn('node-debug', ['--web-port=8888', '--debug-port=5858', '--debug-brk=false', controller]);
+var debug = 'debug' === process.argv[2];
+
+var nodeDebugCmd = path.join(__dirname, '..', 'node_modules', '.bin', 'node-debug');
+
+var cChild;
+if (debug) {
+  cChild = spawn(nodeDebugCmd, ['--web-port=8888', '--debug-port=5858', '--debug-brk=false', controller]);
+} else {
+  cChild = spawn('node', [controller]);
+}
 wireUp(cChild, 'CONTROLLER:');
 
-var gChild = spawn('node-debug', ['--web-port=8889', '--debug-port=5859', '--debug-brk=false', generator]);
+var gChild;
+if (debug) {
+  gChild = spawn(nodeDebugCmd, ['--web-port=8889', '--debug-port=5859', '--debug-brk=false', generator]);
+} else {
+  gChild = spawn('node', [generator]);
+}
 wireUp(gChild, 'GENERATOR:');
+