This adds some functional support for building XHPast on Windows.
Refs T15011
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15011: Build a Phorge Developer Environment
I deleted src/parser/xhpast/bin/xhpast.exe and ran arc lint src/filesystem/FileFinder.php and verified that a new xhpast.exe was produced and linted.
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- xhpastwin
- Lint
Lint Warnings Severity Location Code Message Warning support\xhpast\make.bat:20 TXT3 Line Too Long Warning support\xhpast\make.bat:21 TXT3 Line Too Long Warning support\xhpast\make.bat:27 TXT3 Line Too Long Warning support\xhpast\unistd.h:43 TXT3 Line Too Long - Unit
Test Failures - Build Status
Buildable 1055 Build 1055: arc lint + arc unit
| Time | Test | |
|---|---|---|
| 14 ms | ArcanistBundleTestCase::testDisjointHunks EXCEPTION (Exception): Can't parse an empty diff!
#0 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\ArcanistBundle.php(191): ArcanistDiffParser->parseDiff('')
#1 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\__tests__\ArcanistBundleTestCase.php(33): ArcanistBundle::newFromDiff('')
| |
| 15 ms | ArcanistBundleTestCase::testMergeHunks EXCEPTION (Exception): Can't parse an empty diff!
#0 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\ArcanistBundle.php(191): ArcanistDiffParser->parseDiff('')
#1 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\__tests__\ArcanistBundleTestCase.php(33): ArcanistBundle::newFromDiff('')
| |
| 14 ms | ArcanistBundleTestCase::testNonlocalTrailingNewline EXCEPTION (Exception): Can't parse an empty diff!
#0 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\ArcanistBundle.php(191): ArcanistDiffParser->parseDiff('')
#1 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\__tests__\ArcanistBundleTestCase.php(33): ArcanistBundle::newFromDiff('')
| |
| 13 ms | ArcanistBundleTestCase::testTrailingContext EXCEPTION (Exception): Can't parse an empty diff!
#0 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\ArcanistBundle.php(191): ArcanistDiffParser->parseDiff('')
#1 C:\Users\cspeckrun\Source\phorge\arcanist\src\parser\__tests__\ArcanistBundleTestCase.php(33): ArcanistBundle::newFromDiff('')
| |
| 4 ms | ArcanistBaseCommitParserTestCase::testBasics 2 assertion(s) passed. | |
| View Full Test Results (4 Failed · 71 Passed · 1 Skipped) |
Event Timeline
Comment Actions
The ArcanistBundleTestCase failures are due to diff binary not being present on Windows systems. The tests could be rewritten to utilize git diff instead but would take some effort.
| src/parser/xhpast/bin/PhutilXHPASTBinary.php | ||
|---|---|---|
| 29–32 | I wonder if the caller should avoid to call this, if not windows. And here, maybe exception instead. But that is just my curiosity on a sugar thing. | |
| src/parser/xhpast/bin/PhutilXHPASTBinary.php | ||
|---|---|---|
| 29–32 | (Sorry I get it now. Fine.) | |
Comment Actions
Maybe this is wild, but: should we consider removing the compiled xhpast.exe file from the repository, and host is externally?
I think it's only used for working on PHP on Windows; The equivalent xhpast isn't included, presumably because compiling stuff on Windows is harder.
My argument for this is that the normal way to install arcanist is git clone, which downloads all the history (and 95% of users won't need this particular file).
History is here: https://secure.phabricator.com/source/libphutil/history/master/src/parser/xhpast/bin/xhpast.exe
Also - is the new file statically linked? https://secure.phabricator.com/T5372 did that.
Comment Actions
I think with this change it's reasonable to remove it. I do believe it was included because compiling it for windows is involved/difficult. This makes it slightly easier.
Also - is the new file statically linked? https://secure.phabricator.com/T5372 did that.
I believe so, the last compilation includes -static, though the two libraries are not but I think that's expected/okay.