Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Console] Enable process isolantion in Shell #2894

Merged
merged 1 commit into from Feb 2, 2012

Conversation

Projects
None yet
4 participants
Contributor

canni commented Dec 15, 2011

Bug fix: no
BC break: no
Feature addition: yes
Symfony2 test pass: yes
Fixes the following tickets: #2848 #2847
Todo: Write unit tests

See tickets for reference, need help with unit testing, because I don't know how to test this :)

@stof stof and 2 others commented on an outdated diff Dec 15, 2011

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -67,6 +68,10 @@ public function doRun(InputInterface $input, OutputInterface $output)
if (true === $input->hasParameterOption(array('--shell', '-s'))) {
$shell = new Shell($this);
+ if (true == $input->hasParameterOption(array('--process-isolation'))) {
+ $shell->setProcessIsolation(true);
+ $shell->setEntryScript($this->kernel->getContainer()->getParameter('kernel.root_dir').DIRECTORY_SEPARATOR.'console');
@stof

stof Dec 15, 2011

Member

you should not hardcode the file name here (especially as it is not configurable). It should be possible to retrieve it from the call to the console

@canni

canni Dec 15, 2011

Contributor

Yep I've put this in public for discussion, I was wondering, should this be always console file or configurable value (parameter)

@Seldaek

Seldaek Dec 15, 2011

Member

You should be able to get it from $_SERVER['argv'][0]

@stof stof commented on an outdated diff Dec 15, 2011

src/Symfony/Component/Console/Shell.php
@@ -79,7 +91,20 @@ public function run()
readline_write_history($this->history);
}
- if (0 !== $ret = $this->application->run(new StringInput($command), $this->output)) {
+ if ($this->processIsolation) {
+ $process = new Process(
+ sprintf('%s %s %s', $php, $this->entryScript, $command),
@stof

stof Dec 15, 2011

Member

you need to check that the entry script is set here

@Seldaek Seldaek commented on the diff Dec 15, 2011

...ymfony/Bundle/FrameworkBundle/Console/Application.php
@@ -39,6 +39,7 @@ public function __construct(KernelInterface $kernel)
parent::__construct('Symfony', Kernel::VERSION.' - '.$kernel->getName().'/'.$kernel->getEnvironment().($kernel->isDebug() ? '/debug' : ''));
$this->getDefinition()->addOption(new InputOption('--shell', '-s', InputOption::VALUE_NONE, 'Launch the shell.'));
+ $this->getDefinition()->addOption(new InputOption('--process-isolation', null, InputOption::VALUE_NONE, 'Launch commands from shell as a separate processes.'));
@Seldaek

Seldaek Dec 15, 2011

Member

Shouldn't this be true by default? I mean either it helps everybody and we enable it always, or it causes issues and we tell people to be careful when using the shell.

@canni

canni Dec 15, 2011

Contributor

This may break the "Expected behavior" for users, as it shouldn't break the BC
And, this removes the colorful output from commands :)

@canni

canni Dec 15, 2011

Contributor

This disables interactive mode in shell, so for me it should be only an option, without interactive mode, why you would use shell anyway (besides auto-completion)?.

Contributor

canni commented Dec 16, 2011

I've tested this with different scenarios like "inception" (invoking shell from shell - will not work) ;) and others, everything seems to work great.

As I have no idea on how to pack this with unit testing some help needed, also as I don't have any windows in home ;) need someone to test it on MS os.
And we should decide, do we want process isolation by default? (This will not break the BC, break only the "expected behavior" - colorful output and "interactivity")

Contributor

canni commented Dec 18, 2011

I've rebased this branch to match current HEAD and I've added usage of new process builder, for better portability an shell arg escaping.

Owner

fabpot commented Feb 2, 2012

@canni: Can you squash your commits before I merge this PR? Thanks.

@stof stof commented on an outdated diff Feb 2, 2012

src/Symfony/Component/Console/Shell.php
@@ -65,6 +67,20 @@ public function run()
}
$this->output->writeln($this->getHeader());
+ $php = null;
+ if ($this->processIsolation) {
+ $finder = new PhpExecutableFinder();
+ $php = $finder->find();
+ $this->output->writeln(<<<EOF
+<info>Running with process isolation, you should consider this:</info>
+ * each commad is executed as separate process,
@stof

stof Feb 2, 2012

Member

typo here

@stof stof commented on an outdated diff Feb 2, 2012

src/Symfony/Component/Console/Shell.php
+ ->add($php)
+ ->add($_SERVER['argv'][0])
+ ->add($command)
+ ->inheritEnvironmentVariables(true)
+ ->getProcess()
+ ;
+
+ $output = $this->output;
+ $process->run(function($type, $data) use ($output) {
+ $output->writeln($data);
+ });
+
+ if (0 !== ($ret = $process->getExitCode())) {
+ $output->writeln(sprintf('<error>The command terminated with an error status (%s)</error>', $ret));
+ }
+ } else if (0 !== $ret = $this->application->run(new StringInput($command), $this->output)) {
$this->output->writeln(sprintf('<error>The command terminated with an error status (%s)</error>', $ret));
@stof

stof Feb 2, 2012

Member

you should not duplicate the error message IMO. populate $ret in both cases with the return code and then do the check on its value

Contributor

canni commented Feb 2, 2012

@fabpot @stof done.

@canni canni [Console] Enable process isolantion in Shell
Bug fix: no
BC break: no
Feature addition: yes
Symfony2 test pass: yes
Fixes the following tickets: #2848 #2847
Todo: -

See tickets for reference, need help with testing, because I don't know how to test this :)
e9b4c58

@fabpot fabpot added a commit that referenced this pull request Feb 2, 2012

@fabpot fabpot merged branch canni/command_in_process (PR #2894)
Commits
-------

e9b4c58 [Console] Enable process isolantion in Shell

Discussion
----------

[Console] Enable process isolantion in Shell

Bug fix: no
BC break: no
Feature addition: yes
Symfony2 test pass: yes
Fixes the following tickets: #2848 #2847
Todo: Write unit tests

See tickets for reference, need help with unit testing, because I don't know how to test this :)

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

by canni at 2011-12-16T09:36:32Z

I've tested this with different scenarios like "inception" (invoking shell from shell - will not work) ;) and others, everything seems to work great.

As I have no idea on how to pack this with unit testing some help needed, also as I don't have any windows in home ;) need someone to test it on MS os.
And we should decide, do we want process isolation by default? (This will not break the BC, break only the "expected behavior" - colorful output and "interactivity")

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

by canni at 2011-12-18T15:14:26Z

I've rebased this branch to match current `HEAD` and I've added usage of new process builder, for better portability an shell arg escaping.

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

by fabpot at 2012-02-02T08:28:32Z

@canni: Can you squash your commits before I merge this PR? Thanks.

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

by canni at 2012-02-02T09:07:16Z

@fabpot @stof done.
687703d

@fabpot fabpot merged commit e9b4c58 into symfony:master Feb 2, 2012

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