Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rPHU46be410e4d6b: Create a PHP wrapper class for the XHPAST binary

Summary

Create a PhutilXHPASTBinary class which wraps the XHPAST binary, replacing the following functions:

xhpast_build() is replaced by PhutilXHPASTBinary::build().
xhpast_get_binary_path is replaced by PhutilXHPASTBinary::getPath().
xhpast_get_build_instructions is replaced by PhutilXHPASTBinary::getBuildInstructions().
xhpast_get_parser_future is replaced by PhutilXHPASTBinary::getParserFuture().
xhpast_is_available is replaced by PhutilXHPASTBinary::isAvailable().
xhpast_version is replaced by PhutilXHPASTBinary::getVersion().

Additionally improve handling of out-of-date XHPAST, a minor issue I stumbled across this whilst testing D11516. Basically, calling xhpast_is_available() followed by xhpast_build() followed by xhpast_is_available() has somewhat unexpected results.

Test Plan

Pre-patch, the following code caused an infinite loop. Post-patch, the loop was correctly terminated.

while (!xhpast_is_available()) {
  echo "Building\n";
  xhpast_build();
}

Diff Detail

Repository

rPHU libphutil

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

joshuaspence updated this revision to Diff 27697.Jan 27 2015, 10:02 AM

joshuaspence retitled this revision from to Improve handling of out-of-date XHPAST.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 27 2015, 10:02 AM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

Another thought that I had was to possibly get rid of the xhpast_is_available() function entirely. It is basically unused after D11516 and D11514.

epriestley accepted this revision.Jan 27 2015, 8:25 PM

epriestley edited edge metadata.

This revision is now accepted and ready to land.Jan 27 2015, 8:25 PM

Well, this is sort of potentially bad if some code tries to highlight like 10 blocks of PHP, right?

Maybe we should cache this and clear it after a successful build?

Well, this is sort of potentially bad if some code tries to highlight like 10 blocks of PHP, right?

Specifically:

Checks version;
starts build;
build takes 3 seconds;
fails at the last second because of a permissions issue;
repeat 10x?

I'm not entirely sure how to go about that in a clean way... ideally, xhpast_build() would reset the state of the xhpast_is_available() function.

joshuaspence mentioned this in D11528: Allow `ArcanistPhutilLibraryLinter` to run with an out-of-date XHPAST binary.Jan 27 2015, 9:11 PM

Allow availability to be forced

Pushing this back for review as I'm not too sure about this approach

While it's heavy, how about a PhutilPHPASTBinary class or something? I think 10 extra lines of code is worth not having spooky magic like this.

Create a wrapper class

Minor fix

joshuaspence retitled this revision from Improve handling of out-of-date XHPAST to Create a PHP wrapper class for the XHPAST binary.Feb 2 2015, 9:49 AM

joshuaspence updated this object.

joshuaspence mentioned this in D11516: Attempt to build XHPAST during unit tests.Feb 2 2015, 10:02 AM

joshuaspence added a child revision: D11516: Attempt to build XHPAST during unit tests.Feb 2 2015, 10:02 AM

joshuaspence added a child revision: D11612: Use `PhutilXHPASTBinary` methods.Feb 2 2015, 10:12 AM

joshuaspence added a child revision: D11613: Use `PhutilXHPASTBinary` methods.

joshuaspence added a child revision: D11614: Use `PhutilXHPASTBinary` methods.

I'd vaguely like to rename "xhpast" to "phpast" eventually because it no longer parses XHP, but conflating that with this might be unnecessary mess.

src/parser/xhpast/bin/PhutilXHPASTBinary.php
12	Maybe use EXPECTED_VERSION or similar to make the distinction in the comment blindingly obvious.

This revision is now accepted and ready to land.Feb 2 2015, 1:55 PM

(The name PHPAST might also be garbage -- I do like that XHPAST is more distinctive, but historically it meant "AST for the XHP grammar", and it no longer parses the XHP grammar.)

I didn't realize that XHP was a thing... anyway, I'm going to leave this as XHPAST for now. Eventually I'd like to get rid of XHPAST completely in favor of T6195: Replace XHPAST with a pure PHP implementation.