Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore HHVM support #176

Closed
thekid opened this issue Jun 4, 2017 · 21 comments
Closed

Restore HHVM support #176

thekid opened this issue Jun 4, 2017 · 21 comments

Comments

@thekid
Copy link
Member

thekid commented Jun 4, 2017

$ XP_RT=hhvm xp -v

Fatal error: Uncaught exception 'Exception' with message 'This version of the XP Framework 
requires PHP 7.0.0+, have PHP 5.6.99-hhvm'
@thekid
Copy link
Member Author

thekid commented Jun 4, 2017

... the upcoming 3.11.0 stable release will be the first release of HHVM with support for the major PHP 7 features.
(source: http://hhvm.com/blog/10859/php-7-support)

This means the checks should be on:

  • PHP_VERSION_ID >= 70000
  • defined('HHVM_VERSION_ID') && HHVM_VERSION_ID >= 31100

Also, the config setting hhvm.php7.all needs to be set to 1


By the way, installing HHVM on Bash on Windows works exactly as documented for Ubuntu 😀

@thekid
Copy link
Member Author

thekid commented Jun 4, 2017

Let's see how this does on Travis-CI...

@thekid
Copy link
Member Author

thekid commented Jun 4, 2017

OK, here are some of the issues:

Throwable

namespace lang; class ... extends Throwable raises: Fatal error: Uncaught Error: Class lang\T may not inherit from interface (__SystemLib\Throwable). HHVM seems to prefer its builtin classes over the Throwable class declared in the lang namespace. Fixed by fully qualifying parent class names

HHVM doesn't catch exceptions when using catch (\Throwable $t) - unlike PHP, the Throwable type is not inside global namespace - can be fixed by adding catch (\__SystemLib\Throwable $t)

Strict mode

HHVM uses strict mode by default, causing warnings like preg_match_all() expects parameter 2 to be string, null given - needs a couple of safeguards or casts

Reflection access to fields is broken in HHVM (due to the strict mode):

$ XP_RT=hhvm xp -w '
  class A { public $f; }
  $r= new ReflectionProperty("A", "f");
  return $r->getValue(new A())
'
Fatal error: Uncaught TypeError: Argument 2 passed to hphp_get_property()
must be an instance of string, null given

See https://docs.hhvm.com/hack/reference/function/hphp_get_property/

@thekid
Copy link
Member Author

thekid commented Jun 4, 2017

HHVM branched off to feature/hhvm-7 branch, too much of a hassle right now.

@thekid thekid changed the title HHVM HHVM support Jun 4, 2017
@fredemmott
Copy link

  • PHP version ID: as you found, needs the ini flag setting
  • Throwable: should be fixed in 3.20 - if not, please file an issue against HHVM and mention me in the comment: facebook/hhvm@29fc86f
  • Strict mode: should also be fixed in 3.20 - if not, file an issue, but I'm not the right person for it: facebook/hhvm@f02a287

Sadly, travis is a pain to use with HHVM: HHVM's only supported by Travis on Ubuntu 14.04, and recent versions of HHVM require 16.04 or newer. You can use docker, but this would likely require a change to your PHP testing too, so might not be worth it.

Docker travis example:

https://github.com/facebook/xhp-lib/blob/master/.travis.yml
https://github.com/facebook/xhp-lib/blob/master/.travis.sh

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Strict mode: should also be fixed in 3.20 - if not, file an issue

There actually already is one for it: facebook/hhvm#7709

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Throwable: should be fixed in 3.20

It is, thanks @fredemmott!

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

PHP version ID: as you found, needs the ini flag setting

Nice, HHVM 3.20 now also returns a PHP7-ish version number (if hhvm.php7.all = 1) instead of 5.6.99

$ hhvm --version
HipHop VM 3.20.2 (rel)
Compiler: tags/HHVM-3.20.2-0-gad133f299ece0e45db8536473a8ef77cc4612ae3
Repo schema: 962ff68f0831a12b292f0eb9acd17f705a904446

$ XP_RT=hhvm xp -w '[PHP_VERSION_ID, phpversion()]'
[70199, "7.1.99-hhvm"]

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Giving it a try:

$ XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
terminate called without an active exception
Core dumped: Aborted
Stack trace in /tmp/stacktrace.1108.log

$ cat /tmp/stacktrace.1108.log
Host: slate
ProcessID: 1108
ThreadID: 140197943919552
ThreadPID: 1108
Name: /usr/bin/xp
Type: Aborted
Runtime: hhvm
Version: tags/HHVM-3.20.2-0-gad133f299ece0e45db8536473a8ef77cc4612ae3
DebuggerCount: 0

Arguments: /usr/bin/class-main.php xp.unittest.TestRunner src/test/config/unittest/core.ini
ThreadType: CLI

# 0  0000000003508796
# 1  0000000001167f92
# 2  00007f825e971390
# 3  00007f8257d95428
# 4  00007f8257d9702a
# 5  00007f82586ef84d
# 6  00007f82586ed6b6
# 7  00007f82586ed701
# 8  00000000011c59e2
# 9  0000000010bc02a4
# 10 000000000a1c829c
# 11 000000000640029e
# 12 0000000002000554
# 13 00000000020f321a
# 14 00000000010caea8
# 15 0000000002cab277
# 16 000000000a07377c
# 17 000000000a0736bd
# 18 000000000a072c2e
# 19 000000000a072485
# 20 000000000a06f720
# 21 000000000e0000c0
# 22 000000000e0000c0
# 23 000000000e0000c0
# 24 000000000640029e
# 25 0000000002000554
# 26 00000000020f321a
# 27 00000000010cb505
# 28 00000000010cbba1
# 29 0000000003510348
# 30 00000000035106c9
# 31 00000000013d575b
# 32 00000000013d5fcb
# 33 00000000013eae07
# 34 00000000013eca37
# 35 00000000011b7938
# 36 0000000000a10f38
# 37 00007f8257d80830
# 38 0000000000a0c0d9

PHP Stacktrace:

#0  lang\Throwable::printStackTrace() called at [/mnt/.../src/test/php/net/xp_framework/unittest/core/ExceptionsTest.class.php:118]
#1  net\xp_framework\unittest\core\ExceptionsTest->printStackTrace()
#2  ReflectionMethod->invokeArgs() called at [/mnt/.../src/main/php/lang/reflect/Method.class.php:88]
#3  lang\reflect\Method->invoke() called at [xar:///mnt/.../test.xar?unittest/TestSuite.class.php:317]
#4  unittest\TestSuite->runInternal() called at [xar:///mnt/.../test.xar?unittest/TestSuite.class.php:529]
#5  unittest\TestSuite->run() called at [xar:///mnt/.../test.xar?xp/unittest/TestRunner.class.php:376]
#6  xp\unittest\TestRunner->run() called at [xar:///mnt/.../test.xar?xp/unittest/TestRunner.class.php:386]
#7  xp\unittest\TestRunner::main() called at [/usr/bin/class-main.php:374]

Can be reproduced with this simpler case:

$ XP_RT=hhvm xp -e '
  use io\streams\{Streams, MemoryOutputStream};
  $out= new MemoryOutputStream();
  fwrite(Streams::writeableFd($out), "Test")
'
terminate called without an active exception
Core dumped: Aborted
Stack trace in /tmp/stacktrace.1173.log

...and can be worked around as follows:

$ git diff
diff --git a/src/main/php/io/streams/Streams.class.php b/src/main/php/io/streams/Streams.class.php
index 18eb641..89524ee 100755
--- a/src/main/php/io/streams/Streams.class.php
+++ b/src/main/php/io/streams/Streams.class.php
@@ -61,7 +61,7 @@ abstract class Streams {
       }

       public function stream_flush() {
-        return self::$streams[$this->id]->flush();
+        return parent::$streams[$this->id]->flush();
       }

       public function stream_eof() {

With that patch applied, here's how we fare:

XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2122/2196 run (74 skipped), 2053 succeeded, 69 failed
Memory used: 6144.00 kB (6144.00 kB peak)
Time taken: 15.074 seconds

Additionally, by setting hhvm.hack.lang.look_for_typechecker = 0 in hhvm.ini, I can prevent could not find a .hhconfig file errors, and get this:

XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2122/2196 run (74 skipped), 2069 succeeded, 53 failed
Memory used: 6144.00 kB (6144.00 kB peak)
Time taken: 11.620 seconds

After opening facebook/hhvm#7878 (anonymous classes and self:: reference) and working around it (see eeb3fa5), we're now here:

XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2121/2196 run (75 skipped), 2113 succeeded, 8 failed
Memory used: 4096.00 kB (4096.00 kB peak)
Time taken: 51.356 seconds

Reasons these 8 tests are broken:

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

The test net.xp_framework.unittest.io.streams.Bz2CompressingOutputStreamTest fails because the stream wrapper doesn't honor the blocks setting:

stream_filter_append($this->out, 'bzip2.compress', STREAM_FILTER_WRITE, ['blocks' => $level])
//                                                                      ^^^^^^^^^^^^^^^^^^^^
//                                                                      This

Worked around by ignoring; opened facebook/hhvm#7879

thekid added a commit that referenced this issue Jun 10, 2017
@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Added a workaround for the hphp_(get|set)_property problem by calling them directly. In essence, this is what I'm doing:

if (defined('HHVM_VERSION_ID')) {
  self::$read= function($class, $reflect, $instance, $public) {
    if (null === $instance) {
      return hphp_get_static_property($class, $reflect->name, !$public);
    } else {
      return hphp_get_property($instance, $class, $reflect->name);
    }
  };
  self::$write= function($class, $reflect, $instance, $value, $public) {
    if (null === $instance) {
      return hphp_set_static_property($class, $reflect->name, $value, !$public);
    } else {
      return hphp_set_property($instance, $class, $reflect->name, $value);
    }
  };
} else {
  self::$read= function($class, $reflect, $instance, $public) {
    $public || $reflect->setAccessible(true);
    return $reflect->getValue($instance);
  };
  self::$write= function($class, $reflect, $instance, $value, $public) {
    $public || $reflect->setAccessible(true);
    return $reflect->setValue($instance, $value);
  };
}

Adding a small overhead with the closure function call, though.

Documentation for these functions is available in ext/reflection/ext_reflection-internals-functions.php


This gets us to:

$ XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2121/2196 run (75 skipped), 2116 succeeded, 5 failed
Memory used: 4096.00 kB (4096.00 kB peak)
Time taken: 49.872 seconds

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Added @action(new IgnoredOnHHVM()) to the tests for void return types and variadics with type as long as they're not supported (see facebook/hhvm#7740 and facebook/hhvm#6954) in 9868a57


This gets us to:

$ XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2117/2196 run (79 skipped), 2115 succeeded, 2 failed
Memory used: 4096.00 kB (4096.00 kB peak)
Time taken: 49.337 seconds

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

"Cherry-picked" c5a7000 to fix variadic parameters always returning array on HHVM in 569b3a1. This gets us to:

$ XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
×: 2117/2196 run (79 skipped), 2116 succeeded, 1 failed
Memory used: 4096.00 kB (4096.00 kB peak)
Time taken: 50.479 seconds

The only test failing now is:

F unittest.TestAssertionFailed(test= ErrorsTest::call_to_member_on_non_..., time= 0.046 seconds) {
  unittest.AssertionFailedError{ Caught Exception
    lang.XPException (Call to a member function method() on a non-object (null)) 
    instead of expected lang.Error
  }
    at unittest.TestSuite::run() [line 376 of TestRunner.class.php]
    at xp.unittest.TestRunner::run() [line 386 of TestRunner.class.php]
    at xp.unittest.TestRunner::main() [line 374 of class-main.php]

 }

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

This resulted from PHP throwing an Error (which gets wrapped into a lang.Error), while HHVM throws as BadMethodCallException (which gets wrapped into a lang.XPException). Addressed this in lang.Throwable::wrap(), which now does:

if ($e instanceof self) {
  return $e;
} else if ($e instanceof \BadMethodCallException) {                   // This condition is new:
  $wrapped= new Error($e->getMessage(), $e->getPrevious(), false);    // Wrap in lang.Error
} else if ($e instanceof \Exception) {
  $wrapped= new XPException($e->getMessage(), $e->getPrevious(), false);
} else if ($e instanceof \Throwable) {
  $wrapped= new Error($e->getMessage(), $e->getPrevious(), false);
} else {
  throw new IllegalArgumentException('Given argument ...');
}

And now:

$ XP_RT=hhvm xp -cp test.xar xp.unittest.TestRunner src/test/config/unittest/core.ini
# ...
♥: 2117/2196 run (79 skipped), 2117 succeeded, 0 failed
Memory used: 4096.00 kB (4096.00 kB peak)
Time taken: 50.533 seconds

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Minimum HHVM version required is HHVM 3.20


Example for older HHVM versions:

$ XP_RT=hhvm xp -v

Fatal error: Uncaught exception 'Exception' with message
'This version of the XP Framework requires HHVM 3.20+, have HHVM 3.18.0'

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

HHVM branched off to feature/hhvm-7 branch, too much of a hassle right now.

This branch has now been removed again, HHVM support is back; unlike other frameworks, the XP Framework will continue to support HHVM for the time being.

I agree with the authors of HHVM and MongoDB and Symfony 4: End of HHVM support that HHVM is not used the majority of users and that PHP7 has caught up performance-wise in the meantime. However, the HHVM team seems to have been putting quite a bit of effort in to reducing the PHP 7 incompatibilities lately; and one of the XP Framework's goals has been to operate on as many platforms and environments as possible.

See also:

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Here are some problems from running the entire test suite:

F unittest.TestAssertionFailed(test= net.xp_framework.unittest.io.PathTest::normalize_uppercases_drive_letter, time= 0.072 seconds) {
  unittest.AssertionFailedError{ expected ["C:/test"] but was ["c:/test"] using: 'equals' }
    at unittest.TestCase::assertEquals() [line 314 of PathTest.class.php]
    at net.xp_framework.unittest.io.PathTest::normalize_uppercases_drive_letter() [line 0 of StackTraceElement.class.php]
    at ReflectionMethod::invokeArgs() [line 88 of Method.class.php]
    at lang.reflect.Method::invoke() [line 317 of TestSuite.class.php]
    at unittest.TestSuite::runInternal() [line 529 of TestSuite.class.php]
    at unittest.TestSuite::run() [line 376 of TestRunner.class.php]
    at xp.unittest.TestRunner::run() [line 386 of TestRunner.class.php]
    at xp.unittest.TestRunner::main() [line 374 of class-main.php]

 }
F unittest.TestAssertionFailed(test= net.xp_framework.unittest.io.PathTest::relative_to("c:/Windows", "C:/", "Windows"), time= 0.002 seconds) {
  unittest.AssertionFailedError{ expected ["Windows"] but was ["../../c:/Windows"] using: 'equals' }
    at unittest.TestCase::assertEquals() [line 353 of PathTest.class.php]
    at net.xp_framework.unittest.io.PathTest::relative_to() [line 0 of StackTraceElement.class.php]
    at ReflectionMethod::invokeArgs() [line 88 of Method.class.php]
    at lang.reflect.Method::invoke() [line 317 of TestSuite.class.php]
    at unittest.TestSuite::runInternal() [line 529 of TestSuite.class.php]
    at unittest.TestSuite::run() [line 376 of TestRunner.class.php]
    at xp.unittest.TestRunner::run() [line 386 of TestRunner.class.php]
    at xp.unittest.TestRunner::main() [line 374 of class-main.php]

 }
F unittest.TestAssertionFailed(test= net.xp_framework.unittest.io.PathTest::relative_to("C:\Windows", "c:", "Windows"), time= 0.000 seconds) {
  unittest.AssertionFailedError{ expected ["Windows"] but was ["../C:\Windows"] using: 'equals' }
    at unittest.TestCase::assertEquals() [line 353 of PathTest.class.php]
    at net.xp_framework.unittest.io.PathTest::relative_to() [line 0 of StackTraceElement.class.php]
    at ReflectionMethod::invokeArgs() [line 88 of Method.class.php]
    at lang.reflect.Method::invoke() [line 317 of TestSuite.class.php]
    at unittest.TestSuite::runInternal() [line 529 of TestSuite.class.php]
    at unittest.TestSuite::run() [line 376 of TestRunner.class.php]
    at xp.unittest.TestRunner::run() [line 386 of TestRunner.class.php]
    at xp.unittest.TestRunner::main() [line 374 of class-main.php]

 }

These are caused by facebook/hhvm#7880, but can (and have been) easily be worked around:

-    if (2 === sscanf($this->path, '%c%*[:]', $drive)) {
+    if (strlen($this->path) > 1 && ':' === $this->path{1}) {

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

OK, Travis-CI gives us:

$ hhvm --version
HipHop VM 3.18.3 (rel)
Compiler: tags/HHVM-3.18.3-0-g1ddd4af7b1342c20635afbdc67701fbcbdf97bfa
Repo schema: 61b13efcfd4e2fabea72495f96208746a568a688

See https://travis-ci.org/xp-framework/core/jobs/241581199

This won't work, neither will hhvm-nightly, which is even older(!)

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

✅ Restored unittests success by running HHVM in Docker:

hhvm-in-docker-on-travis

https://travis-ci.org/xp-framework/core/jobs/241593079

@thekid
Copy link
Member Author

thekid commented Jun 10, 2017

Here's the code that replaces HHVM with a Docker-based version - inspired by @fredemmott's solution above.

wrap() {
  local image="$1"
  local cmd="$2"
  local target="$3"
  local wrapper=$(basename $target).in
  local wd=$(pwd)

  mv $target $wrapper
  echo "#!/bin/sh" > $target
  echo "docker run --rm -v $wd:/opt/src -v $wd/php.ini:/etc/hhvm/php.ini -w /opt/src $image $cmd $wrapper \$@" >> $target
  chmod 755 $target
}

replace_hhvm_with() {
  local version="$1"

  printf "\033[33;1mReplacing HHVM\033[0m\n"
  docker pull hhvm/hhvm:$version
  docker run --rm hhvm/hhvm:$version hhvm --version
  echo

  echo "hhvm.php7.all = 1" > php.ini
  echo "hhvm.hack.lang.look_for_typechecker = 0" >> php.ini
  wrap hhvm/hhvm:$version "hhvm --php" /home/travis/.phpenv/versions/hhvm/bin/composer
  wrap hhvm/hhvm:$version "sh" xp-run
}

# Run HHVM inside Docker as the versions provided by Travis-CI are too old
case "$TRAVIS_PHP_VERSION" in
  hhvm-nightly*)
    replace_hhvm_with "latest"
  ;;

  hhvm*)
    replace_hhvm_with "3.20.1"
  ;;
esac

HHVM Versions can be found @ https://hub.docker.com/r/hhvm/hhvm/tags/. Currently latest equals 3.20.1, so both hhvm and hhvm-nightly will use the same HHVM version. This will change one newer HHVM versions are released by Facebook.

See https://travis-ci.org/xp-framework/core/builds/241608653

@thekid thekid closed this as completed Jun 10, 2017
@thekid thekid changed the title HHVM support Restore HHVM support Jun 10, 2017
@thekid thekid added this to the 9.0.0 milestone Jun 10, 2017
@thekid
Copy link
Member Author

thekid commented Sep 24, 2017

Interesting:

PHP7 is charting a new course away from PHP5, and we want to do the same, via a renewed focus on Hack. Consequently, HHVM will not aim to target PHP7. The HHVM team believes that we have a clear path toward making Hack a fantastic language for web development, untethered from its PHP origins.

http://hhvm.com/blog/2017/09/18/the-future-of-hhvm.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants