<div dir="ltr"><div><div>I believe we should do option 2, warnings should turn the job orange, and I agree with gps that we should make this part of `mach build`.<br><br></div>We spent a fair amount of time ramping up the eslint work, and I think hiding warnings is the wrong direction. Ideally we could implement something that only turns the job orange if the warning count increases, this will push developers to decrease the warning count overtime until it reaches 0 where it will stay.<br><br></div><div>- jared<br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 18, 2016 at 3:05 PM, Gregory Szorc <span dir="ltr"><<a href="mailto:gps@mozilla.com" target="_blank">gps@mozilla.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Fri, Jul 15, 2016 at 5:51 AM, Andrew Halberstadt <span dir="ltr"><<a href="mailto:ahalberstadt@mozilla.com" target="_blank">ahalberstadt@mozilla.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">As the original creators, the devtools team has largely been maintaining<br>
the eslint task, mach command and other related infrastructure. As this<br>
is outside of their team's scope, they've asked us (Engineering<br>
Productivity) to take over maintenance.<br>
<br>
We will maintain anything related to the taskcluster task, docker image,<br>
mach command, or any other python related bits. We *will not* deal with<br>
setting the style rules, fixing errors/warnings or custom eslint<br>
plugins. I strongly believe that rules and plugins should be set and<br>
maintained by developers themselves. Please file bugs under Testing::Lint.<br>
<br>
Which brings us to warnings. In the past, the devtools team has sunk a<br>
fair amount of work trying to keep warnings from bloating the log too<br>
much. But fixing them should be the responsibility of patch authors<br>
and/or module owners. The current state where warnings slip in because<br>
the job doesn't burn, but then the log is hard to read, is not a good<br>
situation. Which means there are 3 options:<br>
<br>
1) Warnings should not be logged in automation<br>
2) Warnings should turn the job orange<br>
3) Everyone should accept that there will always be warnings in the log<br>
<br>
I prefer option 1), paired with some kind of --enable-warnings flag that<br>
can be used locally but is turned off in automation. If there are no<br>
objections I will make this change in the coming weeks.<br>
<br>
One final note, I will be refactoring the eslint infrastructure to be<br>
based on a tool called mozlint. This will make it easier for us to<br>
integrate eslint in various places (e.g mozreview). I expect this work<br>
to be mostly transparent to developers.<br></blockquote><div><br></div></div></div><div>AFAIK we currently require developers to run `mach eslint` manually. There is a Mercurial hook to run on checkin. But since it isn't advertised by `mach mercurial-setup`, I'm not sure how many people have it installed.<br><br></div><div>Perhaps we could find a way to shoehorn running eslint as part of `mach build`. We could borrow what we do for C++ and print the warnings list at the end of the build, raising visibility. And depending on what we decide to do about warnings, we could fail local builds if there are warnings.<br><br></div><div>Weighing in on warnings, I think they should be fatal. If a warning isn't fatal, it will just be ignored. Bugs and product quality/stability will ensue.<br><br></div><div>There are a few reasons to not have warnings as errors everywhere:<br><br></div><div>1) Developer convenience. Sometimes you just want to hack without the compiler screaming at you. Workaround: `mach build --lenient` or something.<br></div><div>2) Determinism. Unless you can guarantee the tools detecting warnings are the same everywhere, your local machine may get different warnings from automation and vice-versa. This can be resolved by running the same tools everywhere. If we're not pinning the Node package versions in tree, we should be doing that to ensure ESLint is behaving consistently for any given source checkout revision.<br></div></div></div></div>
<br>_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
<br></blockquote></div><br></div></div></div></div>