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

Add in support for setting a process title. #280

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

keyurdg commented Feb 19, 2013

Contributor

auroraeosrose commented Feb 22, 2013

I have a few pretty small comments on the current code

  1. I see a few C++ style comments (//) floating about, make sure those are all /* */
  2. PHP_FE(cli_get_process_title, NULL) - a null for arginfo probably won't get you what you want
    (try adding a test that calls it with a few args or looking at it via reflection)
  3. also in cli_get_process_title - you need to be using zend_parse_parameters_none
  4. You should separate out the test so the basic calls are tested on all operating systems
    have a test that runs getmypid and ps trick portions are run on non-windows
    on windows a command line like so
    tasklist /fi "imagename eq php.exe" /v | find /i "my title here" 2>NUL
    should allow you to check for the title

Keyur Govande added some commits Feb 22, 2013

Keyur Govande Clean up allocated argv/environ if applicable. This will ensure clean
valgrind runs. (hat-tip to Christopher Jones for the idea)
9a881e0
Keyur Govande Fixes for review comments by auroraeosrose b54f25c
Contributor

keyurdg commented Feb 22, 2013

Thanks @auroraeosrose

Fixed comments 1, 2 and 3.

For the tests on Windows, we don't set the image name, instead we set an event (http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx). Task manager does not show these events; the only program I know of that does is Process Explorer which is not available by default on the box. Any other ideas?

Contributor

auroraeosrose commented Feb 23, 2013

random aside - any reason you're not doing a SetConsoleTitle for win32 - shows up in task manager->applications and other places (not quite the same but still useful)

and then use
PowerShell "get-process php* | Select-Object MainWindowTitle"

to test with powershell (if available - most people who compile/test php have it - just add a skipif ;)

Probably your best bet would be to at least split the tests up anyway - so that the basic usage still attempts to run on windows (you'll still catch leaks or parameter errors that way) and only do the actual functionality test on the linux end

Contributor

keyurdg commented Feb 23, 2013

Good question :) This whole thing is inspired by the PostgreSQL implementation and I'm not very familiar with the Windows API so I didn't give it much thought.

Looking at the docs for SetConsoleTitle, it looks like it wouldn't work as well for a real Windows service? Please correct me if I'm wrong.

On the test, my thoughts were since we couldn't verify that the event was actually set on Windows, it would be misleading to mark the test as passed. I'll create a separate test just to ensure there's no warnings, etc. when making the new API calls.

Contributor

auroraeosrose commented Feb 23, 2013

Real windows service? unless you're using an extension that has other windows API calls you can't do a real windows service with PHP (LOL)

Using php.exe or php-win.exe setconsoletitle would work fine

Contributor

weltling commented Feb 25, 2013

Hi,

compiled with --disable-all --enable-cli. On Linux it's ok when looking with ps, but on windows I can't see any effect with this snippet (using process explorer or standard tools):

Contributor

keyurdg commented Feb 26, 2013

@weltling @auroraeosrose I've changed the code to use SetProcessTitle() on Windows. Give it a whirl now :-)

Also updated the tests to run on Windows.

Contributor

weltling commented Feb 26, 2013

@keyurdg Tested your change vc11/ts/nts - now I can see the windows title changing. Great :). However the process name in the task manager is CLI, not what in the window title is (using the previously posted stippet). Not sure if it could work setting the process name, too. Also running the test, there is the following diff:

003+ Actually loaded from ps: C:\Windows\system32\cmd.exe
004+ Actually loaded from get: Windows PowerShell
003- Successfully verified title using ps
004- Successfully verified title using get

If that's just a test bug, if so, it'd be probably better to fork that test wor windows.

Contributor

keyurdg commented Feb 26, 2013

@weltling Windows does not support changing the process title that's visible in the "Processes" tab of task manager. It will remain php.exe or cmd.exe as applicable.

W/r/t the tests, could you tell me how you are running these? The test passes for me when run as PS php-src> x64\Release_TS\php.exe run-tests.php -p x64\Release_TS\php.exe sapi\cli\tests\cli_process_title.phpt

Contributor

weltling commented Feb 27, 2013

@keyurdg Ok, so the window title is only what we have on windows.

Actually I did the test in a boringly normal cmd window
nmake test TESTS=sapi\cli\tests\cli_process_title.phpt

That's equivalent to what you did (nmake is just not in the PATH in PS). Running it your way in PS gives yet another diff

================ START ===================
003+ Actually loaded from ps: C:\Windows\system32\cmd.exe

004+

003- Successfully verified title using ps
004- Successfully verified title using get
005+

006+

007+

008+ Windows PowerShell
009+ Actually loaded from get: Windows PowerShell
================ END ===================

That's on win8 and x86.

I'd suggest you to split that test as you already have Linux, Windows and BSD in there. That makes it too complex and it might get even more with addition of some other platforms. Straightforward small units are easier debuggable.

Contributor

auroraeosrose commented Feb 27, 2013

I agree - a split test would make this a lot easier to handle and the windows stuff will need a little more flexibility on output looks like...

Keyur Govande Split test for Windows and Unix.
Also fix test for Windows 8: It no longer verifies the title using
PowerShell. It only executes the API calls.
On Windows 7 and older though, we still continue to verify the title.
3443ece
Contributor

keyurdg commented Feb 27, 2013

@auroraeosrose @weltling I split the test into Windows and Unix flavors; also modified it for Windows 8 where we no longer verify the title using Get-Process. More details about why in the commit 3443ece

Contributor

weltling commented Feb 27, 2013

@keyurdg I had no chance to test on win7 but on win8 the test passes. Great work!

Contributor

keyurdg commented Feb 28, 2013

Thanks :)

TBH the test on Windows is flaky. Windows XP also handles the ConsoleTitle differently from Windows 7 and 8. It seems to append "Administrator" to whatever title.

I'm currently setting up Server 2008 as my last test case.

Contributor

weltling commented Feb 28, 2013

PHP 5.5 where this PR should come in is already announced to have no Windows XP support, no need to care about it :) The lowest versions we test with are Vista SP2 and 2008 SP1.

Contributor

keyurdg commented Feb 28, 2013

No doubt, but we are going to backport this to 5.3 and 5.4, so figured I'd have a passing test :-)

I'll see if I can get my hands on a Vista box.

Update: the tests pass on Vista SP2 as well.

Contributor

keyurdg commented Mar 5, 2013

The RFC was accepted. Please merge this pull request. Thanks!

Owner

laruence commented Mar 6, 2013

only for master , or 5.5+? sorry I didn't see that from the RFC's vote

Contributor

keyurdg commented Mar 6, 2013

@laruence Into 5.5

Owner

laruence commented Mar 6, 2013

but your pull request is based on master?

Contributor

keyurdg commented Mar 6, 2013

@laruence I assumed master was 5.5. So I should rebase this PR to the 5.5 branch?

Owner

laruence commented Mar 6, 2013

no, 5.5 is 5.5 branch. so I think yes, you should. thanks :)

Contributor

keyurdg commented Mar 7, 2013

Pull request #300 opened on 5.5. Closing this.

@keyurdg keyurdg closed this Mar 7, 2013

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