1212047 - mach eslint --setup does not report errors

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Tom Tromey :tromey]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I agree... we should simply show output as it can only help with debugging (and reproducing issues).

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This patch correctly displays errors when installing. This means that it is much clearer why an installation has failed.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 14

[Mike Hommey [:glandium]]

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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02b4f070f3f2

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02b4f070f3f2

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/02b4f070f3f2

Comment 18

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion