Fixes T4954. Allow external linters to specify a minimum required version that is required.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4954: Allow version dependencies to be specified for external linters
This isn't quite ready to land, but it does work. I tested it by adding some minimum version requirements to an .arclint file and varying the values.
Diff Detail
- Repository
- rARC Arcanist
- Branch
- linter-version
- Lint
Lint Passed Severity Location Code Message Advice src/lint/linter/ArcanistExternalLinter.php:145 XHP16 TODO Comment Advice src/lint/linter/ArcanistExternalLinter.php:246 XHP16 TODO Comment Advice src/lint/linter/ArcanistExternalLinter.php:255 XHP16 TODO Comment Advice src/lint/linter/ArcanistExternalLinter.php:433 XHP16 TODO Comment Advice src/lint/linter/ArcanistLinter.php:333 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 526 Build 526: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
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.
src/lint/linter/ArcanistExternalLinter.php | ||
---|---|---|
573–575 | We could check here that the linter actually implements the getVersion function by checking $this->getVersion() !== null. |
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?
I don't quite follow what is happening here... can you elaborate or provide some diagnostic info?
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?
- Changed arclint keys.
- Added maximum version support.
- Implement a (fairly hacky) hasVersion method.
This still needs more work, namely filling in TODOs and properly testing.
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...
- Add some sort of (optional) caching to ExecFuture so that subsequent calls to $linter --version don't actually need to run the command.
- 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.