Changeset View
Standalone View
src/applications/config/check/PhabricatorPygmentSetupCheck.php
| Show All 21 Lines | if ($pygment) { | ||||
| 'You have enabled pygments, which requires '. | 'You have enabled pygments, which requires '. | ||||
| '%s to be available in your %s variable.', | '%s to be available in your %s variable.', | ||||
| '$PATH', | '$PATH', | ||||
| 'pygmentize', | 'pygmentize', | ||||
| 'pygmentize', | 'pygmentize', | ||||
| '$PATH'); | '$PATH'); | ||||
| $this | $this | ||||
| ->newIssue('pygments.enabled') | ->newIssue('pygments.enabled') | ||||
tycho.tatitscheff: @epriestley : can I rename this to 'pygments.notfound' ?? | |||||
| ->setName(pht('%s Not Found', 'pygmentize')) | ->setName(pht('%s Not Found', 'pygmentize')) | ||||
| ->setSummary($summary) | ->setSummary($summary) | ||||
| ->setMessage($message) | ->setMessage($message) | ||||
| ->addRelatedPhabricatorConfig('pygments.enabled') | ->addRelatedPhabricatorConfig('pygments.enabled') | ||||
| ->addPhabricatorConfig('environment.append-paths'); | ->addPhabricatorConfig('environment.append-paths'); | ||||
| } else { | } else { | ||||
| list($err) = exec_manual('pygmentize -h'); | list($err, $stdout) = exec_manual('pygmentize -V'); | ||||
| $version = array(); | |||||
| preg_match('/\D+(\d\.\d\.\d)\D+\d{4}-\d{4}\D+/', $stdout, $version); | |||||
cspeckmimUnsubmitted Done Inline ActionsI'm not sure what would work best here. Off-hand I think this would fail if one of the version numbers was more than 1 digit: version 2.0.19As this regex is looking for single digits. Here's some psuedo-php/regex // match the output looking for just "version x.x.x", capturing "x.x.x" in the match group preg_match('/version ([\d|\.]+)/', $stdout, $match); // strip out the "version ", should do error checking in case nothing matched! $match[0] = $match[0].substring("version ".length); That's not correct php but I'm not very familiar with string processing in php and don't have time at the moment to look up the proper way to do it. cspeckmim: I'm not sure what would work best here. Off-hand I think this would fail if one of the version… | |||||
tycho.tatitscheffAuthorUnsubmitted Done Inline Actionsor (\d+\.\d+\.\d+) tycho.tatitscheff: or `(\d+\.\d+\.\d+)` | |||||
cspeckmimUnsubmitted Done Inline ActionsYea, that's what I was thinking but would need to test. I didn't know if the + would work inside a capture group (). Maybe PHP's version_compare has available a regex it uses internally that we can utilize? cspeckmim: Yea, that's what I was thinking but would need to test. I didn't know if the `+` would work… | |||||
tycho.tatitscheffAuthorUnsubmitted Done Inline ActionsSeems so https://regex101.com/r/sK7sA1/1 ! tycho.tatitscheff: Seems so https://regex101.com/r/sK7sA1/1 ! | |||||
| if ($err) { | if ($err) { | ||||
| $summary = pht( | $summary = pht( | ||||
| 'You have enabled pygments and the %s script is '. | 'You have enabled pygments and the %s script is '. | ||||
| 'available, but does not seem to work.', | 'available, but does not seem to work.', | ||||
| 'pygmentize'); | 'pygmentize'); | ||||
Not Done Inline ActionsTODO : Shall be pygments tycho.tatitscheff: TODO : Shall be pygments | |||||
| $message = pht( | $message = pht( | ||||
| 'Phabricator has %s available in %s, but the binary '. | 'Phabricator has %s available in %s, but the binary '. | ||||
| 'exited with an error code when run as %s. Check that it is '. | 'exited with an error code when run as %s. Check that it is '. | ||||
| 'installed correctly.', | 'installed correctly.', | ||||
| phutil_tag('tt', array(), 'pygmentize'), | phutil_tag('tt', array(), 'pygmentize'), | ||||
| phutil_tag('tt', array(), '$PATH'), | phutil_tag('tt', array(), '$PATH'), | ||||
| phutil_tag('tt', array(), 'pygmentize -h')); | phutil_tag('tt', array(), 'pygmentize -h')); | ||||
| $this | $this | ||||
| ->newIssue('pygments.failed') | ->newIssue('pygments.failed') | ||||
| ->setName(pht('%s Not Working', 'pygmentize')) | ->setName(pht('%s Not Working', 'pygmentize')) | ||||
| ->setSummary($summary) | ->setSummary($summary) | ||||
| ->setMessage($message) | ->setMessage($message) | ||||
| ->addRelatedPhabricatorConfig('pygments.enabled') | ->addRelatedPhabricatorConfig('pygments.enabled') | ||||
| ->addPhabricatorConfig('environment.append-paths'); | ->addPhabricatorConfig('environment.append-paths'); | ||||
| } else { | |||||
| if (version_compare($version[1], '2.0.0', '<')) { | |||||
Done Inline ActionsI don't think this will work. Check what the output of pygmentize -V is in order to be parsed; @epriestley gives examples from T9587#140655: $ pygmentize -V Pygments version 2.0.1, (c) 2006-2014 by Georg Brandl. The version_compare will only be able to parse the 2.0.1 and not any of the rest of the line: You will need to parse the version number out of $stdout prior to using it in version_compare in order for this to be a valid comparison. cspeckmim: I don't think this will work. Check what the output of `pygmentize -V` is in order to be parsed… | |||||
| $summary = pht( | |||||
| 'You have enabled pygments and the %s script is '. | |||||
| 'available.Upgrading pygmentize to version 2.0 '. | |||||
cspeckmimUnsubmitted Done Inline ActionsNeeds a space -> "available. Upgrading" cspeckmim: Needs a space -> "available. Upgrading" | |||||
| 'greater can improve performance in syntax highlighting.'); | |||||
Done Inline ActionsThis should probably list the existing version of pygmentize found, and the second sentence here might read better if phrased a little differently: Upgrading pygmentize to version 2.0 or greater can improve performance in syntax highlighting. cspeckmim: This should probably list the existing version of pygmentize found, and the second sentence… | |||||
| $message = pht( | |||||
| 'You may want to upgrade %s. '. | |||||
| 'To upgrade Pygments, visit '. | |||||
| 'pygments.org and follow the '. | |||||
Done Inline ActionsI think these upgrade installations are too specific for a setup. Particularly in my case I do not have pip installed. The instructions on /config/edit/pygments.enabled/ indicate: To install Pygments, visit pygments.org and follow the download and install instructions. Deferring to installation instructions which pygments itself instructs. The upgrade instructions should probably be similar. cspeckmim: I think these upgrade installations are too specific for a setup. Particularly in my case I do… | |||||
| 'download and install instructions.'); | |||||
| $this | |||||
| ->newIssue('pygments.outdated') | |||||
| ->setName(pht('Outdated %s', 'pygmentize')) | |||||
| ->setSummary($summary) | |||||
Done Inline ActionsDoes this need to reference something or can a new issue identifier be created on-the-spot? cspeckmim: Does this need to reference something or can a new issue identifier be created on-the-spot? | |||||
tycho.tatitscheffAuthorUnsubmitted Done Inline ActionsSems so : I grep for pygments.failed and only found one entry. tycho.tatitscheff: Sems so : I grep for `pygments.failed` and only found one entry. | |||||
| ->setMessage($message) | |||||
| ->addRelatedPhabricatorConfig('pygments.enabled') | |||||
| ->addPhabricatorConfig('environment.append-paths'); | |||||
| } | |||||
| } | } | ||||
Done Inline ActionsI'm not very familiar with reporting issues in Phabricator - I'm guessing this line adds Phabricator's environment path to the output,and probably useful to display for this purpose. cspeckmim: I'm not very familiar with reporting issues in Phabricator - I'm guessing this line adds… | |||||
tycho.tatitscheffAuthorUnsubmitted Done Inline ActionsThat was also my thought. tycho.tatitscheff: That was also my thought. | |||||
| } | } | ||||
| } else { | } else { | ||||
| $summary = pht( | $summary = pht( | ||||
| 'Pygments should be installed and enabled '. | 'Pygments should be installed and enabled '. | ||||
| 'to provide advanced syntax highlighting.'); | 'to provide advanced syntax highlighting.'); | ||||
| $message = pht( | $message = pht( | ||||
| 'Phabricator can highlight a few languages by default, '. | 'Phabricator can highlight a few languages by default, '. | ||||
| 'but installing and enabling Pygments (a third-party highlighting '. | 'but installing and enabling Pygments (a third-party highlighting '. | ||||
| "tool) will add syntax highlighting for many more languages. \n\n". | "tool) will add syntax highlighting for many more languages. \n\n". | ||||
| 'For instructions on installing and enabling Pygments, see the '. | 'For instructions on installing and enabling Pygments, see the '. | ||||
| '%s configuration option.'."\n\n". | '%s configuration option.'."\n\n". | ||||
| 'If you do not want to install Pygments, you can ignore this issue.', | 'If you do not want to install Pygments, you can ignore this issue.', | ||||
| phutil_tag('tt', array(), 'pygments.enabled')); | phutil_tag('tt', array(), 'pygments.enabled')); | ||||
| $this | $this | ||||
| ->newIssue('pygments.noenabled') | ->newIssue('pygments.noenabled') | ||||
| ->setName(pht('Install Pygments to Improve Syntax Highlighting')) | ->setName(pht('Install Pygments to Improve Syntax Highlighting')) | ||||
| ->setSummary($summary) | ->setSummary($summary) | ||||
| ->setMessage($message) | ->setMessage($message) | ||||
| ->addRelatedPhabricatorConfig('pygments.enabled'); | ->addRelatedPhabricatorConfig('pygments.enabled'); | ||||
| } | } | ||||
Not Done Inline ActionsMaybe I should not hardcode 2.0.0 and make a variable like $min_perf_version ? This style issue apart what version should we wait to get @gd improvements in ? tycho.tatitscheff: Maybe I should not hardcode 2.0.0 and make a variable like `$min_perf_version` ?
This style… | |||||
Not Done Inline ActionsI think it would be 2.1 !
Shall I put on hold till 2.1 appears ? tycho.tatitscheff: I think it would be `2.1` !
>>! In T9587#140778, @epriestley wrote:
> Possibly we should wait… | |||||
| } | } | ||||
| } | } | ||||
@epriestley : can I rename this to 'pygments.notfound' ??