Closed Bug 1211223 Opened 5 years ago Closed 5 years ago

Error running eslint setup on Windows

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Unfocused, Assigned: miker)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached file Error log
Attempting to setup eslint using |mach eslint -s| from bug 1203520, and I'm getting the attached error log. Windows 10, latest tree, NodeJS v4.1.1 (NPM v2.14.4).
Duplicate of this bug: 1211477
I am surprised by this because we trap WindowsError.
Assignee: nobody → mratcliffe
Change to using set literal syntax r?=pbrosset
Attachment #8670192 - Flags: review?(pbrosset)
Bug 1211223 - Error running eslint setup on Windows r?=pbro

Seems like these changes were dropped when I experienced hg corruption.
Attachment #8670193 - Flags: review?(pbrosset)
I have applied both changes locally and I'm still getting the same error.
If I add some logs, I can see that the npmPath mach is trying to use to install eslint is 'c:\Program Files\nodejs\npm' which is correct, but calling this from inside the Mozilla Build shell thing still fails with: 'WindowsError: [Error 193] %1 is not a valid Win32 application'.
Comment on attachment 8670192 [details]
MozReview Request: Change to using set literal syntax r?=pbrosset

Redirecting to :gps who, I think, did the original review of the mach command.
Attachment #8670192 - Flags: review?(pbrosset) → review?(gps)
Comment on attachment 8670193 [details]
MozReview Request: Bug 1211223 - Error running eslint setup on Windows r?=pbro

Redirecting to :gps who, I think, did the original review of the mach command.
Attachment #8670193 - Flags: review?(pbrosset) → review?(gps)
See comment 5, I don't think these patches fix the problem.
Also, on IRC this week several people have been reporting that ./mach eslint has been broken on both mac and win. Mike, I think an email to a few mailing-lists about using the --setup option would be useful (or quickly fix bug 1211483 maybe).
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> See comment 5, I don't think these patches fix the problem.
> Also, on IRC this week several people have been reporting that ./mach eslint
> has been broken on both mac and win. Mike, I think an email to a few
> mailing-lists about using the --setup option would be useful (or quickly fix
> bug 1211483 maybe).

This is awkward because it works fine on both my virtual and physical Windows 10 boxes so I am completely unable to reproduce the issue... it should just work.

I am starting to suspect that this could be due to using old versions of node and npm so I need to look into that. It could also be that people are not running the command from the root of the project.

Of course, fixing bug 1211483 would make sense if ./mach eslint --setup always worked for everybody.
Attachment #8670192 - Flags: review?(gps)
Comment on attachment 8670192 [details]
MozReview Request: Change to using set literal syntax r?=pbrosset

https://reviewboard.mozilla.org/r/21339/#review19533

::: python/mach_commands.py:269
(Diff revision 1)
> -        return {
> +        return list({

[ ] is Python's list literal syntax (just like JS).

return [
   ...
]
Comment on attachment 8670193 [details]
MozReview Request: Bug 1211223 - Error running eslint setup on Windows r?=pbro

https://reviewboard.mozilla.org/r/21341/#review19535

::: python/mach_commands.py:318
(Diff revision 1)
> -        except subprocess.CalledProcessError:
> +        except (subprocess.CalledProcessError, WindowsError):

What is raising WindowsError? It is extremely rare to see WindowsError references outside low-level ctypes/ffi code calling Win32 APIs.
Attachment #8670193 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] (PTO until Nov 3) from comment #11)
> Comment on attachment 8670193 [details]
> MozReview Request: Bug 1211223 - Error running eslint setup on Windows
> r?=pbro
> 
> https://reviewboard.mozilla.org/r/21341/#review19535
> 
> ::: python/mach_commands.py:318
> (Diff revision 1)
> > -        except subprocess.CalledProcessError:
> > +        except (subprocess.CalledProcessError, WindowsError):
> 
> What is raising WindowsError? It is extremely rare to see WindowsError
> references outside low-level ctypes/ffi code calling Win32 APIs.

It is because we are within the MING32 shell... Attempting to run e.g. npm may not work if environment variables first need to be set via npm.cmd

It appears that this patch doesn't fix the issue for everybody... I suspect outdated npm or node versions could be the culprit.
Just noticed that Blair included his version info:
- NodeJS v4.1.1
- NPM v2.14.4

I will try to replicate the issue with that setup.
Attached patch eslint.patch (obsolete) — Splinter Review
Can somebody send the output of ./mach eslint -s with this patch?
Attached patch eslint.patch (obsolete) — Splinter Review
Sorry, try this one.
Attachment #8676857 - Attachment is obsolete: true
Attached patch eslint.patch (obsolete) — Splinter Review
Works on Win10
Attachment #8676862 - Attachment is obsolete: true
Attached patch eslint.patchSplinter Review
This should fix it.
Attachment #8676884 - Attachment is obsolete: true
Attachment #8676909 - Flags: review?(pbrosset)
Attachment #8670193 - Attachment is obsolete: true
Attachment #8670192 - Attachment is obsolete: true
Comment on attachment 8676909 [details] [diff] [review]
eslint.patch

Review of attachment 8676909 [details] [diff] [review]:
-----------------------------------------------------------------

Tested locally, this fixes the issue for me on Windows 10! So that's great, we need to get this in quickly.
I'm not a peer for this however, so I don't think I can R+ it.
:gps has been reviewing your earlier patches, but it looks like he is on PTO this week and the next, so transferring this to :glandium.
Attachment #8676909 - Flags: review?(pbrosset)
Attachment #8676909 - Flags: review?(mh+mozilla)
Attachment #8676909 - Flags: feedback+
Comment on attachment 8676909 [details] [diff] [review]
eslint.patch

Review of attachment 8676909 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mach_commands.py
@@ +287,5 @@
> +                except which.WhichError:
> +                    pass
> +        else:
> +            try:
> +                appPath = which.which(filename)

You might as well directly `return which.which(filename)`
Attachment #8676909 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/9200ea11d07f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
https://reviewboard.mozilla.org/r/21341/#review19535

> What is raising WindowsError? It is extremely rare to see WindowsError references outside low-level ctypes/ffi code calling Win32 APIs.

It is because we are within the MINGW32 shell... attempting to run e.g. npm may raise a WindowsError if environment variables first need to be set via npm.cmd
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.