Page MenuHomePhabricator

External linters can now specify a version requirement.
ClosedPublic

Authored by jparise on Oct 18 2015, 4:27 AM.
Tags
Referenced Files
F14075422: D14298.diff
Thu, Nov 21, 11:22 AM
Unknown Object (File)
Wed, Nov 20, 12:39 PM
Unknown Object (File)
Fri, Nov 15, 11:00 PM
Unknown Object (File)
Tue, Nov 12, 9:34 AM
Unknown Object (File)
Sun, Nov 10, 9:59 PM
Unknown Object (File)
Sun, Nov 10, 10:30 AM
Unknown Object (File)
Fri, Nov 8, 4:07 PM
Unknown Object (File)
Wed, Nov 6, 3:39 AM
Tokens
"Like" token, awarded by joshuaspence.

Details

Summary

Linters can now use the version configuration value to specify the required
version of the external binary. The version number may be prefixed with <, <=,

, >=, or = to specify the version comparison operator (default: =).

PHP's native version_compare() function is used to perform the comparison.

Fixes T4954.

Test Plan

Tested against a sample of external linters.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jparise retitled this revision from to External linters can now specify a version requirement..
jparise updated this object.
jparise edited the test plan for this revision. (Show Details)
jparise added a reviewer: epriestley.
jparise added a subscriber: joshuaspence.

I just signed the CLA. Sorry, I thought I had already done that a while back.

Fix the version requirement regular expression.

Oh, if you're in under a corporate CLA we have to go manually link you up for now. I'll get things set up...

epriestley edited edge metadata.

This ended up really long. Actual actionable stuff:

  • Let's use = (instead of ==) as the user-facing version of the "exact version" operator.
  • Let's introduce "Upgrade Instructions" as distinct from "Install Instructions", since they won't be the same for many reasonable linters.

I have one concrete thought here first: I don't think we should leak the == PHP implementation detail into the config, and should just use = as the user-facing operator instead.

My guess is that most users won't know what version_compare() is and making =1.2 break when 1.2 and ==1.2 work seems confusing to me. I can't imagine an alternate meaning for =1.2, and the = operator to version_compare() seems like a whacky PHPism rather than a useful behavior.

I can't immediately find a precedent for the == syntax in other packaging systems either (I looked at npm and Composer) although maybe I'm missing something. Both seem (I think?) like they support an = operator but do not really document it as user-facing.

Upshot: My guess is that you picked == because it's what version_compare() does and was the most straightforward approach, but I think this is just PHP being goofy and not the best user-facing syntax we could select. Instead, let's accept = as the optional user-facing operator and just implement it as == internally?


Beyond this, I'd like to have a more solid design for the future before moving too far forward here. We don't need to implement all (or any) of this right now, but whatever we pick here will probably also be used internaly by arc (for "this binding has version requirements") and by T5055 eventually so I'd like to have the next few steps planned out reasonably well.

One concern is that this syntax isn't very powerful right now. Specifically, I'd like to be able to do these kinds of version checks at a minimum, eventually:

  • Minimum version X. Usually because we need a feature introduced in version X. This is the most common type of check we do today by far, and the current syntax is powerful enough to support it.
  • Exclude buggy version X.Y.Z This is much less common than "minimum version", but we have some of these checks today. I think only APC has a version which is so bad that we're completely unable to work around it, but in several cases with hg and git we go to fairly extreme lengths to accommodate bad versions, and I wouldn't expect third-party code to always go as far. The current syntax is not powerful enough to support this.
  • Version ranges / exclude super new versions. We don't currently do this, but may in the future. If we haven't tested version X+1, we might want to exclude it. The current syntax is not powerful enough to do this if there's also a minimum version.

(NPM also has a bunch of Semantic Versioning operators, but I'm not worried about these and they'd be easy to add later if I have some kind of semver epiphany and come to believe in the power of the ~ and ^ operators.)

To cover the "exclude" case above I think we should consider supporting a "not" operator in some form eventually. Composer appears to use !=. NPM doesn't seem to have such an operator at all, which I'm a little surprised by, although they have an || operator which can achieve the same effect (i.e., exclude 3.1.5 with >=2 <=3.1.4 || >3.1.5).

If we support a "not" operator, we need to support constraint lists because it's probably often not useful to specify !3.1.5 without specifying other constraints. We also need constraint lists to support ranges. Both Composer and NPM seem to use spaces in strings to delineate list elements. This seems reasonable to me (e.g. >=2 <3).

We could also at some point accept string|list<string>, like include and exclude, and use delineations between JSON elements to mean something. If we do this, having them mean "||" feels like it's probably best to me, since having them mean "space" looks a little silly and leads to fairly non-logical groupings: ["2", "||", "3"]. I think trading machine-parseability away to get better human readability here is reasonable, since arc and Phabricator are likely to be the only things that need to consume or enforce these rules.

So, tentatively, maybe v2 of this is "multiple AND'ed constraints, separated by spaces, with a 'not' operator", and then maybe we eventually introduce an "||" operator.


On the other side, this mechanism isn't very user friendly. In particular:

  1. It does not tell you why it wants you to upgrade.
  2. In many cases, the install instructions won't be the same as the upgrade instructions.
  3. It would maybe be nice in theory to let it warn you about unsupported/untested versions but let you continue.

(3) probably isn't relevant here (in .arclint) and I think that's a separate mechanism anyway (although it might use some the same syntax). We don't have any actual need for this today and I don't think it's meaningfully entwined with version requirements, so let's ignore it for now.

For (1), maybe it would be nice to have a version.why key or similar which is just a text strings shown to the user when they hit the version prompt. For example:

"version": ">=2.3.5 !3.1.4"
"version.why": "We rely on --good, introduced in 2.3.5, for this linter to work good. We can't use 3.1.4 because it had a serious bug with UTF8."

For (2), I think we should introduce alternate upgrade instructions. In many cases, the upgrade instructions and install instructions will differ, and in at least some cases the install instructions will be misleading. I suspect the install instructions are never useful for downgrading.

Since the "upgrade to latest version" and "upgrade to specific version" instructions are probably also different, I think it's fine to only deal with the former for now, e.g. introduce a getUpgradeInstructions() which is like the existing getInstallInstructions(). If we eventually need it, we can add getUpgradeToSpecificVersionInstructions($version) or whatever, but I suspect we may never actually have any use cases for that.

Upshot:

  • We don't need to add version.why for now, but it seems nice to have at some point and I think it would make this UX smoother. I think receiving an explanation as a user is way more compelling than just being told what to do.
  • I don't think we should print the install instructions, because they'll sometimes be incorrect or actively misleading. Either print nothing and let users figure it out, or provide a way for linters to generate upgrade instructions. If a linter legitimately has the same install/upgrade instructions, it can just call its "get install instructions" method itself.
src/lint/linter/ArcanistExternalLinter.php
392

I think this isn't really the best place to call this (conceptually, there is no guarantee we'll ever need to generate the cache version for a linter) , but I think a better place may not exist today. I'll make a note about this on T7045.

This revision now requires changes to proceed.Oct 18 2015, 1:11 PM

I had considered a getUpgradeInstructions() method and got stalled on the idea that some cases might call for a "downgrade". I suppose that's really not a good reason to forego the actual functionality, though. :)

src/lint/linter/ArcanistExternalLinter.php
392

Yes, I'm not happy with this location either. I originally tried checkBinaryConfiguration(), but then we get into a infinite dependency loop because we need to use the binary to get the version (as I believe you also discovered).

jparise edited edge metadata.

Introduce getUpgradeInstructions() for upgrade-specific instruction text.

jparise edited edge metadata.

Prefer '=' to '==' for user-facing equality.

@epriestley, I've acted on those two suggested action items.

I think a Phase 2 of this feature would involve adding a more smarter version comparison routine to libphutil that does many of the things you suggest.

epriestley edited edge metadata.

Couple of probable inlines. You should have commit access now, let me know if it gives you any issues.

src/lint/linter/ArcanistExternalLinter.php
299–302

This pattern has three "%s"'s but only two parameters (missing $verision)?

319–321

Should be $operator, not $compare_to?

This revision is now accepted and ready to land.Oct 21 2015, 4:22 PM
jparise marked 2 inline comments as done.
jparise edited edge metadata.

Fix silly errors.

This revision was automatically updated to reflect the committed changes.