-
Notifications
You must be signed in to change notification settings - Fork 595
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
[rush] Add suppressedWarnings option #1224
Conversation
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.
I think the list of suppressions should be treated as regular expressions instead of simple substrings.
* external packages from breaking incremental builds (packages marked as "success with warnings" | ||
* will be rebuilt on each rush build). | ||
* | ||
* For example, certain npm packages with C++ bindings intermittently cause Node to display a warning |
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.
I'd remove this second paragraph.
@@ -270,6 +270,18 @@ | |||
*/ | |||
/*[LINE "HYPOTHETICAL"]*/ "hotfixChangeEnabled": false, | |||
|
|||
/** | |||
* If a warning message includes any substrings listed here, it should be treated as a standard |
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.
I think these should be regular expressions instead of simple substrings.
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.
Also the regex's should be anchored to match the whole "message". But see my note below...
@ecraig12345 did you consider --no-deprecation? |
@ecraig12345 in this code: task.stderr.on('data', (data: string) => {
writer.writeError(data);
this._hasWarningOrError = true;
// If this error matches any of the suppressed warnings from the config, write it to
// stdout instead of stderr and don't treat the task as having a warning/error.
if (this._rushConfiguration.suppressedWarnings.some((warning: string) => data.indexOf(warning) !== -1)) {
writer.write(data);
} else {
writer.writeError(data);
this._hasWarningOrError = true;
}
}); ...I'm not sure that the concept of a "warning" or "line" exists at all. The Also, the text buffer sometimes contains color escape characters, and sometimes not. There are probably other similar technical issues that a generic stream parser may need to consider. Up until now, Rush has carefully avoided getting involved with stream parsing. We do collect stderr and interpret it as "there were some warnings" and print it as a summary at the end, but otherwise Rush is pretty agnostic about the tasks that it orchestrates. I wonder if there is a different approach. For example, maybe the "filtering bad messages from a tasks's output" could be made into a separate tool, and then the build task could pipe itself through that tool. This would allow the solution to be more versatile, and developed by a larger audience, and it would give more fine-grained control over which filters are applied to which processes. On the other hand, we could also just go and fix the bad task that's doing deprecated stuff. (It may be easier than you estimated.) Just something to think about... |
How would I pass that to |
@ecraig12345 Yes, but that is sort of what you want, right? Typically a monorepo is going to have a standard toolchain that is shared by all the projects. On the other hand, if different projects have custom toolchains, then in general maybe we would want to suppress different warnings for different toolchains. How selective are you guys about the particular warnings that get suppressed? I noticed there is also a (It might also be useful to try a little harder to investigate and fix the underlying issue, since I haven't seen these issues arise very often. Maybe we can just upgrade a bad package, or remove it, or downgrade NodeJS if the code base isn't ready for it yet, etc.) |
I'm going to close this PR for a few reasons:
|
(Of course if you disagree or think I missed something, feel free to reopen this. I'm just trying to trim down the queue. :-) ) |
Add an option
suppressedWarnings
(naming negotiable) to treat certain warning substrings as standard log messages rather than errors. This is so that if those warnings show up for reasons outside a project's control, they won't break incremental builds.Background: In Fabric, we've increasingly been having problems with an intermittent warning
[DEP0097] DeprecationWarning: Using a domain property in MakeCallback is deprecated. Use the async_context variant of MakeCallback or the AsyncResource class instead.
breaking incremental builds (fabric issue). I did some investigation and figured out that this warning comes is due to the way certain packages are calling into Node's C++ layer, but I had no luck tracking down the actual problem packages. And even if I'd been able to track down the issue, it would likely have been a nightmare of getting dependencies of dependencies to upgrade before it could really be fixed.