Page MenuHomePhabricator
Differential D14297 Diff 34892 src/applications/config/check/PhabricatorPygmentSetupCheck.php

Changeset View

Standalone View

View Options

src/applications/config/check/PhabricatorPygmentSetupCheck.php

<?php <?php
final class PhabricatorPygmentSetupCheck extends PhabricatorSetupCheck { final class PhabricatorPygmentSetupCheck extends PhabricatorSetupCheck {
public function getDefaultGroup() { public function getDefaultGroup() {
return self::GROUP_OTHER; return self::GROUP_OTHER;
} }
protected function executeChecks() { protected function executeChecks() {
$pygment = PhabricatorEnv::getEnvConfig('pygments.enabled'); $pygment = PhabricatorEnv::getEnvConfig('pygments.enabled');
if ($pygment) { if ($pygment) {
if (!Filesystem::binaryExists('pygmentize')) { if (!Filesystem::binaryExists('pygmentize')) {
$summary = pht( $summary = pht(
'You enabled pygments but the %s script is not '. 'You enabled %s but the %s script is not '.
'actually available, your %s is probably broken.', 'actually available, your %s is probably broken.',
'pygments',
'pygmentize', 'pygmentize',
'$PATH'); '$PATH');
$message = pht( $message = pht(
'The environmental variable %s does not contain %s. '. 'The environmental variable %s does not contain %s. '.
'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', phutil_tag('tt', array(), '$PATH'),
'pygmentize', phutil_tag('tt', array(), 'pygmentize'),
'pygmentize', phutil_tag('tt', array(), 'pygmentize'),
'$PATH'); phutil_tag('tt', array(), '$PATH'));
$this $this
->newIssue('pygments.enabled') ->newIssue('pygments.enabled')
tycho.tatitscheffAuthorUnsubmitted
Not Done
Inline Actions

@epriestley : can I rename this to 'pygments.notfound' ??

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_matches = null;
preg_match('/Pygments version (\d(\.\d)+)/', $stdout, $version_matches);
cspeckmimUnsubmitted
Done
Inline Actions

I'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.19

As 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 Actions

or (\d+\.\d+\.\d+)

tycho.tatitscheff: or `(\d+\.\d+\.\d+)`
cspeckmimUnsubmitted
Done
Inline Actions

Yea, 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 Actions

Seems 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 %s and the %s script is '.
'available, but does not seem to work.', 'available, but does not seem to work.',
'pygment',
tycho.tatitscheffAuthorUnsubmitted
Not Done
Inline Actions

TODO : Shall be pygments

tycho.tatitscheff: TODO : Shall be pygments
'pygmentize'); 'pygmentize');
$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 -V'));
$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 (empty($version_matches)) {
$summary = pht(
cspeckmimUnsubmitted
Done
Inline Actions

I 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:
http://php.net/manual/en/function.version-compare.php

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…
'You have enabled %s and the %s script is '.
'available, but does not seem to output a valid '.
'version number.',
cspeckmimUnsubmitted
Done
Inline Actions

Needs a space -> "available. Upgrading"

cspeckmim: Needs a space -> "available. Upgrading"
'pygments',
cspeckmimUnsubmitted
Done
Inline Actions

This 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…
'pygmentize');
$message = pht(
'Phabricator has %s available in %s, but the binary '.
'output is not a valid version number when run as %s. '.
cspeckmimUnsubmitted
Done
Inline Actions

I 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…
'Check that it is installed correctly.',
phutil_tag('tt', array(), 'pygmentize'),
phutil_tag('tt', array(), '$PATH'),
phutil_tag('tt', array(), 'pygmentize -V'));
$this
cspeckmimUnsubmitted
Done
Inline Actions

Does 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 Actions

Sems 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.
->newIssue('pygments.invalide')
->setName(pht('%s Invalide Version', 'pygmentize'))
->setSummary($summary)
->setMessage($message)
->addRelatedPhabricatorConfig('pygments.enabled');
cspeckmimUnsubmitted
Done
Inline Actions

I'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…
} else if (version_compare($version_matches[1], '2.0.0', '<')) {
tycho.tatitscheffAuthorUnsubmitted
Not Done
Inline Actions

Maybe 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…
tycho.tatitscheffAuthorUnsubmitted
Not Done
Inline Actions

I think it would be 2.1 !

In T9587#140778, @epriestley wrote:

Possibly we should wait until that makes it to a release and specifically recommend the release containing that fix ("Upgrade to 2.0.5 or newer...", or whatever the next release number is) since it's probably the highest-impact user-facing change between 1.4 and today for most users, albeit only for a subset of inputs.

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…
$summary = pht(
'You have enabled %s and the %s script is '.
'available. Upgrading %s from version %s '.
'to version %s or greater can improve performance.',
'pygments',
'pygmentize',
'pygments',
$version_matches[1],
'2.0');
$message = pht(
'You may want to upgrade %s '.
'from version %s to version %s or greater. '.
'To upgrade %s, visit '.
'%s and follow the '.
'upgrade instructions.',
phutil_tag('tt', array(), 'pygments'),
$version_matches[1],
'2.0',
phutil_tag('tt', array(), 'pygments'),
phutil_tag('a', array('href' => 'http://pygments.org'),
'pygments.org'));
$this
->newIssue('pygments.outdated')
->setName(pht('Outdated %s', 'pygmentize'))
->setSummary($summary)
->setMessage($message)
->addRelatedPhabricatorConfig('pygments.enabled');
} }
tycho.tatitscheffAuthorUnsubmitted
Done
Inline Actions

That 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, '.
Show All 16 Lines
Phabricator · Built by Phacility