1342715 - mach eslint should search $PATH to find binaries (all platforms, not just linux/mac)

Status: RESOLVED FIXED

(Firefox Build System :: Lint and Formatting, defect)

Description

[Masatoshi Kimura [:emk]]

I installed node on D: drive bacause my current system drive is SSD whose disk space is precious. But mach eslint could not find the node binary even if I set $PATH environment variable. I had to create an NTFS junction only to satisfy mach eslint.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

Review commit: https://reviewboard.mozilla.org/r/145262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145262/

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

https://reviewboard.mozilla.org/r/145262/#review149286

::: tools/lint/eslint/setup_helper.py:259
(Diff revision 1)
>          for ext in [".cmd", ".exe", ""]:
>              try:
>                  return which.which(filename + ext, path=get_possible_node_paths_win())
>              except which.WhichError:
>                  pass
>      else:

Rather than handling the path expansion ourselves why not just fall back to calling which with no set of paths to search?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145262/diff/1-2/

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

https://reviewboard.mozilla.org/r/145262/#review149286

> Rather than handling the path expansion ourselves why not just fall back to calling which with no set of paths to search?

I was originally trying to keep the which_path function a bit simpler, especially due to all the try/except. However I've reworked that and added a function to handle the try/except, so I think it is clearer overall now.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

https://reviewboard.mozilla.org/r/145262/#review149778

Do we actually need to do lookup for multiple file extensions? Which claims that on windows it supports finding anything with PATHEXT extensions and a simple test on my machine confirms that a search for "node" finds "node.EXE".

I think we can simplify this all down to:

    if (windows)
        try which for get_possible_node_paths_win()
        
    try which for the default path
    
Or invert that if you want to test the default path first.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

https://reviewboard.mozilla.org/r/145262/#review149778

Unfortunately for npm we need to find the "npm.cmd". If we don't specify an extension, then it finds the "npm" file. This seems to be hard-coded in the which module, and the only way around it is to specify the filename including the extension.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8873830 [details]
Bug 1342715 - Allow node to be in the PATH on Windows, rather than just a set directory.

https://reviewboard.mozilla.org/r/145262/#review150840

I think there's something nicer we can do here but I'm going to tinker myself rather than hold up this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c0b44852e1e
Allow node to be in the PATH on Windows, rather than just a set directory. r=mossop

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c0b44852e1e