[TIMOB-12169] Create build log with compilation #6297
Conversation
| @@ -123,7 +123,17 @@ exports.run = function (logger, config, cli) { | |||
| }); | |||
| }); | |||
| } else { | |||
| next(); | |||
| if(fs.lstatSync(fulldir).isFile() && path.extname(fulldir) === '.log') { | |||
There was a problem hiding this comment.
After talking this over with the team, I think we need to just nuke everything in the build folder. We can trim this whole chunk of code (including the code before the else to:
var file = path.join(buildDir, dir);
cli.fireHook('clean.' + dir + '.pre', function () {
logger.debug(__('Deleting %s', file.cyan));
if (fs.lstatSync(file).isDirectory()) {
wrench.rmdirSyncRecursive(file);
} else {
fs.unlinkSync(file);
}
cli.fireHook('clean.' + dir + '.post', function () {
next();
});
});
| * @param {Object} logger - The logger instance | ||
| * @param {Object} config - The CLI config | ||
| * @param {Object} cli - The CLI instance | ||
| */ | ||
| Builder.prototype.config = function config(logger, config, cli) { | ||
| cli.on('cli:beginlogging', function() { |
There was a problem hiding this comment.
We should move all logging functionality out of builder.js and into build.js. Then we won't need the fileLoggerRegistered flag and we don't need the cli:beginlogging event. This chunk of code would just go in a private function.
|
I've moved the code from builder.js to build.js as you suggested, removed the beginlogging event, and added OS version info to the log output |
| @@ -97,6 +97,7 @@ function Builder(buildModule) { | |||
| /** | |||
| * Defines common variables prior to running the build's config(). This super | |||
| * function should be called prior to the platform-specific build command's config(). | |||
| * It overrides the logger to add file logging for just the build command. | |||
There was a problem hiding this comment.
No need for this comment.
| var origLoggerLog = logger.log, | ||
| platform = ti.resolvePlatform(cli.argv.platform), | ||
| logFilePath = path.join(cli.argv['project-dir'], 'build'), | ||
| logFile = path.join(logFilePath, logFileName.replace('PLATFORM', platform)), |
There was a problem hiding this comment.
What's the point of the logFilename variable? It confuses. Someone could unknowingly change the definition and then replace will not work. Just write it out:
logFile = path.join(cli.argv['project-dir'], 'build', 'build_'. + platform + '.log'),
Remove the logFilePath since it's not used.
| @@ -106,6 +106,9 @@ exports.config = function (logger, config, cli) { | |||
| } | |||
| } | |||
|
|
|||
| // start file logging here | |||
| logger = patchLogger(logger, cli); | |||
There was a problem hiding this comment.
You don't need to return logger here. Logger is passed by reference.
| logFileStream.write("\n" + prefix + args[1].replace(/\x1B\[\d+m/g, '')); | ||
|
|
||
| // call the original logger with our cleaned up args | ||
| origLoggerLog.apply(logger, [args[0], args.length > 2 ? sprintf.apply(null, args.slice(1)) : args[1]]); |
There was a problem hiding this comment.
This line fails with:
ReferenceError: sprintf is not defined
at logger.log (/Users/chris/Library/Application Support/Titanium/mobilesdk/osx/3.5.0/cli/commands/build.js:315:59)
at target.(anonymous function) (/Users/chris/appc/titanium/node_modules/winston/lib/winston/common.js:45:21)
at ioslib.simulator.launch.on.on.on.finished (/Users/chris/Library/Application Support/Titanium/mobilesdk/osx/3.5.0/iphone/cli/hooks/run.js:64:24)
at EventEmitter.emit (events.js:95:17)
at Tail.<anonymous> (/Users/chris/Library/Application Support/Titanium/mobilesdk/osx/3.5.0/node_modules/ioslib/lib/simulator.js:553:20)
at Tail.emit (events.js:95:17)
at /Users/chris/Library/Application Support/Titanium/mobilesdk/osx/3.5.0/node_modules/ioslib/node_modules/always-tail/index.js:73:32
at wrapper (fs.js:465:17)
You forgot to add sprintf = require('sprintf') at the top, but that's not enough. The Titanium SDK does not ship with sprintf. You will need to add that to the package.json in the root of the timob repo:
"sprintf": "~0.1.4",
Then you need to add the module to the Titanium SDK repo:
/path/to/titanium_mobile$ npm install --production
|
Code reviewed and tested. Looks great! APPROVED |
[TIMOB-12169] Create build log with compilation
Resolves https://jira.appcelerator.org/browse/TIMOB-12169