Possibly the getMinimumVersion stuff should be moved into the ArcanistLinter class, as some of this stuff could be applied to other linters as well. However, there is no obvious location that I can see where the version check could be made.

Also, it could be worthwhile to provide getVersionRequirements instead of getMinimumVersion, with getVersionRequirements being able to perform more assertions (e.g. $this->getVersion() < $this->getVersionRequirements()), although I imagine that 99% of the time, it was be a minimum version, rather than a maximum version, that is specified.

Some general stuff on this and other version things:

• Currently, getVersion() returns null and false to sort of mean different things: "this isn't versioned" and "checking the version failed".
• I think we should probably separate those ideas, e.g. hasVersions() and getVersion()? And then we can make getVersion() throw, and simplify all the code.
• arc unit currently spends like 15 seconds repeatedly checking the jsonhint version. So maybe hasVersions() is actually getVersionCacheKey(), which returns null to indicate that the thing has no versions, and any string (usually path/to/command --args) to provide a cache key.
• Then, we add a cache across the entire process so jsonhint can hit the cache for each similar linter.
• I like version.min and version.max slightly more for these (vs minimum-version) maybe?

In D9161#8, @epriestley wrote:

• arc unit currently spends like 15 seconds repeatedly checking the jsonhint version. So maybe hasVersions() is actually getVersionCacheKey(), which returns null to indicate that the thing has no versions, and any string (usually path/to/command --args) to provide a cache key.
• Then, we add a cache across the entire process so jsonhint can hit the cache for each similar linter.

I don't quite follow what is happening here... can you elaborate or provide some diagnostic info?

In D9161#8, @epriestley wrote:

• I think we should probably separate those ideas, e.g. hasVersions() and getVersion()? And then we can make getVersion() throw, and simplify all the code.

I probably agree with you on this, however I wonder if this will lead to us having a large number of related methods (is this a bad thing?). What I mean is this... if a class implements the getVersion method, doesn't this automatically imply that hasVersion is true?

I don't quite follow what is happening here... can you elaborate or provide some diagnostic info?

Sure. If you run arc unit --everything --trace, I get output like this on my system:

>>> [84] <exec> $ 'jsonlint' --version
<<< [84] <exec> 201,101 us
>>> [85] <lint> ArcanistJSONLintLinter <paths = 1>
>>> [86] <exec> $ which 'jsonlint'
<<< [86] <exec> 4,346 us
>>> [87] <exec> $ 'jsonlint' '--compact' 
<<< [85] <lint> 13,127 us
>>> [88] <lint> ArcanistJSONLintLinter
<<< [87] <exec> 206,788 us
<<< [88] <lint> 204,246 us
Working Copy: Path "/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/l6thxcuxkqo4wog0" is not in any working copy.
>>> [89] <exec> $ which 'jsonlint'
<<< [89] <exec> 4,409 us
>>> [90] <exec> $ 'jsonlint' --version
<<< [90] <exec> 198,107 us
>>> [91] <lint> ArcanistJSONLintLinter <paths = 1>
>>> [92] <exec> $ which 'jsonlint'
<<< [92] <exec> 4,158 us
>>> [93] <exec> $ 'jsonlint' '--compact' 
<<< [91] <lint> 12,725 us
>>> [94] <lint> ArcanistJSONLintLinter
<<< [93] <exec> 202,100 us
<<< [94] <lint> 202,065 us
Working Copy: Path "/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/dped4df5vvso84cs" is not in any working copy.
>>> [95] <exec> $ which 'jsonlint'
<<< [95] <exec> 4,405 us
>>> [96] <exec> $ 'jsonlint' --version
<<< [96] <exec> 199,599 us
>>> [97] <lint> ArcanistJSONLintLinter <paths = 1>
>>> [98] <exec> $ which 'jsonlint'
<<< [98] <exec> 4,308 us
>>> [99] <exec> $ 'jsonlint' '--compact' 
<<< [97] <lint> 13,052 us
>>> [100] <lint> ArcanistJSONLintLinter
<<< [99] <exec> 202,621 us
<<< [100] <lint> 200,886 us
Working Copy: Path "/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/8clyq6t77dkw88ok" is not in any working copy.
>>> [101] <exec> $ which 'jsonlint'
<<< [101] <exec> 4,570 us
>>> [102] <exec> $ 'jsonlint' --version
<<< [102] <exec> 197,451 us

Particularly, note:

>>> [96] <exec> $ 'jsonlint' --version
<<< [96] <exec> 199,599 us
>>> [102] <exec> $ 'jsonlint' --version
<<< [102] <exec> 197,451 us
>>> [108] <exec> $ 'jsonlint' --version
<<< [108] <exec> 196,221 us

We build a new Linter for each test, it calls getVersion() (I'm not immediately sure why it does this right now, but it must do it as soon as we introduce minimum versions, so we can't stop this from happening), jsonlint --version takes 200ms to run, and we have about 30 tests, so we spend ~6 full seconds just checking the version of jsonlint. Other linters have similar costs.

This only really slows down unit tests so it isn't too critical that we fix this, and the complexity of introducing a cache is a little sketchy. However, I think the cache won't generate false positives (that is, the key for the cache can pretty easily capture all of the necessary state), and that having the test run quickly is really valuable. On my machine, it looks like arc unit --everything currently spends more than 50% of its runtime repeatedly checking linter versions. This is a lot of seconds that look like they're not too hard to reclaim.

Would a better solution be to use the same linter across the entire test suite? Adding some sort of cache in front of the getVersion linter seems overkill if it is only going to impact unit tests.

I've had a couple of thoughts on this and come up with a few ideas...

1. Add some sort of (optional) caching to ExecFuture so that subsequent calls to $linter --version don't actually need to run the command.
2. Use the same ArcanistLintEngine instance across the entire test suite (currently we instantiate a new lint engine for each test) and allow linters to cache their version details in the lint engine.

D11315 might help here as well.