Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build fail issues #122

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Build fail issues #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
836a40a
Ensure that build fails are logged at the right times
andrewnicols Apr 17, 2014
2824585
Linting failures are only warnings
andrewnicols Apr 17, 2014
582efe9
Ensure consistent logging for build component failures
andrewnicols Apr 17, 2014
9ad3b94
Check the size of source files in the output file size check
andrewnicols Apr 17, 2014
92a9881
Fail on ant failures too
andrewnicols Apr 17, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/ant.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ exports.process = function (options, callback) {
fs.readdir(shifter.cwd(), function (err, files) {
if (err) {
log.err(err);
callback(err);
}
var props = [];
files.forEach(function (file) {
Expand All @@ -137,7 +138,9 @@ exports.process = function (options, callback) {
}
});
} else {
callback('no .properties files located, hit the brakes');
err = 'no .properties files located, hit the brakes';
log.err(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're about to hit the brakes, why logging the error? normally the error will be logged in the callback

callback(err);
}

});
Expand Down
8 changes: 6 additions & 2 deletions lib/builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,12 @@ exports.start = function (json, options, buildCallback) {
buildStat,
end = function (err) {
var end = new Date();
log.info('done racing, the gears are toast');
log.info('finished in ' + timer.calc(start, end) + ', pretty fast huh?');
if (err) {
log.err('Build failed: Errors encountered during build');
} else {
log.info('done racing, the gears are toast');
log.info('finished in ' + timer.calc(start, end));
}
if (buildCallback) {
buildCallback(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if buildCallback is set, there is not need to log the error, we can mix this into the previous condition.

}
Expand Down
10 changes: 8 additions & 2 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,16 @@ var runQueue = function() {
}
};

function standardExitCallback(err) {
if (err) {
process.exit(1);
}
}

function logAndExit(err) {
if (err) {
log.err(err);
process.exit(1);
standardExitCallback(err);
}
}

Expand All @@ -70,7 +76,7 @@ exports.init = function (opts, initCallback) {
log.reset(options);

if (!initCallback) {
initCallback = logAndExit;
initCallback = standardExitCallback;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not logging all the time? this is confusing.

}

if (options.cwd) {
Expand Down
22 changes: 14 additions & 8 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ var Stack = require('./stack').Stack,
fn = 'error';
}
if (!!lint && lint.length) {
log.err(file + ' contains ' + lint.length + ' lint errors');
log.warn(file + ' contains ' + lint.length + ' lint errors');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a non-bc change? it seems that lint issues were reported as errors, and now they are just warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it makes no sense for them to be marked as errors when they do not kill the build. They only stop the build if ``--fail` is used as an argument. As such, they are just warnings and should be logged as such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what log.err() was doing (no that I agree with the naming), but I prefer to keep this as it was before, just a simple log.err().

fyi, log.err() was (and still is a error log and continue), while log.error() was a log and break, this one is not longer available.


lint.forEach(function (item) {
counter = counter + 1;
Expand Down Expand Up @@ -131,7 +131,7 @@ var Stack = require('./stack').Stack,
}

if (messages.length) {
log.err(linted.name + ' contains ' + messages.length + ' lint errors');
log.warn(linted.name + ' contains ' + messages.length + ' lint errors');
messages.forEach(function (item) {
if (item && item.reason) {
counter = counter + 1;
Expand Down Expand Up @@ -363,6 +363,9 @@ var buildJS = function (mod, name, callback) {
.write(path.join(mod.buildDir, fileName, fileName + '-min.js'))
.run(function (err, result) {
if (err) {
if (typeof err.error !== 'undefined') {
err = err.error;
}
if (/file has not changed/.test(err)) {
log.warn(name + ': ' + err);
} else if (/ENOENT/.test(err)) {
Expand Down Expand Up @@ -539,14 +542,13 @@ var buildSkin = function (mod, name, callback) {
})
.cssmin()
.check()
.log('writing skin file with core wrapper')
.log('writing skin file with core wrapper for ' + skinName)
.write(path.join(mod.buildDir, name, 'assets', 'skins', skinName, name + '.css'))
.run(stack.add(function (err) {
if (err) {
log.err(err);
if (err.code === 'ENOENT') {
log.err('skin file is missing: ' + err.path);
return;
if (err.error.code === 'ENOENT') {
log.err('Skin: file is missing: ' + err.error.path);
callback(err.error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, if callback is prompting the error, why logging the error right before?

}
}

Expand All @@ -567,7 +569,11 @@ var buildSkin = function (mod, name, callback) {
})
.log('writing skin file without core wrapper')
.write(path.join(mod.buildDir, name, 'assets', 'skins', skinName, name + '-skin.css'))
.run(stack.add(function () {
.run(stack.add(function (err) {
if (err) {
log.err('Skin: ' + err.error);
return callback(err.error);
}
}));
}));
});
Expand Down
12 changes: 10 additions & 2 deletions lib/tasks/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@ Copyright (c) 2012, Yahoo! Inc. All rights reserved.
Code licensed under the BSD License:
http://yuilibrary.com/license/
*/
var fs = require('fs');

exports.check = function (options, blob, done) {
var bail = null;
var bail = null,
sourceFile;

if (blob.result.length === 0) {
bail = 'writing zero length file from ' + blob.name;
if (blob.name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't seem related to the problem. if that's the case, let's have it in a different PR

sourceFile = fs.readFileSync(blob.name).toString();
}

if (typeof sourceFile === 'undefined' || sourceFile.length !== 0) {
bail = 'writing zero length file from ' + blob.name;
}
}

done(bail, new blob.constructor(blob.result, blob));
Expand Down
Empty file added tests/assets/copy/out/empty.txt
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions tests/assets/copy/out/foo.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Some sample content to check for a non-null source
Empty file added tests/assets/copy/src/in/files/empty.txt
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions tests/assets/copy/src/in/files/foo.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Some sample content to check for a non-null source
16 changes: 14 additions & 2 deletions tests/general.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,23 @@ var tests = {
assert.isFunction(topic);
},
'and should produce an error on empty results': function(topic) {
var testFile = path.join(__dirname, 'assets/copy/src/in/files/foo.txt');

topic({}, {
result: '',
name: testFile
}, function(err) {
assert.equal(err, 'writing zero length file from ' + testFile);
});
},
'and should not produce an error on empty results when source is also empty': function(topic) {
var testFile = path.join(__dirname, 'assets/copy/src/in/files/empty.txt');

topic({}, {
result: '',
name: 'foo.txt'
name: testFile
}, function(err) {
assert.equal(err, 'writing zero length file from foo.txt');
assert.equal(err, null);
});
}
},
Expand Down