T15011: Update support for XHPast on Windows
AcceptedPublic
Actions

Authored by speck on Feb 6 2024, 03:06.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15011: Build a Phorge Developer Environment

Summary

This adds some functional support for building XHPast on Windows.

Refs T15011

Test Plan

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

Repository

rARC Arcanist

Branch

xhpastwin

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 1056
Build 1056: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	13 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('')
	13 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('')
	12 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('')
	2 ms	ArcanistBaseCommitParserTestCase::testBasics 2 assertion(s) passed.
		View Full Test Results (4 Failed · 71 Passed · 1 Skipped)

Event Timeline

speck created this revision.Feb 6 2024, 03:06

Owners added a reviewer: O1: Blessed Committers.Feb 6 2024, 03:06

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptFeb 6 2024, 03:06

speck requested review of this revision.Feb 6 2024, 03:06

Harbormaster completed remote builds in B1054: Diff 1685.Feb 6 2024, 03:06

Fix use of ExecFuture to pass a scalar string as first argument instead of variable.

Harbormaster completed remote builds in B1055: Diff 1686.Feb 6 2024, 03:10

Fix some lints about line length

Harbormaster completed remote builds in B1056: Diff 1687.Feb 6 2024, 03:13

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.

speck edited the summary of this revision. (Show Details)Feb 6 2024, 03:41

speck added a task: T15011: Build a Phorge Developer Environment.

Passed a week. Nobody commented. I blindly trust you in this field :3 Approve.

This revision is now accepted and ready to land.Feb 15 2024, 23:46

valerio.bozzolan added inline comments.Feb 15 2024, 23:47

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.

valerio.bozzolan added inline comments.Feb 15 2024, 23:48

src/parser/xhpast/bin/PhutilXHPASTBinary.php
29–32	(Sorry I get it now. Fine.)

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.

speck mentioned this in T15937: Exception when viewing diff while not logged in.Sep 4 2024, 02:03

In D25535#15480, @avivey wrote:

Maybe this is wild, but: should we consider removing the compiled xhpast.exe file from the repository, and host is externally?

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.