Skip to content

chore(build): Only suppress expected warnings from closure-make-deps #6350

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

Merged
cpcallen merged 1 commit into from
Aug 18, 2022
Merged

chore(build): Only suppress expected warnings from closure-make-deps #6350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 41 additions & 12 deletions scripts/gulpfiles/build_tasks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ gulp.sourcemaps = require('gulp-sourcemaps');

var path = require('path');
var fs = require('fs');
var execSync = require('child_process').execSync;
const {exec, execSync} = require('child_process');
var through2 = require('through2');

const clangFormat = require('clang-format');
Expand Down Expand Up @@ -336,18 +336,47 @@ function buildDeps(done) {
'tests/mocha'
];

const args = roots.map(root => `--root '${root}' `).join('');
execSync(
`closure-make-deps ${args} 2>/dev/null >'${DEPS_FILE}'`,
{stdio: 'inherit'});
function filterErrors(text) {
return text.split('\n')
.filter(
(line) => !/^WARNING /.test(line) ||
!(/Missing type declaration./.test(line) ||
/illegal use of unknown JSDoc tag/.test(line)))
Comment on lines +343 to +344
Copy link
Contributor

@BeksOmega BeksOmega Aug 15, 2022

Choose a reason for hiding this comment

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

Do you think we'll be dealing with this long enough that it would be useful to separate these out into an array?

Copy link
Collaborator Author

@cpcallen cpcallen Aug 18, 2022

Choose a reason for hiding this comment

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

I sincerely hope not. But when I find myself a fourth regexp to this conditional then it will be time to reevaluate…

.join('\n');
}

// Use grep to filter out the entries that are already in deps.js.
const testArgs = testRoots.map(root => `--root '${root}' `).join('');
execSync(
`closure-make-deps ${testArgs} 2>/dev/null \
| grep 'tests/mocha' > '${TEST_DEPS_FILE}'`,
{stdio: 'inherit'});
done();
new Promise((resolve, reject) => {
const args = roots.map(root => `--root '${root}' `).join('');
exec(
`closure-make-deps ${args} >'${DEPS_FILE}'`,
{stdio: ['inherit', 'inherit', 'pipe']},
(error, stdout, stderr) => {
console.warn(filterErrors(stderr));
if (error) {
reject(error);
} else {
resolve();
}
});
}).then(() => new Promise((resolve, reject) => {
// Use grep to filter out the entries that are already in deps.js.
const testArgs =
testRoots.map(root => `--root '${root}' `).join('');
exec(
`closure-make-deps ${testArgs} 2>/dev/null\
| grep 'tests/mocha' > '${TEST_DEPS_FILE}'`,
{stdio: ['inherit', 'inherit', 'pipe']},
(error, stdout, stderr) => {
console.warn(filterErrors(stderr));
if (error) {
reject(error);
} else {
resolve();
}
});
})).then(() => {
done();
Copy link
Contributor

@BeksOmega BeksOmega Aug 15, 2022

Choose a reason for hiding this comment

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

Is this fixing the prepare error?

Copy link
Collaborator Author

@cpcallen cpcallen Aug 18, 2022

Choose a reason for hiding this comment

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

Not as far as I know!

The switch to Promises is to allow the non-Sync version of exec to be used (which in turn is so that we can capture stderr even when the child process does exit(0)). We still need to make sure done() is called after all the rest of the work is done, though, as it was in the original version. (We could of course alternatively return a vinyl stream, but that doesn't seem applicable for this task.)

});
}

/**
Expand Down