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

[Process][Tests] Prove process fail #5575

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lyrixx
Member

lyrixx commented Sep 22, 2012

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets: -
Todo: Fix that
License of the code: MIT

I am not sure this is an issue, but the following code does not work :

$process = new Process("echo -n 1 ; echo -n 1");
$process->run();
var_dump('11' == $process->getOutput()); // false, 
var_dump($process->getOutput()) // 1

I don't know why.

tests are run on php 5.4.7 and ubuntu 11.04

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 24, 2012

Member

BTW, this test fails on PHP 5.3.15

Member

lyrixx commented Sep 24, 2012

BTW, this test fails on PHP 5.3.15

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 24, 2012

Member

This is actually normal because echo -n 1 ; echo -n 1 is two process.
By design, Process return the output of the first command you pass.

There's a workaround for your problem :

$process = new Process("bash -c 'echo -n 1 ; echo -n 1'");
$process->run();
var_dump('11' == $process->getOutput()); // true, 
Member

romainneutron commented Sep 24, 2012

This is actually normal because echo -n 1 ; echo -n 1 is two process.
By design, Process return the output of the first command you pass.

There's a workaround for your problem :

$process = new Process("bash -c 'echo -n 1 ; echo -n 1'");
$process->run();
var_dump('11' == $process->getOutput()); // true, 
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

another solution is to run it as a single process: echo -n 1 && echo -n 1

Member

stof commented Sep 25, 2012

another solution is to run it as a single process: echo -n 1 && echo -n 1

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

I added more tests case , and it always fail.
I think it's a bug.

I found this bug in Sismo repo https://github.com/fabpot/Sismo/blob/master/tests/Sismo/Tests/GithubProjectTest.php#L51 : When I lunch the test suite, Sismo tests fails because git remote add ... is not executed.
So I tried to reproduce and to isolate this "bug".

Note: 86d9dea is tested on debian 6 and php 5.3.15

`1) Symfony\Component\Process\Tests\SigchildDisabledProcessTest::testChainedCommandsOutput with data set #0 ('11', ';', '1')
Failed asserting that '1' matches expected '11'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

2) Symfony\Component\Process\Tests\SigchildDisabledProcessTest::testChainedCommandsOutput with data set #1 ('22', '&&', '2')
Failed asserting that '2' matches expected '22'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

3) Symfony\Component\Process\Tests\SimpleProcessTest::testChainedCommandsOutput with data set #0 ('11', ';', '1')
Failed asserting that '1' matches expected '11'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

4) Symfony\Component\Process\Tests\SimpleProcessTest::testChainedCommandsOutput with data set #1 ('22', '&&', '2')
Failed asserting that '2' matches expected '22'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99
Member

lyrixx commented Sep 25, 2012

I added more tests case , and it always fail.
I think it's a bug.

I found this bug in Sismo repo https://github.com/fabpot/Sismo/blob/master/tests/Sismo/Tests/GithubProjectTest.php#L51 : When I lunch the test suite, Sismo tests fails because git remote add ... is not executed.
So I tried to reproduce and to isolate this "bug".

Note: 86d9dea is tested on debian 6 and php 5.3.15

`1) Symfony\Component\Process\Tests\SigchildDisabledProcessTest::testChainedCommandsOutput with data set #0 ('11', ';', '1')
Failed asserting that '1' matches expected '11'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

2) Symfony\Component\Process\Tests\SigchildDisabledProcessTest::testChainedCommandsOutput with data set #1 ('22', '&&', '2')
Failed asserting that '2' matches expected '22'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

3) Symfony\Component\Process\Tests\SimpleProcessTest::testChainedCommandsOutput with data set #0 ('11', ';', '1')
Failed asserting that '1' matches expected '11'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99

4) Symfony\Component\Process\Tests\SimpleProcessTest::testChainedCommandsOutput with data set #1 ('22', '&&', '2')
Failed asserting that '2' matches expected '22'.

/var/www/dev/labs/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:99
@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

This s still actually normal, @stof example is wrong as it's the same of your example, there is still two commands

You have to wrap it inside a bash script to get your expected result $process = new Process("bash -c 'echo -n 1 ; echo -n 1'");

Member

romainneutron commented Sep 25, 2012

This s still actually normal, @stof example is wrong as it's the same of your example, there is still two commands

You have to wrap it inside a bash script to get your expected result $process = new Process("bash -c 'echo -n 1 ; echo -n 1'");

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

@romainneutron But, how can sismo test pass ? It is impossible that @fabpot push failed tests ! ;)

Member

lyrixx commented Sep 25, 2012

@romainneutron But, how can sismo test pass ? It is impossible that @fabpot push failed tests ! ;)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@romainneutron is it really 2 processes when using && ?

Member

stof commented Sep 25, 2012

@romainneutron is it really 2 processes when using && ?

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

BTW, my sample is not related to echo (echo is a shell builtin).

$process = new Process("git status && git status");

have the same problem.

Member

lyrixx commented Sep 25, 2012

BTW, my sample is not related to echo (echo is a shell builtin).

$process = new Process("git status && git status");

have the same problem.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

This PR is related to #5398
Before this PR, tests passed

EDIT : I was wrong, the changeless happend in 7bafc69 ; Related PR : #5543

Member

lyrixx commented Sep 25, 2012

This PR is related to #5398
Before this PR, tests passed

EDIT : I was wrong, the changeless happend in 7bafc69 ; Related PR : #5543

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

It's pretty weird, this PR is related to exceptions.

Are you sure you're pointing the right one ?

@stof If you run sleep 3 && sleep 3 and monitor, you'll see that two process with two differents pids are run

Member

romainneutron commented Sep 25, 2012

It's pretty weird, this PR is related to exceptions.

Are you sure you're pointing the right one ?

@stof If you run sleep 3 && sleep 3 and monitor, you'll see that two process with two differents pids are run

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

@lyrixx BTW I'm sure that if you encapsulate your commands with bash -c "...", you won't have these problems

Member

romainneutron commented Sep 25, 2012

@lyrixx BTW I'm sure that if you encapsulate your commands with bash -c "...", you won't have these problems

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@lyrixx I don't understand how it is possible. The PR is not changing the logic at all. It is simply using a subclass for exception it throws

Member

stof commented Sep 25, 2012

@lyrixx I don't understand how it is possible. The PR is not changing the logic at all. It is simply using a subclass for exception it throws

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

Oh, sorry, i pointed a wrong PR, the changeless happend in 7bafc69 ; Related PR : #5543

@romainneutron I agree with you about bash -c "..."

Member

lyrixx commented Sep 25, 2012

Oh, sorry, i pointed a wrong PR, the changeless happend in 7bafc69 ; Related PR : #5543

@romainneutron I agree with you about bash -c "..."

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

Ok, the change appeared because we now preprend commands with exec.
I'm sorry, I did'nt notice it happened and introduce a BC break

This change is quite important and necessary because it allows us to get the exact Pid of the process, signal it and have more control over it.

If we remove it , the process will be wrapped by a sh -c as mentionned by in #5030 and we will not have access to its Pid ; we will not be able to signal it.

I do think we have to mention it in the doc and docblocks, I'll work on it

Member

romainneutron commented Sep 25, 2012

Ok, the change appeared because we now preprend commands with exec.
I'm sorry, I did'nt notice it happened and introduce a BC break

This change is quite important and necessary because it allows us to get the exact Pid of the process, signal it and have more control over it.

If we remove it , the process will be wrapped by a sh -c as mentionned by in #5030 and we will not have access to its Pid ; we will not be able to signal it.

I do think we have to mention it in the doc and docblocks, I'll work on it

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@romainneutron but without the wrapping in sh -c, it looks like it does not support all previous cases anymore, meaning this particular change should be reverted at least in the 2.1 branch as it is not BC.

Member

stof commented Sep 25, 2012

@romainneutron but without the wrapping in sh -c, it looks like it does not support all previous cases anymore, meaning this particular change should be reverted at least in the 2.1 branch as it is not BC.

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

@stof which cases ? such one : new Process('git status && git branch -a') ?

Member

romainneutron commented Sep 25, 2012

@stof which cases ? such one : new Process('git status && git branch -a') ?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@romainneutron yes. And I can tell you that such cases are used in many places in Composer for instance.

Member

stof commented Sep 25, 2012

@romainneutron yes. And I can tell you that such cases are used in many places in Composer for instance.

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

So, this is an issue

Member

romainneutron commented Sep 25, 2012

So, this is an issue

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@fabpot should we revert the prepending of exec then ?

Member

stof commented Sep 25, 2012

@fabpot should we revert the prepending of exec then ?

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

Right now, I don't have any quick fix except revert. But revert will drop many features

Member

romainneutron commented Sep 25, 2012

Right now, I don't have any quick fix except revert. But revert will drop many features

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

we should probably revert for 2.1 and work on it on 2.2

Member

romainneutron commented Sep 25, 2012

we should probably revert for 2.1 and work on it on 2.2

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

And, if we wrap exec with parenthesis ?

$this->commandline = 'exec (' . $this->commandline . ')';

EDIT : does not work

Member

lyrixx commented Sep 25, 2012

And, if we wrap exec with parenthesis ?

$this->commandline = 'exec (' . $this->commandline . ')';

EDIT : does not work

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@romainneutron drop many features ? It will not drop features comparing to the state in 2.0 but reintroduce the BC.
The prepending of exec is dropping some supported cases currently, and so breaking some use cases. Sending signals is only a feature request for 2.2, not a supported feature in 2.1.

Member

stof commented Sep 25, 2012

@romainneutron drop many features ? It will not drop features comparing to the state in 2.0 but reintroduce the BC.
The prepending of exec is dropping some supported cases currently, and so breaking some use cases. Sending signals is only a feature request for 2.2, not a supported feature in 2.1.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

Auto-answer : It does not work.

Member

lyrixx commented Sep 25, 2012

Auto-answer : It does not work.

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

okay, you are correct. The only features I'm wondering that might have a substancial change is ones that use proc_terminate and could have some glitchy results

Member

romainneutron commented Sep 25, 2012

okay, you are correct. The only features I'm wondering that might have a substancial change is ones that use proc_terminate and could have some glitchy results

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

As proc_terminate will send signal on the sh wrapper, then this wrapper could lose its child which will run without parent process

Member

romainneutron commented Sep 25, 2012

As proc_terminate will send signal on the sh wrapper, then this wrapper could lose its child which will run without parent process

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

@stof What do you think about this PR, Should it be merged ? If yes, into master, 2.1, 2.0 ?

Member

lyrixx commented Sep 25, 2012

@stof What do you think about this PR, Should it be merged ? If yes, into master, 2.1, 2.0 ?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2012

Member

@lyrixx in 2.1 as this is where the exec has been introduced and needs to be reverted.

The tests could also be added in 2.0 but it is not worth it IMO as merging tests between 2.0 and 2.1 is a real pain for @fabpot as we moved them.

Member

stof commented Sep 25, 2012

@lyrixx in 2.1 as this is where the exec has been introduced and needs to be reverted.

The tests could also be added in 2.0 but it is not worth it IMO as merging tests between 2.0 and 2.1 is a real pain for @fabpot as we moved them.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 25, 2012

Member

OK, So i sould close this PR right now, and open it again against 2.1 branch ?

Member

lyrixx commented Sep 25, 2012

OK, So i sould close this PR right now, and open it again against 2.1 branch ?

@romainneutron

This comment has been minimized.

Show comment
Hide comment
@romainneutron

romainneutron Sep 25, 2012

Member

yup, and reference this PR in your new one

Member

romainneutron commented Sep 25, 2012

yup, and reference this PR in your new one

@lyrixx lyrixx closed this Sep 25, 2012

fabpot added a commit that referenced this pull request Sep 28, 2012

merged branch lyrixx/process (PR #5592)
Commits
-------

27b2df9 [Process] Fixed bug introduced by 7bafc69.
7a955c0 [Process][Tests] Prove process fail (Add more test case)
598dcf3 [Process][Tests] Prove process fail

Discussion
----------

[Process][Tests] Prove process fail with chained commands

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets: -
Todo: Fix that
License of the code: MIT

This PR is against 2.1 branch. Previous PR was #5575

This PR try to hiligh a regression in Process component.

``` php
$process = new Process("echo -n 1 && echo -n 1");
// or $process = new Process("echo -n 1 ; echo -n 1");
$process->run();
var_dump('11' == $process->getOutput()); // false,
var_dump($process->getOutput()); // '1',
```

This test failed because of PR #5543 ; see 7bafc69#L0R233

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:05:45Z

You've to revert the change that causes the fail (ie: remove https://github.com/symfony/symfony/blob/2.1/src/Symfony/Component/Process/Process.php#L233)

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:06:56Z

BTW, removing this line re-open #5030

---------------------------------------------------------------------------

by stof at 2012-09-25T13:11:15Z

@lyrixx please add a commit reverting the addition of ``exec`` in the case of sigchild not being used (only this addition, not the full commit you linked) as it should fix your test.

---------------------------------------------------------------------------

by stof at 2012-09-25T13:12:21Z

@fabpot btw, this regression is quite important. As I said in the previous PR, it impacts composer in a bunch of places.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:30:07Z

You reverted too much things, you just had to remove line 233

---------------------------------------------------------------------------

by stof at 2012-09-25T13:42:49Z

@lyrixx I explicitly asked you to revert only the ``exec`` addition for the case without sigchild.

---------------------------------------------------------------------------

by lyrixx at 2012-09-25T13:55:57Z

@stof Sorry, i fixed that.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:56:26Z

@lyrixx just remove the two last commit, edit Process.php and remove line 233

---------------------------------------------------------------------------

by lyrixx at 2012-09-25T13:59:59Z

@romainneutron I think it's ok now.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T14:11:28Z

yep it's good :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment