Closed Bug 1212047 Opened 4 years ago Closed 4 years ago

mach eslint --setup does not report errors

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tromey, Assigned: miker)

References

Details

Attachments

(1 obsolete file)

I tried mach eslint --setup and was puzzled when it didn't have
any effect.

It turns out that the setup code ignores errors from the subprocesses
it invokes, and furthermore redirects their output to /dev/null, e.g.:

        with open(os.devnull, "w") as fnull:
            subprocess.call([npmPath, "install", "eslint", "-g"],
                            stdout=fnull, stderr=fnull)

In the end I got this working by running the commands myself (I am
not using a global install and "-g" will fail for me...); but this
would have been easier if mach had reported the errors.
I agree... we should simply show output as it can only help with debugging (and reproducing issues).
Assignee: nobody → mratcliffe
Bug 1212047 - Improve ESLint logging in case of error r?=mh+mozilla

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=mh+mozilla

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Attachment #8678208 - Flags: review?(mh+mozilla)
Attachment #8678208 - Flags: review?(mh+mozilla)
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

https://reviewboard.mozilla.org/r/23131/#review20835

::: python/mach_commands.py:264
(Diff revision 1)
> +        print("\nInstalling " + name + " using \"" + " ".join(cmd) + "\"...")

Please use string formatting (either `"{}".format()` or `"%s" % ()`)

::: python/mach_commands.py:277
(Diff revision 1)
> +            return 1
> +
> +        return 0

You might as well return True/False.
Attachment #8678208 - Attachment description: MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=mh+mozilla → MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium
Attachment #8678208 - Flags: review?(mh+mozilla)
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Attachment #8678208 - Flags: review?(mh+mozilla)
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Rather than always showing verbose and confusing information I have chosen to only show command output when errors occur.
Attachment #8678208 - Flags: review?(mh+mozilla)
This patch correctly displays errors when installing. This means that it is much clearer why an installation has failed.
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

https://reviewboard.mozilla.org/r/23131/#review20951

::: python/mach_commands.py:235
(Diff revisions 1 - 3)
> -            return 1
> +            return False

I was only asking for True/False being returned from callProcess. It's fine that you change it here too, albeit unrelated to the changes from this bug, but you're also inverting the return value, without adjusting the caller, which uses the returned value as a return code, so the new values are now opposite to what they ought to be.
Attachment #8678208 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/23131/#review20951

> I was only asking for True/False being returned from callProcess. It's fine that you change it here too, albeit unrelated to the changes from this bug, but you're also inverting the return value, without adjusting the caller, which uses the returned value as a return code, so the new values are now opposite to what they ought to be.

Well spotted.
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Fix return codes
Attachment #8678208 - Flags: review?(mh+mozilla)
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Bug 1212047 - Improve ESLint logging in case of error r?=glandium

Fix *all* return codes
Comment on attachment 8678208 [details]
MozReview Request: Bug 1212047 - Improve ESLint logging in case of error r?=glandium

https://reviewboard.mozilla.org/r/23131/#review21091

::: python/mach_commands.py:274
(Diff revision 5)
> +            return False
> +
> +        return True

Random thought: if you raise an exception instead of returning False, the caller can become:

try:
  self.callProcess("eslint"...)
  self.callProcess("eslint-plugin-mozilla"...)
  self.callProcess("eslint-plugin-react"...)
except:
  return 1

But I won't block you on this.
Attachment #8678208 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/02b4f070f3f2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Attachment #8678208 - Attachment is obsolete: true
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.