Add PHAR constraint check #116

theofidry · 2018-04-11T21:16:17Z

This feature is about adding a check in the PHAR stub in order to gracefully fail with a human friendly error when the user is using the PHAR in a non-supported environment, e.g. invalid PHP version or missing extension. The goal is to gather those requirements without requiring any configuration from the user.

Here's a glimpse of how it looks like:

theofidry · 2018-04-11T21:42:09Z

@MacFJA as promised here's the script. It's still a big WIP, but that's a gist of it.

As there is still much to do and because it's quite fragile (it requires manual autoloading so right it's very coupled to the FS) I don't think it will be easily portable. Tbh I'm more worried about making it work properly and testing it properly (I still have no idea on how to automate that) before considering making it a portable standalone.

MacFJA · 2018-04-14T12:52:27Z

@theofidry Thanks to keep me in touch!

In your checklist, for the entry "Make the PHP check work: right now compare_version(PHP_VERSION?, '^7.2', '>=') will return true on PHP 5.3...", maybe you can the package composer/semver

Here a example on how it can be used:

<?php

require_once __DIR__ . '/src/Semver.php';
require_once __DIR__ . '/src/VersionParser.php';
require_once __DIR__ . '/src/Constraint/ConstraintInterface.php';
require_once __DIR__ . '/src/Constraint/Constraint.php';
require_once __DIR__ . '/src/Constraint/MultiConstraint.php';

use Composer\Semver\Semver;

var_dump(Semver::satisfies('5.3', '^7.2')); // Return false
var_dump(Semver::satisfies('7.2', '^7.2')); // Return true
var_dump(Semver::satisfies('7.3', '^7.2')); // Return true

(As you see, 5 files will need to be added)

theofidry · 2018-04-14T13:35:53Z

Do you have an idea on how, I remember there was a similar thing in PhpScoper but it was not working correctly with the different PHP versions: humbug/php-scoper#154

…

On Sat 14 Apr 2018 at 13:52, MacFJA ***@***.***> wrote: @theofidry <https://github.com/theofidry> Thanks to keep me in touch! In your checklist, for the entry "Make the PHP check work: right now compare_version(PHP_VERSION?, '^7.2', '>=') will return true on PHP 5.3...", maybe you can the package composer/semver — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gY7NGUBNw0GeaGn3CY3wY_jm1Im2ks5tofELgaJpZM4TQw6x> .

MacFJA · 2018-04-14T13:00:42Z

src/RequirementChecker/Checker.php

+
+        $checkPassed = self::evaluateRequirements($requirements);
+
+        if (false === $checkPassed) {


IMO, the test can be change to:

if ($checkPassed === true && $verbose === false) { return true; }

And if we don't enter into this condition then the verbosity is always true (because of an error, or because it was asked)

I refactored this piece with different level of verbosity which I believe is cleaner and more in line with the verbosity of the console in general

MacFJA · 2018-04-14T13:10:17Z

src/RequirementChecker/LazyRequirement.php

+    public function isFulfilled()
+    {
+        if (null === $this->isFulfilled) {
+            $this->isFulfilled = eval($this->checkIsFulfilled);


I'm not a big fan of eval and a lot of company will block this function.

We know in advance that we have 2 types of checks: version_compare (for php) and extension_loaded (for extensions), maybe we can found an alternative way to do our checks

MacFJA · 2018-04-14T13:17:54Z

src/RequirementChecker/AppRequirementsFactory.php

+
+    private static function configurePhpVersionRequirements(LazyRequirementCollection $requirements, array $composerLockContents): void
+    {
+        $installedPhpVersion = PHP_VERSION;


Why do you need the version of PHP of the machine that make the phar?

The generated content will have nothing to do with it later on, as the runner machine is not the same as the maker machine

You're right. The evaluation is right as it's using the PHP version of the runner but the message generated is wrong as uses the PHP version used when building the PHAR

I "fixed" it by simply not displaying it: the PHP version is now showed in the output and the PHP version requirement check is the first one coming in the list so it should be pretty user friendly still without introducing the need to lazily evaluate the help message

MacFJA · 2018-04-14T15:43:11Z

Oh right, I didn't think about this case.

Maybe a normalization step can be added to remove backport/system additionnal data.

In Composer code they cleanup the version to handle this case.

It will be something like:

<?php

require_once __DIR__ . '/src/Semver.php';
require_once __DIR__ . '/src/VersionParser.php';
require_once __DIR__ . '/src/Constraint/ConstraintInterface.php';
require_once __DIR__ . '/src/Constraint/Constraint.php';
require_once __DIR__ . '/src/Constraint/MultiConstraint.php';

use Composer\Semver\Semver;

var_dump(Semver::satisfies('5.3', '^7.2')); // Return false
var_dump(Semver::satisfies('7.2', '^7.2')); // Return true
var_dump(Semver::satisfies('7.3', '^7.2')); // Return true

//var_dump(Semver::satisfies('7.0.28-0ubuntu0.16.04.1', '^7.2')); // Throw an UnexpectedValueException

$phpVersion = preg_replace('#^([^~+-]+).*$#', '$1', '7.0.28-0ubuntu0.16.04.1');
var_dump($phpVersion); // Return "7.0.28"
var_dump(Semver::satisfies($phpVersion, '^7.2')); // Return false
var_dump(Semver::satisfies(preg_replace('#^([^~+-]+).*$#', '$1', '7.1.11-0ubuntu0.17.10.1'), '^7.1')); // Return true

theofidry · 2018-04-14T22:42:19Z

fixtures/check-requirements/fail-complete/expected-output

+========================
+
+> PHP is using the following php.ini file:
+  /Users/tfidry/.phpbrew/php/php-7.1.10/etc/php.ini


Remove the dependency on the local path; Same for the others

theofidry · 2018-04-14T22:43:57Z

src/Box.php

-        Assertion::readable($file);
-
-        $contents = null === $contents ? file_get_contents($file) : $contents;
+        if (null === $contents) {


Should be a bug fixed in another PR

theofidry · 2018-04-14T22:55:59Z

I've pushed a new implementation:

Refactored to be simpler to read
The verbosity handling is now properly done. I'm not a big fan of how some IO related code is duplicated, but the issue is that relying on third-party code here is really annoying as 1. you need to include all the necessary code and 2. this code needs to be 5.3 compatible which won't be the case with Symfony 4+
The dependency to symfony/requirements-checker has been removed: that makes a bit of copy/pasted code but IMO there was enough differences to justify this:
- There is no optional requirements
- There is no HTML handling
- The conditions need to be serialized and evaluated lazily

Thanks for the help, I'll try to find a way to handle the PHP version thing

MacFJA · 2018-04-15T08:40:41Z

src/RequirementChecker/IO.php

+     */
+    private function parseInput()
+    {
+        $this->tokens = $_SERVER['argv'];


Why don't use getopt function ?

It can handle short and long argument syntax, remove the use of global variable $_SERVER, simplify the class. And IMHO, here, the arguments management is not fancy enough to need an external/custom implementation.

I agree the arguments management is not as complex as a regular input and is bound to be simpler because it can't know about the input definitions. So I simplified the implementation to reflect on that.

I admit I'm not familiar enough with the getopt() function. I added the tests for the IO class so it should be easier to change whilst ensuring the same feature set.

Would it be possible for you to take a look at it?

After a quick test getopt have some issue for this case:

It can not be UnitTested (cf. this message)

It can not handle option -vv and -vvv

Too bad, well I hope the current implementation appears simpler than previously then

theofidry · 2018-04-15T18:10:06Z

Ok added some more tests/bugfixes, there is only a handful of things left to do now...

theofidry · 2018-04-16T09:23:14Z

Remove the dependency to the php ini file location and PHP version in the tests

To be honest I'm not sure on how to do that one. One way would be to execute the tests in a container so we could have a fixed PHP version & php.ini file path, however I couldn't find how to hook the docker run logs on a host FS.

Maybe @Simperfit or @sanmai you have an idea? For a bit of context, I'm executing the PHAR and put the output in a actual-output file and then do a diff with the expected output: https://github.com/humbug/box/pull/116/files#diff-b67911656ef5d18c4ae36cb6741b7965R85

The dependency I'm referring to is this one and this one

theofidry · 2018-04-16T23:29:45Z

Updated the list of what's left to do. Looks like I won't be able to make it work without some patching in PHP-Parser & PHP-Scoper

theofidry · 2018-04-17T23:51:46Z

So there is a small bit missing from PHP-Scoper but I think this can be re-worked later. I need to update a couple of things in PHP-Scoper to simplify Box and vice-versa.

So now we're just waiting on that PHP-Parser issue. The real problem remaining is that dependency to the local paths. Ideally we would run those commands (the check requirements) inside the docker containers so that the path is controlled, but I don't know how to log the output of the command on a host file still

theofidry

Besides the comments, we are still missing:

Configure Travis to run those containers, check if there is a way to cache those images
Run the CS: this warrants a dedicated configuration for the 5.3 only code

theofidry · 2018-04-18T22:08:34Z

doc/configuration.md

@@ -105,6 +106,28 @@ an octal value e.g. `0750`. By default the permissions of the created PHAR are u
 Check the following [link](https://secure.php.net/manual/en/function.chmod.php) for more on the possible values.


+## Check requirements (`check-requirements`)


Any opinion about the name? Maybe this should be requirements-checker instead

theofidry · 2018-04-18T22:10:17Z

src/Box.php

@@ -240,7 +241,10 @@ public function addFile(string $file, string $contents = null, bool $binary = fa
        }

        if ($binary) {
-            $this->phar->addFile($file, $local);
+            true === file_exists($file)


this could be extracted in a separate PR

theofidry · 2018-04-18T22:10:58Z

src/Configuration.php

@@ -307,6 +326,16 @@ public function getBasePath(): string
        return $this->basePath;
    }

+    public function getComposerJson(): ?string


This could be extracted in a separate PR and tests added about the values gotten from those, e.g. when the file does not exists, is a directory and so on

src/Configuration.php

+    {
+        // TODO: emit warning when stub is not generated and check requirements is explicitly set to true
+        // TODO: emit warning when no composer lock is found but check requirements is explicitely set to true
+        if (false === $hasComposerLock) {


src/Console/Command/Compile.php

@@ -365,15 +380,41 @@ private function registerMainScript(Configuration $config, Box $box, BuildLogger
        return $localMain;
    }

-    private function registerStub(Configuration $config, Box $box, string $main, BuildLogger $logger): void
+    private function registerRequirementsChecker(Configuration $config, Box $box, BuildLogger $logger): bool


theofidry · 2018-04-18T22:19:13Z

src/RequirementChecker/IO.php

+    }
+
+    /**
+     * {@inheritdoc}


invalid inheritdoc doc block

src/RequirementChecker/Requirement.php

+ *
+ * @private
+ */
+final class Requirement


src/RequirementChecker/RequirementCollection.php

+ *
+ * @private
+ */
+final class RequirementCollection implements IteratorAggregate, Countable


tests/ConfigurationTest.php

@@ -2663,6 +2663,20 @@ public function testIsPrivateKeyPromptSetString(): void
        $this->assertFalse($this->config->isPrivateKeyPrompt());
    }

+    public function test_the_requirement_checker_is_enabled_by_default(): void


tests/StubGeneratorTest.php

@@ -218,6 +260,9 @@ public function test_it_can_generate_a_complete_stub(): void

 Phar::mapPhar('test.phar');
 Phar::interceptFileFuncs();
+
+require 'phar://test.phar/.box/check_requirements.php';
+
 require 'phar://test.phar/index.php';


theofidry · 2018-04-18T22:26:11Z

@MacFJA almost done. There is not much to do I believe I might be able to finish that this weekend. Maybe you want to do a review now or I can ping you when everything is green for a final review. As you wish :)

theofidry · 2018-04-19T23:09:51Z

Ready for review. The build failure looks like an issue in Infection itself rather that here, so I'm fine with it for now

MacFJA · 2018-04-20T21:29:44Z

src/RequirementChecker/AppRequirementsFactory.php

+        foreach ($packages as $packageInfo) {
+            $packageRequire = $packageInfo['require'] ?? [];
+
+            if (1 === preg_match('/symfony\/polyfill-(?<extension>.+)/', $packageInfo['name'], $matches)) {


I could be interesting to add the ability to have another polyfill format: the current implementation only allow polyfill from Symfony.

There are other polyfill like mcrypt_compat or random_compat (with the name containing FEATURE_compat)

Indeed! For now I just added this one because it was easy to do, but we could definitely add support for more polyfills. I would do in another PR though as it's already a lot here. I opened #131 for this to not be forgotten

MacFJA · 2018-04-21T11:53:26Z

src/RequirementChecker/AppRequirementsFactory.php

+
+    private static function generatePhpCheckStatement(string $requiredPhpVersion): string
+    {
+        return "return require_once 'Semver.php'; \\_HumbugBox\Composer\Semver::satisfies(preg_replace('#^([^~+-]+).*$#', '$1', \PHP_VERSION), '$requiredPhpVersion');";


After some research, as our minimum PHP version is 5.3, this part can be simplify to:

return "return require_once 'Semver.php'; \\_HumbugBox\Composer\Semver::satisfies(sprintf('%d.%d.%d', \PHP_MAJOR_VERSION, \PHP_MINOR_VERSION, \PHP_RELEASE_VERSION), '$requiredPhpVersion');";

No need of a RegExp to parse the version. Those constants have been added in PHP 5.2.7.

Tested on:

Ubuntu:

echo PHP_VERSION; // output: 7.0.28-0ubuntu0.16.04.1 echo sprintf('%d.%d.%d', \PHP_MAJOR_VERSION, \PHP_MINOR_VERSION, \PHP_RELEASE_VERSION); // output: 7.0.28

Docker (dockette/php71):

echo PHP_VERSION; // output: 7.1.15-1+0~20180306120016.15+stretch~1.gbp78327e echo sprintf('%d.%d.%d', \PHP_MAJOR_VERSION, \PHP_MINOR_VERSION, \PHP_RELEASE_VERSION); // output: 7.1.15

…-Scoper

theofidry · 2018-04-22T16:22:40Z

@MacFJA a few notes as I ended up changing a lot of things. Indeed the PHP version check was not working due to missing classes, but adding those classes was a real pain. In the end I ended up doing something simpler: I created a sub-project for the requirement checker for which I create a PHAR (which allows to filter only the necessary files as well as taking care of the scoping). Then extract this PHAR and add the extracted files to the Box PHAR, which then can add the requirement checker as usual.

The big benefit of that is:

It is going to be easier to filter the necessary files
The scoping is done by Box, not extra step required
We can freely use the autoloading except in the eval for the PHP version check

Feel free to do a final review, we can address the report points in new PRs. I'll be merge this now as I really need some of the stuff here and I think it's ok as a first iteration.

theofidry force-pushed the feature/requirements branch from d340b33 to 9cea332 Compare April 11, 2018 21:38

theofidry force-pushed the feature/requirements branch from 3f36857 to e12b31a Compare April 12, 2018 18:59

MacFJA reviewed Apr 14, 2018

View reviewed changes

theofidry commented Apr 14, 2018

View reviewed changes

MacFJA reviewed Apr 15, 2018

View reviewed changes

theofidry force-pushed the feature/requirements branch from 634a02d to 7464289 Compare April 16, 2018 18:29

theofidry mentioned this pull request Apr 16, 2018

Scope FQCN in evaled contents humbug/php-scoper#194

Closed

theofidry force-pushed the feature/requirements branch from 7464289 to 3483f31 Compare April 17, 2018 16:17

theofidry force-pushed the feature/requirements branch from 9c31192 to 5c014d9 Compare April 18, 2018 20:00

theofidry commented Apr 18, 2018

View reviewed changes

theofidry force-pushed the feature/requirements branch 3 times, most recently from 04d3bff to 3f18a30 Compare April 19, 2018 18:05

MacFJA reviewed Apr 20, 2018

View reviewed changes

theofidry mentioned this pull request Apr 20, 2018

Add support for functions polyfills for the requirement checker #131

Closed

MacFJA reviewed Apr 21, 2018

View reviewed changes

theofidry force-pushed the feature/requirements branch 3 times, most recently from 6466261 to 16d41fd Compare April 22, 2018 14:42

theofidry force-pushed the feature/requirements branch 6 times, most recently from 33a1b00 to 70f04fb Compare April 22, 2018 15:23

Add a requirement checker

18ab77a

theofidry force-pushed the feature/requirements branch from 70f04fb to 18ab77a Compare April 22, 2018 15:28

theofidry added 5 commits April 22, 2018 16:33

Fix EOF

570f429

Removing FQCN function calls: those optimisations will be made by PHP…

5f3efe3

…-Scoper

Fix test dependency

0880d83

Fix test

c0fc889

Fix dependencies

01e573b

theofidry force-pushed the feature/requirements branch from 821ff94 to 7933bef Compare April 22, 2018 16:05

Fix Infection build

e49382a

theofidry force-pushed the feature/requirements branch from 7933bef to e49382a Compare April 22, 2018 16:11

Remove unnused private methods

556bc25

theofidry merged commit 58e173f into box-project:master Apr 22, 2018

theofidry deleted the feature/requirements branch April 22, 2018 16:23

theofidry mentioned this pull request Apr 23, 2018

Check constraints before running the application #13

Closed


		$checkPassed = self::evaluateRequirements($requirements);

		if (false === $checkPassed) {

		@@ -105,6 +106,28 @@ an octal value e.g. `0750`. By default the permissions of the created PHAR are u
		Check the following [link](https://secure.php.net/manual/en/function.chmod.php) for more on the possible values.


		## Check requirements (`check-requirements`)

Add PHAR constraint check #116

Add PHAR constraint check #116

Conversation

theofidry commented Apr 11, 2018 • edited Loading

theofidry commented Apr 11, 2018

MacFJA commented Apr 14, 2018 • edited by theofidry Loading

theofidry commented Apr 14, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MacFJA commented Apr 14, 2018 • edited by theofidry Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Apr 15, 2018

theofidry commented Apr 16, 2018

theofidry commented Apr 16, 2018

theofidry commented Apr 17, 2018

theofidry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

theofidry commented Apr 18, 2018

theofidry commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Apr 22, 2018

theofidry commented Apr 11, 2018 •

edited

Loading

MacFJA commented Apr 14, 2018 •

edited by theofidry

Loading

MacFJA commented Apr 14, 2018 •

edited by theofidry

Loading

theofidry commented Apr 14, 2018 •

edited

Loading

theofidry left a comment •

edited

Loading