Summary:
If you type `arc`, you currently get a fatal since $config isn't defined. Don't try to run the hook if we never built a config.
(We could build the config earlier and then run the hook anyway, but $workflow and $command would not be defined. It seems unlikely that executing the hook here is useful, since it affects only the case that the user types `arc` on its own.)
Test Plan: Typed `arc`. Typed `arc diff`, etc.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3841
Summary:
We start Sandcastle push when `arc diff` starts.
If `arc diff` throws then HHVM waits for finishing the futures.
We need to kill them sooner.
Test Plan: Will implement the hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3713
Summary: Use python from env, instead of the distro installed one in /usr/bin
Test Plan:
Ran arc anoid on a system that only had python in /usr/bin, still
works. Ran arc anoid on a system that had a python in /usr/local/bin before
/usr/bin. Still works as well
Reviewers: epriestley, alanh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3451
Summary:
- See D3422.
- Also improve some event configuration/debugging stuff.
Test Plan: Ran `arc list --trace`, set bogus/valid event handlers.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3423
Summary:
- Add zlib functions to extension functions.
- Provide a better error if the extension is actually missing.
Test Plan: Eyeballed it.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D3321
Summary:
In some cases (notably, homebrew) an installer may not control where arcanist/ and libphutil/ live and may not be able to control 'include_path'.
Allow libphutil/ to be symlinked into arcanist/externals/includes/ if all else fails.
Test Plan:
- Moved `libphutil` to `libphutilx`. Ran "arc" and got a failure.
- Symlinked it into externals/includes/, ran `arc`, got success.
- Moved it back to `libphutil`, ran `arc`, success.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, tfmeusburger
Differential Revision: https://secure.phabricator.com/D3243
Summary: I originally excluded `mysql` these under the theory that it would be impossible to test anything without it, but `arc` doesn't need it and you could have only one of the mysql-flavored extensions without having the other one. Whitelist all the mysql and mysqli extension functions as far as depenencies are concerned.
Test Plan: @floatinglomas, let me know if this fixes things for you?
Reviewers: btrahan, vrana, floatinglomas
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3119
Summary: Just like we ship with a list of extension functions, add a list of extension classes so people stop getting lint errors about DOMDocument just because some linter uses it.
Test Plan: Ran "arc lint".
Reviewers: vrana, btrahan, codeblock
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3082
Summary:
See <https://github.com/facebook/arcanist/issues/45>
Currently, when the user types `arc set-config x false`, we set it as the string "false", which is usually not desirable. We have some steps toward typed config already, but expand on what we have and move as much stuff as possible into it, including all the config settings that aren't currently documented (there are still some lint-specific and project-specific settings not present here, but this is most of it).
Also make the `phutil_libraries` key a legacy name for `load`, and `immutable_history` a legacy name for `history.immutable`. Generally the goal here is to make config simpler and bring it more in-line with Git/Mercurial, which use dotted hierarchies.
I'll add some documentation here but I think most of the changes should be fairly straightforward.
Test Plan:
- `arc set-config history.immutable on` (And similar -- sets to boolean true.)
- `arc set-config history.immutable off` (And similar -- sets to boolean false.)
- `arc set-config history.immutable derp` (And similar -- raises exception.)
- `arc set-config history.immutable ''` (And similar -- removes setting value.)
- `arc set-config --show`
- `arc get-config`
- `arc get-config base`
Reviewers: dschleimer, bos, btrahan, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1546
Differential Revision: https://secure.phabricator.com/D3045
Summary:
Phabricator, as we all know, is marketed as a fun adventure game. However, while it is occasionally fun and often an adventure, it's so far been sorely deficient in the game aspect. This patch aims to rectify that oversight. (Presence of the first two qualities is not guaranteed.)
Note: In case there's any doubt, this is not a serious suggestion. I was bored.
Test Plan: Seriously?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3026
Summary: The mbstring extension is common but not part of the PHP core, and we have a soft dependency on it but generally guard calls with function_exists(). Add the mbstring functions to the whitelist of known extension functions so lint doesn't raise errors about them if you don't have mbstring installed.
Test Plan: @ffx, can you verify this fixes the lint issue for you? Probably faster than recompiling my PHP without mbstring.
Reviewers: ffx, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2961
Summary: I think @nh needs this at some point?
Test Plan: Added bogus and valid "load" to system config, got warning and load respectively.
Reviewers: nh, dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Differential Revision: https://secure.phabricator.com/D2931
Summary: We currently print //additional// libraries, but not the core libraries, which makes diagnosing include problems more difficult than necessary.
Test Plan: ran `arc derp --trace`, got libphutil/arcanist locations
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2929
Summary: Otherwise svn diff fails when the file is binary but the "svn:mime-type = application/x-shellscript" is missing in it.
Test Plan: arc diff succeeded against deleting a binary file which doesn't have svn:mime-type = application/x-shellscript.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2915
Summary:
Delete all code related to v1 libraries in arcanist.
When users liberate a v1 library, prompt them to upgrade.
Test Plan: Reverted phabricator/ to a couple of months ago and liberated it. Got prompted to upgrade. Upgraded.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2861
Summary:
- Add flags to exit after an idle time or client count.
- Add flags to control daemonization.
- Add flags to control output.
- Add flags to skip the "hello" frame of the protocol.
- Make the client launch a server if one does not exist.
The one-time overhead to launch a server and run a command through it looks to be ~130% of the overhead to run the command directly with "hg", so even if we never run a second command we're not paying too much.
The incremental overhead to run subsequent command appears to be less than 3% of the overhead to run the command directly with "hg" (and maybe less than 1%, I'm not sure how long the computation part of a command like 'hg log' "actually" takes).
The overhead to launch a PHP client, connect to an existing server, run a command, and then print it and exit is roughly 50% of the overhead to run the command directly with "hg". So theoretically a user can achieve an amortized 2x performance increase for all 'hg' commands by aliasing 'hg' to the PHP client in their shell.
Test Plan:
- Ran servers with idle and client count limits, let them idle and/or hit their connection limits, saw them exit.
- Ran foreground and background servers.
- Ran a daemon server with redirected stdout/stderr. Verified logs appeared.
- Ran with --quiet.
- Ran clients and servers with and without --skip-hello, things work if they agree and break if they disagree. The throughput gain on this is fairly small (maybe 5%?) but it seems simple enough to keep for the moment.
- Ran serverless clients and verified that servers launched the first time, were available subsequently, and relaunched after 15 seconds idle.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2680
Test Plan: Show Full Unit Results on this diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2777
Summary: D2728 added some extensions as builtin functions to the symbol map, but users may have old caches which still list these dependencies. Bump the symbol cache version; @nodren reported that dropping caches fixed the issue for him.
Test Plan: Ran "arc lint", got cache rebuilds.
Reviewers: vrana, nodren
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1347
Differential Revision: https://secure.phabricator.com/D2746
Summary:
PHP thinks this thing is an integer... and also thinks it's not an
integer... I'm so confused... Anyway, this seems to persuade it to
use the correct overload.
Test Plan:
Ran arc patch with the incant provided in the bug report. It
successfully returned 1, where it didn't before. I didn't test the
perflab stuff; hopefully that aborts when it detects nonzero return.
Reviewers: nh
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2743
Summary:
This adds the concept of a local (i.e. working copy local) config file
to arc. This config file has the highest precedence for config
settings that may come from any config file. The config is stored in
.(git|hg|sv)/arc/config, and is read ahead of the project config by
getConfigFromAnySource().
Test Plan:
#Testing arc set-config and arc get-config
[16:57:04 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
[16:57:12 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc set-config foo bar
Set key 'foo' = 'bar' in global config.
[16:57:23 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
[16:57:26 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc set-config --local foo baz
Set key 'foo' = 'baz' in local config.
[16:57:35 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
(local) foo = baz
[16:57:39 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc set-config foo ''
Deleted key 'foo' from global config (was 'bar').
[16:57:49 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo =
(local) foo = baz
[16:57:51 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc set-config --local foo ''
Deleted key 'foo' from local config (was 'baz').
[16:58:05 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19414 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
#testing getConfigFromAnySource by means of lint
[11:26:57 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19612 $ ./bin/arc set-config lint.engine BogusLintEngine
Set key 'lint.engine' = 'BogusLintEngine' in global config.
[11:30:04 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19613 $ ./bin/arc set-config --local lint.engine DummyLintEngine
Set key 'lint.engine' = 'DummyLintEngine' in local config.
[11:30:19 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
Exception
Failed to load symbol 'DummyLintEngine'. If this symbol was recently added or moved, your library map may be out of date. You can rebuild the map by running 'arc liberate'. For more information, see: http://www.phabricator.com/docs/phabricator/article/libphutil_Libraries_User_Guide.html
(Run with --trace for a full exception trace.)
[11:30:25 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19586 $ ./bin/arc set-config --local lint.engine ''
Deleted key 'lint.engine' from local config (was 'DummyLintEngine').
[11:30:34 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
OKAY No lint warnings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2739
Summary:
It is currently causing us some pain because we use libraries in read-only directories.
Not saving the cache is still pretty bad because analysis is quite slow.
But it is better than exploding.
I considered other solutions like not .gitignoring the cache file but it would require committing it with each and every change.
I plan to resolve the note https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/linter/ArcanistPhutilLibraryLinter.php;d93bb5abd4935bc8$56-59 in longer term.
Test Plan: `arc liberate` without writable cache.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2734
Summary:
Spamming everyone who got bitten by this. We upgraded the libphutil library system recently, but the "use of undeclared function" lint check used to run only on files you touched and now runs on every file in every library you use. This means that if you don't have pcntl or ldap installed, you'll get errors about use of functions from them on every change.
Instead, ship with a list of functions which are provided by extensions that we'll ignore when calculating dependencies, so not having pcntl doesn't mean you have to excuse through irrelevant errrors every time.
Test Plan: Ran script with an unknown function, got it in the map; added it to the extension list, it vanished from the map.
Reviewers: vrana, btrahan, allenjohnashton, ddfisher, keebuhm, phleet, nodren
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1347
Differential Revision: https://secure.phabricator.com/D2728
Summary:
This need a bunch of work, but is a sort of minimum viable product for the hgdaemon.
The two ProtocolChannels implement the client and server halves of the protocol.
The Server implements the server logic, the Client implements the client logic.
Test Plan: Launched a server and client on the same repo, got hg output in ~2-3ms instead of 100ms+.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2665
Summary:
Currently, when a file has a syntax error and you run "arc liberate", the symbol analyzer will fail and throw, which will make the mapper fail and throw, which will make arc fail and throw. This gives you a stack trace pile three scripts deep which is a giant pain to analyze visually.
Instead, raise a clear message.
Test Plan: Ran "arc liberate" with a syntax error. Got useful diagnostic output instead of 30 pages of stack mess.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2659
Summary:
We added "repeat" to PhutilArgumentParser a while ago, so we can modernize the rest of the argument parsing (allow multiple --load-phutil-library flags).
Add a "--conduit-timeout" flag to allow users to set the timeout. See D2602.
Test Plan:
$ arc diff --conduit-timeout=0.01
Exception
[cURL/28] <CURLE_OPERATION_TIMEOUTED> The request took too long to complete.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-version 33
Exception
ERR-BAD-VERSION: Your 'arc' client version is '33', which is newer than the server version, '5'. Upgrade your Phabricator install.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-uri http://derp.derp/
Usage Exception: YOU NEED TO INSTALL A CERTIFICATE TO LOGIN TO PHABRICATOR
You are trying to connect to 'http://derp.derp/api/' but do not have a certificate installed for this host. Run:
$ arc install-certificate
...to install one.
$ arc diff --load-phutil-library src --load-phutil-library ../phabricator/src --trace
Loading phutil library '0' from '/INSECURE/devtools/arcanist/src'...
Loading phutil library '1' from '/INSECURE/devtools/phabricator/src'...
Reviewers: phleet, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2603
Summary:
Khan Academy is looking into lint configuration, but doesn't use ".arcconfig" because they have a large number of repositories. Making configuration more flexible generally gives us more options for onboarding installs.
- Currently, only project config (".arcconfig") can load libraries. Allow user config ("~/.arcrc") to load libraries as well.
- Currently, only project config can set lint/unit engines. Allow user config to set default lint/unit engines.
- Add some type checking to "arc set-config".
- Add "arc set-config --show".
Test Plan:
- **load**
- Ran `arc set-config load xxx`, got error about format.
- Ran `arc set-config load ["apple"]`, got warning on running 'arc' commands (no such library) but was able to run 'arc set-config' again to clear it.
- Ran `arc set-config load ["/path/to/a/lib/src/"]`, worked.
- Ran `arc list --trace`, verified my library loaded in addition to `.arcconfig` libraries.
- Ran `arc list --load-phutil-library=xxx --trace`, verified only that library loaded.
- Ran `arc list --trace --load-phutil-library=apple --trace`, got hard error about bad library.
- Set `.arcconfig` to point at a bad library, verified hard error.
- **lint.engine** / **unit.engine**
- Removed lint engine from `.arcconfig`, ran "arc lint", got a run with specified engine.
- Removed unit engine from `.arcconfig`, ran "arc unit", got a run with specified engine.
- **--show**
- Ran `arc set-config --show`.
- **misc**
- Ran `arc get-config`.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2618
Summary:
Adds a linter for v2 libraries which raises the relevant errors.
NOTE: Not hooked up anywhere yet, so this diff has no effect.
Test Plan:
Switched the ModuleLinter to LibraryLinter and ran it with a junk block to trigger errors:
>>> Lint for src/lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php:
Error (PHL3) One Class Per File
File 'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' mixes
function (id) and class/interface (ArcanistPhutilLibraryLinter)
definitions in the same file. A file which declares a class or an
interface MUST declare nothing else.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL2) Duplicate Symbol
Definition of function 'id' in
'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' in library
'arcanist' duplicates prior definition in 'utils/utils.php' in library
'phutil'.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL1) Unknown Symbol
Use of unknown class 'XYZ'. This symbol is not defined in any loaded
libphutil library.
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2597
Summary:
For `PhutilSymbolLoader` queries which include `setAncestorClass()`, we need the map of which classes/interfaces things extend/implement to issue the query efficiently.
Without this map, we need to load //every// class/interface and then do `is_subclass_of()`. This is doable, but not very performant if we don't have php-fpm warmup. There are a few performance-sensitive interfaces where we run queries like this, including some in Arcanist, where we'll never have warmup.
This map isn't particularly difficult to generate or maintain, so just include it in symbol generation and in the library map.
Also set parallelism with a flag, since it was arbitrarily hard-coded and adding flags is easy. 8 actually seems approximately optimal on my machine at least, though.
Test Plan: Ran "phutil_rebuild_map.php", opened library map, got a reasonable extension map.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2585
Summary:
Modernize `phutil_mapper.php` to prepare for killing `__init__.php`.
The current mapper is module-oriented and complex. Instead, make the mapper file-oriented and simpler.
We build a one-to-one cache of file content to symbols (built with `phutil_symbols.php`) and then write a simpler map. See some discussion in D2561.
Also make the script less messy/bad in general. It may be useful to compare this to phutil_mapper.php.
(Additionally, we now write versions into the library map and cache.)
NOTE: Nothing can read this new map right now, of course.
Test Plan: Ran "phutil_rebuild_map.php src/" in phabricator/ with --quiet, --drop-caches, etc. Verified cache file, cache behavior, and generated map output.
Reviewers: vrana, nh, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2562
Summary:
With the current order of the include_path (checking in the parent dir of
arcanist last), it is possible to load the wrong libphutil which can have
bad side effects. Instead, the first place we check in include_path should
be the parent dir of arcanist.
(The issue I ran into is that I had a checkout of libphutil in my homedir,
and I was running arc from my homedir with --load-phutil-library to load
the libraries, and since ./ is the default value for include_path, we were
loading libphutil from my homedir instead of from alongside the copy of arc
that I was running. The libphutil alongside that copy of arc worked, but my
checkout of libphutil had D2545 in it, so it was broken.)
Test Plan:
ran arc (not in my homedir) with a broken libphutil in my homedir in my homedir
and it worked (before this change it didn't)
Reviewers: jungejason, vrana, epriestley
Reviewed By: vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2576
Summary: Git supports this feature (https://git.wiki.kernel.org/index.php/Aliases), it's fairly simple to implement, and gives us a little more ammunition to dissuade users with unusual workflow requirements from actually requesting features.
Test Plan: Defined the alias described in the documentation. Ran "arc ls", "arc ls -alh", etc.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2552
Summary:
Several related changes:
- Add a "--conduit-version" flag, so you can actually diff conduit version bumps. Otherwise, the server rejects you.
- Make "arc upgrade" upgrade both libphutil and arcanist, not just arcanist.
- Bump the version number to 5. See D2527.
Test Plan:
- Ran "arc upgrade".
- Ran "arc diff". Got told there was a version issue.
- Ran "arc diff --conduit-version=4" to create this diff.
Reviewers: indiefan, nh, vrana, btrahan, jungejason, Makinde
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2528
Summary:
This is mostly an onboarding thing, but also allows "arc upload", "arc download", and "arc paste" to work anywhere on the system.
- Try to read the Phabricator install URI from arc global config if we can't find ".arcconfig".
- Build a WorkingCopy anyway if we can't find ".arcconfig", as long as we can find ".svn", ".git", or ".hg".
- Make all the workflows handle "no project ID" at least somewhat gracefully.
Test Plan:
- Ran "arc diff" in .arcconfig-less Mercurial, Git, and Subversion working copies.
- Ran "arc upload" and "arc download" from my desktop.
- Ran "arc paste" from somewhere random.
- Cleared my config and hit the error, got useful instructions.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2424
Summary: We do this in Windows, but it's not available by default in Ubuntu
Test Plan: Ran "arc" after faking the function name, got a reasonable error.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2404
Summary:
rPHUf9ba25d188c1dcf39e4454b2c6bb058e0beeaa3e adds
global function phutil_is_hiphop_runtime() to
__phutil_library_init__.php. This diff helps lint files that call that
function.
Test Plan: Lint D2365.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2366
