Skip to content
This repository has been archived by the owner on Nov 14, 2019. It is now read-only.

Don't use HHVM to run box as it gets rejected #184

Closed
wants to merge 1 commit into from
Closed

Conversation

stof
Copy link
Member

@stof stof commented Sep 10, 2015

This is an attempt at making the testsuite run on HHVM. We don't really care about running Box on each version, as we are not testing Box itself (the version being downloaded will not have been built with the same PHP version than the one you use when running the installer anyway)

@javiereguiluz
Copy link
Member

This solution worked great! Thanks.

However, HHVM seems to have some issues getting the output of the executed commands:

1) Symfony\Installer\Tests\IntegrationTest::testDemoApplicationInstallation
Failed asserting that Binary String: 
0x9553c16eda4010bdfb2ba6511a1b09db2025524529110a48e1922048e9a154
d6d63b866ded5d77770d4155ffbd6363882152955a3e783cefbd99d937dbbfcdd7
b9e38804bce9fd345a8c67f3c9e3433419411f6e3ad79d4e0b7e3b404f22528cf2
...
contains "Downloading the Symfony Demo Application".

@stof
Copy link
Member Author

stof commented Sep 10, 2015

actually, it seems to have some issues with the phar. Here is what the binary string contains when decoding it: https://3v4l.org/4pBfp

@stof
Copy link
Member Author

stof commented Sep 10, 2015

I found the issue. It is here:

"compression": "GZ",

HHVM does not support compressed phars: facebook/hhvm#4263
this is the reason why Composer does not use compression for its phar for instance.

@stof
Copy link
Member Author

stof commented Sep 10, 2015

and this means we should either stop using compression or stop testing on HHVM and marking it as unsupported

@javiereguiluz
Copy link
Member

To be completely honest, I always remove the compression configuration in my computer because otherwise I get this error:

PHP Fatal error:  Uncaught exception 'ErrorException' with message 'proc_open(): unable to create pipe Too many open files' in phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php:974
Stack trace:
#0 [internal function]: KevinGH\Box\Application->KevinGH\Box\{closure}(2, 'proc_open(): un...', 'phar:///usr/loc...', 974, Array)
#1 phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php(974): proc_open('stty -a | grep ...', Array, NULL, NULL, NULL, Array)
#2 phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php(784): Symfony\Component\Console\Application->getSttyColumns()
#3 phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php(745): Symfony\Component\Console\Application->getTerminalDimensions()
#4 phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php(675): Symfony\Component\Console\Application->getTerminalWidth()
#5 phar:/// in phar:///usr/local/bin/box/src/vendors/symfony/console/Symfony/Component/Console/Application.php on line 974

@javiereguiluz
Copy link
Member

Just for the record, this is the size of each version:

  • Compressed: 193,370 bytes
  • Uncompressed: 618,588 bytes

My vote is for keeping it compressed and dropping support for HHVM (which we never claimed in the first place). But I'd like to read other opinions.

@Pierstoval
Copy link
Contributor

Actually this is just an installer, it does not matter to me that it's 200KB or 600KB, it's a single file used once in an environment.
If it was a dependency for many packages, I would change my mind, but this is not the case.

@weaverryan
Copy link
Member

@javiereguiluz does this mean the installer wouldn't work for people on hhvm machines? Or just that the installer phar couldn't be created on an hhvm machine?

@javiereguiluz
Copy link
Member

@weaverryan the installer won't work on HHVM machines. To be honest, I don't consider this a problem at all:

  • Symfony itself started working on HHVM only very recently (in july 2015)
  • I can't conceive a PHP developer who has installed HHVM on his development machine but not PHP
  • HHVM is mostly useful on production servers (on dev machines you probably only install HHVM to test if it works)
  • [Personal bet] PHP 7 is about to be released ... and it could make HHVM useless.
  • [Personal opinion] I know a lot of PHP developers, but I know no one who uses HHVM daily on his dev machine to develop apps (I'm not talking about the production server).

@weaverryan
Copy link
Member

My gut says "support it, the file size isn't a big deal". But, if we don't support hhvm and anyone uses it, we'll get bug reports and can change. I'm fine either way.

@xabbuh
Copy link
Member

xabbuh commented Sep 22, 2015

I am fine with either way. Apparently, not many people tried to use it on HHVM right now or we would have received some complaints I bet.

@javiereguiluz
Copy link
Member

Closing it for now, because "worsening" the current PHP installer to support HHVM looks wrong to me. Let's wait for the launch of PHP 7 and we'll revisit this decision then. Thanks.

@javiereguiluz javiereguiluz deleted the stof-patch-1 branch October 18, 2015 14:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants