Restart the process with a different configuration #91

theofidry · 2018-03-09T19:24:14Z

No description provided.

weirdan · 2018-03-11T17:10:12Z

I see you're bundling XdebugHandler (under a different name). Is there a reason to not use composer/xdebug-handler package instead?

theofidry · 2018-03-11T18:14:25Z

So far it's an experiment so I'm trying to assess what I really need first and then how I can integrate an existing solution.

However looking at composer/xdebug-handler there is two things that are missing:

Handling the scanned ini files correctly (I'm still not sure how to do it and under what circumstances it appears exactly; see Fix generated php.ini config file infection/infection#215)
Allowing to tweak other settings than xdebug: in the case of Box for example, we also need to set the phar.readonly param to 0 or Off. In the case of Composer itself, it also set things like the memory limit.

There is also a slightly different case with https://github.com/infection/infection.

@johnstevenson your input could be welcomed here. IMO it would a great addition to have xdebug-handler (but then the name may require to change) being able to handle those cases properly. From afar the work done doesn't look trivial and has a few platform adjustments that I would be more than happy to not have to figure out

theofidry · 2018-03-21T23:46:10Z

ping @johnstevenson. Sorry I thought I pinged you initially but somehow I typed @ johnstevenson instead :/

johnstevenson · 2018-03-22T00:40:15Z

Could you outline exactly what you want to achieve, please.

theofidry · 2018-03-22T00:42:24Z

Mostly allowing to tweak other settings than xdebug: in the case of Box for example, we also need to set the phar.readonly param to 0 or Off or eventually the memory limit like done in Composer

johnstevenson · 2018-03-22T15:09:17Z

Okay, so you want to restart the process in two scenarios: when xdebug is enabled and/or if phar.readonly is enabled.

I cannot really understand what the inflection code you have used brings to the party, other than to completely confuse things by splitting the code up (and consequently miss out some important stuff, like actually storing the original inis in the environment so that PHP_INI_SCAN_DIR can be restored).

I'm just glad they didn't attribute it to me :) Having said that, though, this stuff can be surprisingly tricky to get one's head round.

I suggest that you start off by trying https://packagist.org/packages/composer/xdebug-handler This is a lot more robust now (in terms of testing a variety of ini environments) and it also provides all current ini settings to the restarted process, so command-line overrides are available.

To tweak other ini stuff, just call ini_set before xdebug-handler kicks in. This obviously won't work for phar.readonly or other stuff that is restricted so you would need to extend the library, use reflection to set the $loaded property and add your settings to the end of the tmp ini. But you'd be hacking a hack.

Alternatively you could update your entry points to call php -d phar.readonly=0 and this will get added to the tmp ini.

Hope this helps.

theofidry · 2018-03-22T15:48:49Z

Sorry I didn't push my PoC yesterday... That would have helped I think.

I cannot really understand what the inflection code you have used brings to the party, other than to completely confuse things by splitting the code up (and consequently miss out some important stuff, like actually storing the original inis in the environment so that PHP_INI_SCAN_DIR can be restored).

AFAIK the case with Infection is slightly different: when run Infection runs the tests once with coverage enabled (so it does require xdebug) and then generate mutants and will run the tests again for them. This second part should be run without. It does store and restore the original inis, it's however named differently.

Alternatively you could update your entry points to call php -d phar.readonly=0 and this will get added to the tmp ini.

Well the point is to avoid that :)

So I've just pushed a PoC using composer/xdebug-handler. Although rather than using it, it's more correct to say that right now I copy/pasted it and tweaked it to my needs.

So the status is the following: I've got something working right now. I however would like to leverage composer/xdebug-handler as much as possible which requires some changes to make it more extendable. I've annotated all the changes with the comments // MODIFIED & // --MODIFIED.

Would you mind taking a look at it and tell me if those changes look acceptable to you? If they are I'll do the PRs accordinly.

johnstevenson

Sorry but this is all to specific to your use case

johnstevenson · 2018-03-22T17:28:06Z

src/Php/PhpSettingsHandler.php

+class PhpSettingsHandler
+{
+    // MODIFIED
+    const SUFFIX_ALLOW = '_ALLOW_INITIAL_SETTINGS';


This would be bc break

yeah, I changed it to match more what I had but I think we can keep it unchanged that's ok

johnstevenson · 2018-03-22T17:31:11Z

src/Php/PhpSettingsHandler.php

+     *
+     * @throws RuntimeException If a parameter is invalid
+     */
+    final public function __construct($envPrefix, $colorOption = '', LoggerInterface $logger = null)


This breaks the tests

Yep. I kinda prefer to have the logger in the constructor as IMO there is no reason to rely on a setter here and this allows a cleaner form:

(new XdebugHandler(..., $logger))->check(); // as opposed to $handler = new XdebugHandler(...); $handler->setLogger($logger); $handler->check(); unset($handler);

No, it breaks the tests by making the constructor final!

oh that, we can remove it it's unnecessary :P Sorry I didn't actually try the implementation against the test yet I just manually tried it

johnstevenson · 2018-03-22T17:34:31Z

src/Php/PhpSettingsHandler.php

+
+        // MODIFIED
+        // renamed $this->loaded to $this->restart
+        $this->restart = $this->shouldRestart();


The original variable is a string containing the xdebug version or empty

yes, but now the purpose would be more needs to restart or not so a boolean would make more sense. Note though that I assume you always want to disable xdebug on restart in the whole thing, it's just that we are also changing other stuff.

It won't make any sense to folks who want to know (and report) the version (like Composer for example)

what about adding a second restart variable then? That way we can keep the current usage for loaded but the actual reloading is defined by the state of the boolean value restart

johnstevenson · 2018-03-22T17:35:35Z

src/Php/PhpSettingsHandler.php

+        $this->restart = $this->shouldRestart();
+
+        if (null !== $logger) {
+            $this->setLogger($logger);


What's this got to do with any of this?

See #91 (comment). I can live without that though

johnstevenson · 2018-03-22T17:36:05Z

src/Php/PhpSettingsHandler.php

+     */
+    protected function shouldRestart()
+    {
+        return extension_loaded('xdebug') || in_array(ini_get('phar.readonly'), ['1', 'On'], true);


Should be the xdebug version or an empty string

As per #91 (comment)

johnstevenson · 2018-03-22T17:38:01Z

src/Php/PhpSettingsHandler.php

+    private function writeTmpIni(array $iniFiles)
+    {
+        // MODIFIED
+        if (!$this->tmpIni = tempnam(sys_get_temp_dir(), $this->envPrefix.'_')) {


Identifiable tmp files can be a security risk, which is why we stopped doing something similar. Is there a specific reason you need this?

Oh I see. Well my train of though is that I wanted to be able to identify them to double check they were removed properly or something. Since it's for security reasons though we can disregard that change completely

johnstevenson · 2018-03-22T17:45:02Z

I made a handful of comments (I didn't review it all).

Alternatively you could update your entry points to call php -d phar.readonly=0 and this will get added to the tmp ini.
Well the point is to avoid that :)

Sure, but it seems an awful lot of work when you could just add a few characters to your .bat and create a one-line shell proxy. I guess there must be a good reason why folks using a phar library don't have this setting enabled ?!

theofidry · 2018-03-22T19:12:05Z

@johnstevenson thanks for the review. So what do you think about it? If you wish to keep composer/xdebug-handler as unchanged as possible which is understandable, what would be needed is:

An entry point to specify if it should be restarted or not. Right now what you have is $this->loaded = ... directly and it's a string version of the xdebug version. AFAICT the version itself serves no purposes besides logging. So if we could transform it to a boolean and move the assignment to a protected method (right no shouldRestart(), which we could name it as checkIfLoaded() or isLoaded().
An entry point for the ini settings: instead of doing ini_get_all() you could call a protected method
getIniSettings() which I could extend as I currently did

And with your review, actually I think that's the only elements really required. The rest is nitpicks I can live with if it allows me to re-use composer/xdebug-handler to that extend.

Also note that I added a logging entry for the restart command:

[debug] Restart with: "'/path/to/bin/php' '-c' '/poath/to/xEb6XO' 'bin/box' 'compile' '-vvv' '--ansi'"

Which I think could be added without any problem too

Sure, but it seems an awful lot of work when you could just add a few characters to your .bat and create a one-line shell proxy. I guess there must be a good reason why folks using a phar library don't have this setting enabled ?!

Because what people will do is:

composer require --dev humbug/box # or install the PHAR in one liner
box compile

That's it, and it should work. Sure it's not a requirement but it's a better DX to just make sure that works out of the box.

That enough was not good enough of a reason however. Another reason is that when using PHP-Scoper (see https://github.com/humbug/box/blob/master/doc/configuration.md#compactors-compactors), a lot of processing will be done relying on nikic/PHP-Parser. And this one is horribly slow if xdebug is used so disabling xdebug is almost a requirement here and it is more awkward to do at the user level

johnstevenson · 2018-03-22T21:18:32Z

Hey, thanks for the PRs. I just knocked you up a quick way to extend it, as it is at the moment:

use Composer\XdebugHandler\XdebugHandler;

class Restarter extends XdebugHandler
{
    private $refClass;

    public function __construct($logger)
    {
        parent::__construct('box', '--ansi');
        $this->setLogger($logger);

        if (ini_get('phar.readonly')) {
            $this->init();
        }
    }

    protected function restart($command)
    {
        if ($this->refClass) {
            $prop = $this->refClass->getProperty('tmpIni');
            $prop->setAccessible(true);
            $this->updateIni($prop->getValue($this));
        }

        parent::restart($command);
    }

    private function init()
    {
        $this->refClass = new \ReflectionClass('Composer\XdebugHandler\XdebugHandler');
        $prop = $this->refClass->getProperty('loaded');
        $prop->setAccessible(true);

        if (!$loaded = $prop->getValue($this)) {
            $prop->setValue($this, 'forced-restart');
        }
    }

    private function updateIni($tmpIni)
    {
        $content = file_get_contents($tmpIni);
        $content .= 'phar.readonly = 0'.PHP_EOL;
        file_put_contents($tmpIni, $content);
    }
}

As you surmised, I want to keep the library as unchanged as possible (because everything is there for a very real purpose). However, I think it could be useful if it was more easily extendable, so I'm sure we'll get somewhere in the end.

theofidry · 2018-03-23T01:30:43Z

Thanks @johnstevenson. I do like your solution, but what about composer/xdebug-handler#45? It would make it possible more easily without having to resort to reflection on private properties

Check for the PHP configuration when starting box and restart the process with xdebug disabled and `phar.readonly` turned off. Closes box-project#5

theofidry · 2018-04-07T20:16:27Z

Many thanks for the help @johnstevenson! I'm still having issues within PHPUnit i.e. if I allow the restart during the tests, it fails with a weird error:

[warning] No restart (Unable to access main script: bin/phpunit)

This is happening for the tests in CompileTest.

If I jumped to it, remove <env name="BOX_ALLOW_XDEBUG" value="1"/> of phpunit.xml.dist an
When I try to run the test directly from PhpStorm I see a different output (which is weird - I made sure it's executed from the same directory & same interpreter)

There was 1 error:

1) KevinGH\Box\Console\Command\CompileTest::test_it_can_build_a_PHAR_file
chdir(): No such file or directory (errno 2)

/Users/tfidry/Project/Humbug/box/vendor/phpunit/phpunit/phpunit:53

This scenario is weird enough so I'll just drop it as it is for now, but in case you encounter this kind of issue or have an idea, there's an easy reproducer here

johnstevenson · 2018-04-07T21:07:24Z

Hey thanks. I'll have a play with this later and see what is happening.

johnstevenson · 2018-04-07T21:11:15Z

Ah, it's a chdir issue. Use setMainScript(). If this is off the mark, then give me step by step outline of what calls what please.

theofidry · 2018-04-07T21:15:17Z

Use setMainScript(). If this is off the mark

Not exactly off the mark but rather how to guess which is the main script to use?

johnstevenson · 2018-04-07T21:18:16Z

The outline would be helpful ( especially as I'm drinking beer now)

theofidry · 2018-04-07T21:57:04Z

Ok I found the issue, indeed it's a chdir issue & I get a different error with PhpStorm because they have their phpunit-ide.php custom script.

I execute my tests with bin/phpunit from my project directory, the test that interests us is CompileTest
CompileTest creates a temporary directory and chdir() in it
CompileTest executes the Compile command
Compile calls XdebugHandler (the box variant) to check if a restart is needed, it does so it tries to restart
XdebugHandler checks the existence of the script bin/phpunit, because it's done from the temporary directory instead of the original directory the file does not exists and it fails

Now I can try to hack that last bit to make sure it point to the right script. However:

Making it absolute, e.g. with $_SERVER['argv'][0] = $this->cwd.'/'.$_SERVER['argv'][0]; will point to the right PHPUnit script but execute PHPUnit again which is not the desired effect...
Changing it to a php binary does not work for some reasons. I'm giving the absolute path to my PHP interpreter and the output is... cryptic...:

Also I realise I actually don't want a restart: I need xdebug for the right coverage anyway... So my real issue is actually sebastianbergmann/phpunit#3047.

So I don't think there is anything to fix on XdebugHandler side in the end... That said this report might help you for a weird case if that ever happens^^

especially as I'm drinking beer now

🍻 I'm actually passing on that one as I drank the whole week & day... But enjoy yours!

johnstevenson · 2018-04-07T22:40:08Z

Changing it to a php binary does not work for some reasons.

Well, php tries to run its binary as a script which I guess results in that output.

Also I realise I actually don't want a restart:

You can check for bin/phpunit in requiresRestart() and return false.

theofidry · 2018-04-07T22:44:11Z

You can check for bin/phpunit in requiresRestart() and return false.

I think it's easier to do by setting BOX_ALLOW_XDEBUG in the PHPUnit config. Checking against a PHPUnit script looks more brittle to me. Less hacky but still leveraging your extension points :)

johnstevenson · 2018-04-07T23:01:49Z

Checking against a PHPUnit script looks more brittle to me.

Sure, but it's either going to be your main box script (or something else). But the override variable in the PhpUnit config would be neat as well.

theofidry mentioned this pull request Mar 11, 2018

Fix generated php.ini config file infection/infection#215

Closed

theofidry force-pushed the feature/php-ini-restarter branch from 5001a0d to 7c510d5 Compare March 22, 2018 15:42

theofidry changed the title ~~[WIP] Restart the process with a different configuration~~ Restart the process with a different configuration Mar 22, 2018

johnstevenson reviewed Mar 22, 2018

View reviewed changes

theofidry mentioned this pull request Mar 22, 2018

Allow XdebugHandler to tweak PHP ini settings composer/xdebug-handler#45

Closed

theofidry added 2 commits April 7, 2018 21:33

Restart PHP process with the right settings

7e793e6

Check for the PHP configuration when starting box and restart the process with xdebug disabled and `phar.readonly` turned off. Closes box-project#5

Update the code thanks to the new changes made in Composer XdebugHandler

c40139a

theofidry force-pushed the feature/php-ini-restarter branch from 7c510d5 to c40139a Compare April 7, 2018 19:33

theofidry added 2 commits April 7, 2018 21:45

Fix tests

68a9f01

Add logging

1de79d6

theofidry merged commit 2066b0a into box-project:master Apr 7, 2018

theofidry deleted the feature/php-ini-restarter branch April 7, 2018 20:10

theofidry mentioned this pull request Apr 8, 2018

Automatically set phar.readonly=0 #5

Closed

Restart the process with a different configuration #91

Restart the process with a different configuration #91

Conversation

theofidry commented Mar 9, 2018

weirdan commented Mar 11, 2018

theofidry commented Mar 11, 2018 • edited Loading

theofidry commented Mar 21, 2018

johnstevenson commented Mar 22, 2018

theofidry commented Mar 22, 2018

johnstevenson commented Mar 22, 2018

theofidry commented Mar 22, 2018

johnstevenson left a 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

Choose a reason for hiding this comment

theofidry Mar 22, 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 Mar 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstevenson commented Mar 22, 2018

theofidry commented Mar 22, 2018 • edited Loading

johnstevenson commented Mar 22, 2018

theofidry commented Mar 23, 2018

theofidry commented Apr 7, 2018

johnstevenson commented Apr 7, 2018

johnstevenson commented Apr 7, 2018

theofidry commented Apr 7, 2018

johnstevenson commented Apr 7, 2018

theofidry commented Apr 7, 2018

johnstevenson commented Apr 7, 2018

theofidry commented Apr 7, 2018

johnstevenson commented Apr 7, 2018

theofidry commented Mar 11, 2018 •

edited

Loading

theofidry Mar 22, 2018 •

edited

Loading

theofidry Mar 22, 2018 •

edited

Loading

theofidry commented Mar 22, 2018 •

edited

Loading