Skip to content
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

Forward PATH to child processes #24

Closed
wants to merge 1 commit into from
Closed

Conversation

Seldaek
Copy link

@Seldaek Seldaek commented Jan 4, 2015

Without this I can't get it to run at all, as it doesn't find php or phpunit or any command I give it. Not sure if it's the correct fix..

@liuggio
Copy link
Owner

liuggio commented Jan 12, 2015

Sorry for the late reply, I was on holiday...
I need to investigate, but what $_server['path'] is?
Which OS are you using?

@Seldaek
Copy link
Author

Seldaek commented Jan 12, 2015

Windows and $_SERVER['PATH'] is my PATH environment variable.

@ioleo
Copy link
Collaborator

ioleo commented Jan 28, 2016

This should be fixed by #52

@Seldaek can you retest and confirm?

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

$ find tests/ -name "*Test.php" | ../fastest/fastest
- 87 shuffled tests into the queue.
- Will be consumed by 4 parallel Processes.


87/87 [============================] 100%  1 sec 4.0 MiB

     87 failures.
[1] tests/Composer/Test/Repository/CompositeRepositoryTest.php'bin' is not recognized as an internal or external command,
operable program or batch file.

All tests fail with that same message. Sounds like it tries to execute "bin" as a command, not sure why and I don't really have the motivation to debug this right now.. Have you tried it on windows at all? :)

@ioleo
Copy link
Collaborator

ioleo commented Jan 29, 2016

@Seldaek without the -c option, the default command is bin/phpunit {}

for Windows you need to change that to bin\phpunit {}

so try with find tests/ -name "*Test.php" | ../fastest/fastest -c "bin\phpunit {}"

ill make a PR with a fix for that

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

[3] tests/Composer/Test/Downloader/PerforceDownloaderTest.php'phpunit' is not recognized as an internal or external command,
operable program or batch file.

$ which phpunit
/c/Users/seld/AppData/Roaming/Composer/vendor/bin/phpunit

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

Seems like that is a similar issue to what I was having when I created this PR :)

@ioleo
Copy link
Collaborator

ioleo commented Jan 29, 2016

@Seldaek you must edit your variable_order in php.ini to add E flag

see readme

PS. I've discovered and added it to readme yesterday :P

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

Well.. sorry but I don't think I should have to tweak my variable_order to make this work. It's just not nice user experience :) $_SERVER['PATH'] or getenv('PATH') work fine without having the E in var order. Can't fastest just use that?

@ioleo
Copy link
Collaborator

ioleo commented Jan 29, 2016

@Seldaek fastest uses $_ENV variable to pass all enviroment variables (along with PATH) to the child process. By default php has variable_order set to EGPCS (so, haveing E flag is the default). However some Windows PHP stacks (like EasyPHP) have it set to GPCS by default.

Without E flag, the $_ENV variable is empty.

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

Actually no, EGPCS is the default if not specified, but php.ini-production and php.ini-development in php7 both ship without E.

; Default Value: "EGPCS"
; Development Value: "GPCS"
; Production Value: "GPCS";
; http://php.net/variables-order
variables_order = "GPCS"

proc_open's env argument should just be set to null to forward current env (docs say An array with the environment variables for the command that will be run, or NULL to use the same environment as the current PHP process).

@ioleo
Copy link
Collaborator

ioleo commented Jan 29, 2016

@Seldaek fastest needs to add some custom ENV variables per process (so it cannot set them in the ENV) and the part responsible for this is Process/EnvCommandCreator.php#L16

To also include current process ENV variables it uses $_ENV. So we can't set it to null.

@Seldaek
Copy link
Author

Seldaek commented Jan 29, 2016

Hmm ok.. you could probably just use putenv() to set your new env vars and then use null to forward it all though?

@liuggio
Copy link
Owner

liuggio commented Jan 29, 2016

Hi
I see two problems here:

  1. the "PATH" env that is accesible only via getenv('PATH')
  2. decide which is (or how to get) the standard test command to execute on windows.

for the first maybe just adding the PATH with getenv here: https://github.com/liuggio/fastest/blob/master/src/Process/EnvCommandCreator.php#L17

for the second I'm not a Windows user but maybe if is well configured we could just leave phpunit /phpunit.de/#installation.phar.windows

@liuggio
Copy link
Owner

liuggio commented Jan 29, 2016

/c @loostro

@ioleo
Copy link
Collaborator

ioleo commented Jan 29, 2016

@Seldaek sounds good, i'll test that

@ioleo
Copy link
Collaborator

ioleo commented Mar 23, 2016

Should be fixed by #58.

@ioleo ioleo closed this Mar 23, 2016
@ioleo
Copy link
Collaborator

ioleo commented Mar 23, 2016

Reopening, as #58 has been reverted due to some unexpected side effects.

@francoispluchino
Copy link
Collaborator

This PR can be closed, because already fixed.

@DonCallisto
Copy link
Collaborator

@francoispluchino have you tested it?

@francoispluchino
Copy link
Collaborator

Yes, and it's tested in the Liuggio\Fastest\Process\ProcessFactoryTest class.

@DonCallisto
Copy link
Collaborator

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants