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

Support identifying the HHVM version when not running with HHVM #7913

Merged
merged 4 commits into from Feb 8, 2019

Conversation

Projects
None yet
8 participants
@fredemmott
Copy link
Contributor

fredemmott commented Jan 22, 2019

hhvm-nightly (and the next release) are no longer able to execute
Composer. Support executing Composer with PHP to install dependencies
for hack projects.

The goal is for this to be temporary, until Hack identifies a new
package manager, given that Composer does not aim to be a multi-language
package manager.

fixes #7734

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 22, 2019

I've tested this both by running the unit tests, and with hhvm path/to/composer install and php path/to/composer install for the hack standard library with HHVM 3.30.2 (can execute composer) and HHVM 2019.01.22 (can't execute composer).

Executing with either HHVM or PHP works fine with 3.30.2. Executing with HHVM fails on 2019.01.22, but works fine with php with that version of HHVM available.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 22, 2019

For the curious, HHVM is no longer able to run composer because if we have function foo(&$byref), it must be called as foo(&$myvariable), not as foo($myvariable).

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 22, 2019

So, the failure now is that part of PHPUnit is incompatible with PHP 5.3.

Is it fine to just skip this test on PHP 5.3? It's been unsupported since 2014.

@azjezz

azjezz approved these changes Jan 22, 2019

@usox

usox approved these changes Jan 23, 2019

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jan 26, 2019

Instead of making such hack, which would report your platform as being HHVM as soon as HHVM is installed on the system (even if your project is meant to run on PHP).

Isn't it already possible to use the platform config to make composer considers that the platform is HHVM ?

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 26, 2019

That’s basically asking each user to explicitly ignore platform requirements, which seems worse - though if this change means that “I need PHP7.3” will fail because HHVM is on the system, that needs fixing.

From my read through of the source and the tests, this shouldnprimarily affect explicit dependencies on HHVM - the “what runtime is composer running in?” stuff seems to be explicitly keyed on the HHVM_VERSION constant in each place, not depending on the platform data.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Jan 28, 2019

@fredemmott I tweaked the PR so it's mergeable, but I do still find it kinda sub-par that every user will have to start a process which will fail in 99.9% of cases, simply to support this.

Do you have a timeline on a new solution? I'm curious to know when we can get rid of this.. hack 🙄

The alternative would be that you configure in composer.json that hhvm is present, using the platform configuration, so you don't need to ignore platform reqs, but Composer wouldn't be able to verify the version of hack installed is correct though, so there is some upkeep needed on the HHVM users' side.

That'd go something like:

{
  "config": {
    "platform": {
      "hhvm": "2019.01.22"
    }
  }
}

This makes composer believe HHVM is present in the given version. Or alternatively you can "provide": {"hhvm": "*"} I suppose, which wouldn't need updating every time you change your version.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 28, 2019

Is there a per-user or per-system composer.json that can set defaults for every project?

As for the sub process - perhaps it could be worth using executable finder here, just so that we don’t fork+exec if it isn’t present?

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 28, 2019

As for timeline: we can probably start moving stuff to npm in the next few months (it seems whatever we use, it’ll be backed by the npm repository).

Once we start moving, it should be fairly quick for new users to not need composer, though I’ll ask some of our existing users to respond here about migration.

As for HHVM release cycle: most of our releases become unsupported rather quickly, so probably aren’t a factor here. Neither current LTS needs this diff, assuming the previous handling stays in place. The longest lasting one is 3.30, which is currently due to end support in November, but we might be extending that by 6 months.

FYI both releases can be specifically tested on Travis now, with “hhvm-3.30” and “hhvm-3.27” targets. 4.0 will be the first that needs this

@azjezz

This comment has been minimized.

Copy link

azjezz commented Jan 28, 2019

@fredemmott i can move @nuxed to npm before tagging any release, but what about recursive dependencies ? i don't think installing hhvm/hhvm-autoload for every dependency would be normal by example.
also i need @hhvm and @facebook packages in npm first 😛

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Jan 28, 2019

Is there a per-user or per-system composer.json that can set defaults for every project?

composer config --global platform.hhvm 1.2.3 should apply system wide I believe.

Using PATH resolution via ExecutableFinder would probably be better than a process yes.. I doubt open_basedir matters a lot in this context. If you have a second to add that, and rebase this branch on 1.8 instead of master, I can get it in the next patch release (feel free to squash my commits away).

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Jan 28, 2019

recursive dependencies

Flat installation of all recursive dependencies, pinned at a single version. This is supported (optionally) by yarn, and (mandatory) by esy. This is required as the typechecker requires globally unique names.

@hhvm and @facebook

Yep.

If you have a second to add that, and rebase this branch on 1.8 instead of master, I can get it in the next patch release (feel free to squash my commits away).

Will do, thanks.

Support identifying the HHVM version when not running with HHVM
hhvm-nightly (and the next release) are no longer able to execute
Composer. Support executing Composer with PHP to install dependencies
for hack projects.

The goal is for this to be temporary, until Hack identifies a new
package manager, given that Composer does not aim to be a multi-language
package manager.

fixes #7734

@fredemmott fredemmott force-pushed the fredemmott:hhvm-version branch from 6c1609d to 1b19672 Jan 29, 2019

@fredemmott fredemmott changed the base branch from master to 1.8 Jan 29, 2019

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 1, 2019

@Seldaek sorry the poke - do you need anythign else from me? FYI we're planning on releasing the next HHVM version on the 11th, which won't be able to execute composer

fredemmott added some commits Feb 6, 2019

Better error message for present but incompatible versions
hhvm-nightly (and next week's release) now report 4.x, so all the 3.x
constraints are now giving misleading error messages with this patch.

Before:

```
    - facebook/fbexpect v2.3.0 requires hhvm ^3.28 -> you are running this with PHP and not HHVM.
```

After:

```
    - facebook/fbexpect v2.3.0 requires hhvm ^3.28 -> your HHVM version (4.0.0-dev) does not satisfy that requirement.
```

@Seldaek Seldaek merged commit f2cc666 into composer:1.8 Feb 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 8, 2019

Looks good thanks

@Seldaek Seldaek added this to the 1.8 milestone Feb 8, 2019

@fredemmott fredemmott deleted the fredemmott:hhvm-version branch Feb 8, 2019

@ste93cry

This comment has been minimized.

Copy link

ste93cry commented Feb 11, 2019

Actually this change is breaking on systems with both PHP and HHVM installed. For example in one of my projects I'm requiring PHPCS-Fixer which has a conflict with every version of HHVM. The installation fails with the following error:

Your requirements could not be resolved to an installable set of packages.

Problem 1
- friendsofphp/php-cs-fixer v2.13.0 conflicts with hhvm[3.29.1].
- friendsofphp/php-cs-fixer v2.13.1 conflicts with hhvm[3.29.1].
- friendsofphp/php-cs-fixer v2.13.2 conflicts with hhvm[3.29.1].
- friendsofphp/php-cs-fixer v2.13.3 conflicts with hhvm[3.29.1].
- friendsofphp/php-cs-fixer v2.14.0 conflicts with hhvm[3.29.1].
- friendsofphp/php-cs-fixer v2.14.1 conflicts with hhvm[3.29.1].
- Installation request for hhvm 3.29.1 -> satisfiable by hhvm[3.29.1].
- Installation request for friendsofphp/php-cs-fixer ^2.13 -> satisfiable by friendsofphp/php-cs-fixer[v2.13.0, v2.13.1, v2.13.2, v2.13.3, v2.14.0, v2.14.1].

If I remove HHVM the composer installation ends successfully

@schmittjoh

This comment has been minimized.

Copy link
Contributor

schmittjoh commented Feb 11, 2019

Yes, we also have a couple support tickets related to php-cs-fixer. I think this will cause problems for CI systems in general which have a lot of different versions installed, but only a subset of those is actually used.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 11, 2019

Oh nice.. conflicting against HHVM, that's a lovely idea. /cc @keradus can we get a new release of php-cs-fixer without the conflict perhaps? I don't see what the point of doing that is really. Now that HHVM won't run PHP code at all anymore, there is really no reason.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 11, 2019

Now that HHVM won't run PHP code at all anymore, there is really no reason.

We've broke enough (especially around references) to no longer be able to run composer. We're currently evaluating if simply banning <?php and friends in the parser could be better or worse for users (causing more problems vs "it probably doesn't work anyway and gives a clear message").

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented Feb 11, 2019

Oh nice.. conflicting against HHVM, that's a lovely idea. /cc @keradus can we get a new release of php-cs-fixer without the conflict perhaps? I don't see what the point of doing that is really.

Sorry @Seldaek , but I'm not fully aware about the context here.
PHP CS Fixer can not be run under HHVM (3.x or 2019.x), as PHP CS Fixer heavily rely on ext-tokenizer, which is simply not compatible under HHVM (multiple issues raised to HHVM, after that HHVM decided to not keep compatibility of tokenizer).
For that, in PHP CS Fixer, we got plenty of users complaining "hi, I can install the tool under my HHVM, but the result is not good". For that, we forbid to install the PHP CS Fixer using HHVM.

I don't understand why, with this PR, we consider the Composer to be executed via HHVM even if it's only present on the machine, but not being used to call the Composer

Now that HHVM won't run PHP code at all anymore, there is really no reason.

Ppl still can install old HHVM that would run PHP code, still.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 11, 2019

A potential compromise would be change the conflict to HHVM <4. This would:

  • provide a correct experience for those with HHVM 4 installed: HHVM4 can not be used to invoke composer
  • provide a correct experience for those invoking composer with HHVM 3
  • provide an incorrect experience for those invoking composer with PHP but with HHVM 3 installed
@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 11, 2019

provide an incorrect experience for those invoking composer with PHP but with HHVM 3 installed

This could be resolved by making Composer ignore system HHVM versions unless they're 4.0 or above - i.e. for HHVM 3, you must execute Composer with HHVM, which was previously true anyway.

@ste93cry

This comment has been minimized.

Copy link

ste93cry commented Feb 12, 2019

I don't understand why, with this PR, we consider the Composer to be executed via HHVM even if it's only present on the machine, but not being used to call the Composer

I agree, I don't have the full context to understand why the changes in this PR have been made but I don't understand at all why if I have both PHP and HHVM running on my machine and I'm executing Composer with PHP it should prevent packages from being installed if not compatible with HHVM.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 12, 2019

The point of this was supporting the new HHVM releases which can not execute Composer themselves. As such you have to use Composer with PHP, even though you are targetting HHVM..

@fredemmott how bad would be #7913 (comment) as workaround? Because it seems like the current solution is causing pain for random users of HHVM (or at least people that have it installed).. unlike the platform config workaround which could be documented in your release/upgrade notes as a requirement to upgrade, and then shouldn't cause issues as it won't be applied where not needed.

So I am wondering if we should revert this altogether.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 12, 2019

If the main change here is reverted, it would be good to at least keep the error message changes: "you are running this with PHP and not HHVM" is misleading about the problem with 4.0

That said, I don't think it's a good approach:

  1. It doesn't really fix the php-cs-fixer problem:
$ cd ~/code/composer
$ git reset --hard 1.8.3
$ cd $(mktemp -d)
$ php ~/code/composer/bin/composer config --global platform.hhvm 4.0.0
$ php ~/code/composer/bin/composer require friendsofphp/php-cs-fixer
Using version ^2.14 for friendsofphp/php-cs-fixer
./composer.json has been created
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - friendsofphp/php-cs-fixer v2.14.0 conflicts with hhvm[4.0.0].
    - friendsofphp/php-cs-fixer v2.14.1 conflicts with hhvm[4.0.0].
    - Installation request for hhvm 4.0.0 -> satisfiable by hhvm[4.0.0].
    - Installation request for friendsofphp/php-cs-fixer ^2.14 -> satisfiable by friendsofphp/php-cs-fixer[v2.14.0, v2.14.1].


Installation failed, deleting ./composer.json.
  1. It requires every user to run a command every time they switch HHVM versions (which is more frequent than every update) - and , given the above, every time they want to work on a PHP project instead of Hack. A lot of users aren't going to do this consistently and correctly: we're going to end up with a bunch of people with composer configured at hhvm=4.0.0 forever. This feels like it's going to end up being equivalent to --ignore-platform-reqs but with extra steps and a false sense of safety

  2. 4.0's out. I can edit a change into the release announcement, but given that it would be after the release, I don't think many people are going to see it. You're going to see

I think both 2 and 3 would lead to 'bug reports' and support requests for both HHVM and Composer.

A few other options:

  1. ask projects such as php-cs-fixer to conflict with HHVM <= 3, and change composer to ignore the system HHVM version if it's < 4
  2. we tell HHVM users to use 1.8.4 - and never upgrade - until we're not using Composer any more. This change gets backed out, and possibly remove all support for the HHVM platform from composer itself.
  3. Facebook forks composer to a hack-composer temporarily, until we move. Conflicts like this are resolved by 'use hack-composer for Hack, composer for PHP'. This change gets backed out, and possibly remove all support for the HHVM platform from composer itself. I'm not sure how quickly we could do this (there's some process to creating a new Facebook fork/project).

I don't have a real preference between any of those 3.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented Feb 12, 2019

  1. ask projects such as php-cs-fixer to conflict with HHVM <= 3, and change composer to ignore the system HHVM version if it's < 4

can you give some insights on this, please? like, PHP CS Fixer can not run with any version of HHVM, why it shall not conflict with >4 ?

@azjezz

This comment has been minimized.

Copy link

azjezz commented Feb 12, 2019

@keradus hhvm > 4 doesn't support php, so it shouldn't be able to execute php-cs-fixer anyways.

@schmittjoh

This comment has been minimized.

Copy link
Contributor

schmittjoh commented Feb 12, 2019

@keradus, instead of relying on composer to enforce the non-HHVM constraint, could you maybe just add something at the CLI entry point of php-cs-fixer that checks if HHVM is used for running it?

I think this would be more failure proof as you theoretically might use php for installing php-cs-fixer, but then you could still use hhvm for running it. This would not be catched by your composer constraint.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 12, 2019

HHVM 3.30 is a runtime for PHP and Hack. HHVM 4+ is a runtime for Hack only, and afaik should be irrelevant to any PHP projects.

HHVM3 is also able to execute compoesr directly, so using the HHVM_VERSION_ID constants instead of shelling out works there.

@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 12, 2019

@keradus actually, can you resolve this by explicitly depending on the tokenizer extension?

$ cat test.php
<?php

$re = new ReflectionExtension('tokenizer');
var_dump($re->getVersion());
$ php --version
PHP 7.1.19 (cli) (built: Aug 17 2018 20:10:18) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
$ php test.php
string(6) "7.1.19"
$ /usr/local/Cellar/hhvm@3.27-lts/3.27.6/bin/hhvm test.php # oldest supported version
string(3) "0.1"
$ /usr/local/Cellar/hhvm@3.30-lts/3.30.2/bin/hhvm test.php # newest version that supports PHP
string(3) "0.1"
$ /usr/local/Cellar/hhvm/4.0.0/bin/hhvm test.php # HHVM 4 does not have the tokenizer extension
string(0) ""
@fredemmott

This comment has been minimized.

Copy link
Contributor Author

fredemmott commented Feb 12, 2019

Hmm, php 5.3 reports '0.1' - a combination of depending on ext-tokenizer and conflicting with hhvm < 4 gives you dependencies that can not be resolved in any HHVM environment, even if HHVM started being able to execute composer again - but does work with the current patch if using PHP on a system where HHVM is also installed.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented Feb 12, 2019

@schmittjoh that's one of the option indeed.

@fredemmott , PHP CS Fixer already does depends on tokenizer. Issue of HHVM was not lack of it, but not compatible with PHP implementation of it.

TBH, I'm not sure if this is the issue of PHP CS Fixer itself, or the implementation of requested change on Composer level. like, PHP CS Fixer may configure a conflict with HHVM, but how you ensure it is the only package which does that ?
I have the feeling that changing PHP CS Fixer for this would be fighting with the symptoms and not the root cause.
Having said that, for sure I'm not fully aware about each aspect of the issue and I spent way less time on it that you guys here.
@Seldaek , when you would have conclusion which path you would like to take, please ping me ! :)

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 12, 2019

@keradus I think if you could remove that conflict and instead implement a runtime check like @schmittjoh suggested it would be more reliable and also remove some pressure on us to fix this, because it seems to be the only package causing issues at the moment. If that works for you that'd be great.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented Feb 12, 2019

as you wish, @Seldaek : FriendsOfPHP/PHP-CS-Fixer#4306
I prepared it for very next release of our LTS version (any any newer version we would release, of course).
We are dropping Composer conflict, runtime check is already there.

Please, be warned that I'm out of my homeland now, I will try to release it during the weekend.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 12, 2019

Thanks for the prompt reaction!

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