-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Code reviewed and tested. Looks great! APPROVED |
[TIMOB-12169] Create build log with compilation
Resolves https://jira.appcelerator.org/browse/TIMOB-12169