Page MenuHomePhabricator

Fixing so that on windows local paths to binaries can be used
ClosedPublic

Authored by bluehawk on Jun 11 2014, 11:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 9:03 AM
Unknown Object (File)
Tue, Apr 2, 5:38 AM
Unknown Object (File)
Tue, Apr 2, 5:38 AM
Unknown Object (File)
Tue, Apr 2, 5:38 AM
Unknown Object (File)
Tue, Apr 2, 5:38 AM
Unknown Object (File)
Tue, Apr 2, 5:38 AM
Unknown Object (File)
Sat, Mar 30, 10:45 PM
Unknown Object (File)
Thu, Mar 28, 11:37 AM

Details

Summary

On windows, using something like "vendor/bin/phpcs" does not work because the code tries to run "where vendor/bin/phpcs" which fails badly. So instead, when we try to test for a binary on windows, we first check if a file exists locally with that name. If it does, just use that. If someone has a file in their project with the same name as a linter, but that is not the actual linter, this could fail, but I can't imagine a case where that would happen.

Test Plan

On windows, run arc lint with a linter configured that is local e.g. "vendor/bin/phpcs"

Diff Detail

Repository
rPHU libphutil
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

bluehawk retitled this revision from to Fixing so that on windows local paths to binaries can be used.
bluehawk updated this object.
bluehawk edited the test plan for this revision. (Show Details)
bluehawk added a reviewer: epriestley.

I'm pretty sure the unit tests failed because I'm on windows. The change itself is fairly simple and shouldn't have affected them, but I don't have a system to easily test the unit tests on that isn't windows.

bluehawk edited edge metadata.

Improving it so that checking local happens after checking for a global bin in path.

Running unit tests and lint

epriestley edited edge metadata.
epriestley added inline comments.
src/filesystem/Filesystem.php
916

This can be simplified to if ($err), right?

918

Spell this as pathExists, not PathExists.

This revision now requires changes to proceed.Jun 17 2014, 5:46 PM
epriestley edited edge metadata.

Sorry, thought of one more issue.

src/filesystem/Filesystem.php
918

Oh, sorry -- this should also probably have an is_executable() check, too.

This revision now requires changes to proceed.Jun 17 2014, 9:55 PM
src/filesystem/Filesystem.php
918

That sounds totally reasonable, but when I add that it stops working. Not sure why...

$path is string(26) "F:\Work\TestRepo\bin\phpcs"
is_executable($path) is bool(false)

Maybe it's because I'm in a MinGW environment? the phpcs file is a shell script, so maybe windows or php doesn't understand that it's executable.

Is that actual file an executable, or is it something like a symlink?

Maybe it needs, like: is_executable($path) || (is_link($path) && is_executable(readlink($path)))?

It's not a symlink. It's a bash file.

$ cat bin/phpcs
#!/usr/bin/env sh
SRC_DIR="`pwd`"
cd "`dirname "$0"`"
cd "../vendor/squizlabs/php_codesniffer/scripts"
BIN_TARGET="`pwd`/phpcs"
cd "$SRC_DIR"
"$BIN_TARGET" "$@"
epriestley edited edge metadata.

Oh, gross. Okay, let's go with this for now and we'll see if it's a problem.

We could also try running which unconditionally (which should work in Unix-like Windows shells) and then fall back to where only if it's not available.

Thanks!

This revision is now accepted and ready to land.Jun 17 2014, 10:31 PM
epriestley updated this revision to Diff 23035.

Closed by commit rPHU6c3160d2b6f0 (authored by @bluehawk, committed by @epriestley).

Oh, although to figure out if we can run which or not we'd have to start by running which which.

Anyway, hopefully this is good enough in all real scenarios.