PHP : Dispatch available descriptor specs in js_open_process function #963

mho22 · 2024-01-20T17:45:45Z

What is this PR doing?

Based on the issue #930

For now, if you have this PHP code :

$process = proc_open( 'stty', [ 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes );

You get an error because of the lack of 0 descriptor in $descriptorspec parameter.

This PR is giving the opportunity to proc_open() users to add more, less or exactly 3 descriptors.

What problem is it solving?

It solves the problem of inevitably adding 3 descriptors, even if only 2 sometimes are enough.

Testing Instructions

proc.php

<?php

$command = 'ls -a';

$descriptorspec = [
    0 => [ 'pipe', 'r' ],
    1 => [ 'pipe', 'w' ],
    //2 => [ 'pipe', 'w' ],
    //3 => [ 'pipe', 'w' ],
    //4 => [ 'pipe', 'w' ]
];

$process = proc_open( $command, $descriptorspec, $pipes );

var_dump( stream_get_contents( $pipes[ 1 ] ) );

cli.js

import { NodePHP } from '@php-wasm/node';
import { spawn } from 'child_process';


const php = await NodePHP.load( '8.2' );


php.setSpawnHandler( ( command ) => spawn( command, [], { shell : true, stdio : [ 'pipe', 'pipe', 'pipe' ], timeout : undefined } ) );


php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) );

node cli.js proc.php

returns [ in any case with more than 1 descriptor ] :

string(61) "cli.js
node_modules
package-lock.json
package.json
proc.php
"

Next steps if validated :

Compile:all
Remove phpversion 8.2 in actual tests
Test:all

@adamziel New in php.spec.ts :

'popen("cat", "w")' test works now.
'popen("cat", "w")' and 'echo – stdin=file (empty), stdout=pipe, stderr=pipe'seem to still need sleep(1) while it is not needed when using php.

…open

adamziel · 2024-01-22T13:57:56Z

packages/php-wasm/node/src/test/php.spec.ts


-			expect(result.text).toEqual('stdout: WordPress\n');
-		});
+                    sleep(1);


Good spot, I just ran into the same thing, this test passes randomly if cat > out finishes before this point.

adamziel · 2024-01-22T13:58:34Z

packages/php-wasm/node/src/test/php.spec.ts

-				proc_close($res);
-			`,
+
+			it('Uses only stdin and stdout descriptor specs', async () => {


Nice! Thank you for adding test coverage

adamziel · 2024-01-22T13:59:58Z

packages/php-wasm/compile/php/php_wasm.c

-			stderr_pipe[0],
-			stderr_pipe[1]
+			cmd,
+            descv,


Nice! TIL you can pass arrays like this.

Edit: A-ha, you still decode that on the JS side by looping over the memory. Gotcha!

Thinking about this more, this seems to add quite a few lines of code and some indirection that needs to be mentally processed when reading the code. What do you think about preserving the long js_open_process signature and, whenever a descriptor is missing, passing NULLs directly as function args?

Yes! What is passed is the array pointer in C, and I had to loop over the memory to find all the data in JS.

In the php method, you can add more or less than 3 descriptors. If you want to have the same behavior, this will need a dynamic array. Keeping the old way will prevent adding new descriptors, but you're the boss. :).

A-ha! I missed the fact PHP actually uses those additional descriptors. Indeed this is what the documentation says:

The file descriptor numbers are not limited to 0, 1 and 2 - you may specify any valid file descriptor number and it will be passed to the child process. This allows your script to interoperate with other scripts that run as "co-processes". In particular, this is useful for passing passphrases to programs like PGP, GPG and openssl in a more secure manner. It is also useful for reading status information provided by those programs on auxiliary file descriptors.

This is really a good spot, thank you! Let's use the array, then. I just checked, and this can actually be handled by Playground as Node.js child_process.spawn() supports arbitrary file descriptors via the stdio option.

adamziel · 2024-01-22T14:04:27Z

packages/php-wasm/compile/php/php_wasm.c

@@ -90,8 +91,10 @@ EMSCRIPTEN_KEEPALIVE FILE *wasm_popen(const char *cmd, const char *mode)
 		int current_procopen_call_id = ++procopen_call_id;
 		char *device_path = js_create_input_device(current_procopen_call_id);
 		int stdin_childend = current_procopen_call_id;
+        int stdin_parentend = open(device_path, O_WRONLY);


Interesting! What does this line change?

I actually add a child and parent in every descriptors found. No matter which one it is. So I had to find a parent end for the stdin in wasm_popen to use the same parameters for js_open_process. And when I analyzed the proc_open7.4.c file I found out that for the stdin parentend it was that. You can find this in proc_open7.4.c line 489. I copy pasted. I dont think the parentend is used in the js_open_process but it allows the function to work.

Gotcha! It seems like leaning on the multi-arg signature would remove the need for adding this line?

Actually, could the open() call be avoided by assigning NULL instead of stdin_parentend?

Yep ! I tried it right now and it works without that unused stdin_parentend. I will remove that part.

adamziel · 2024-01-22T14:05:44Z

packages/php-wasm/compile/php/phpwasm-emscripten-library.js

@@ -270,6 +260,20 @@ const LibraryExample = {
 			return 0;
 		}

+		if (descriptorsLength < 2) {


This could use a comment to explain why short-circuit in this case. Is this what native PHP does? Or is this a limitation of this implementation?

Oh no, I added this condition because I thought it had no sense to add only one descriptor. Sorry, subjective thought. What I know too is that when you add only one element in a dynamic array, the array has to add 16 to find the first element in memory. Let me give you an explanation about this : [ elements added in array -> pointer offset ] :

1 -> 16
2 -> 16
3 -> 16
4 -> 24
5 -> 24
6 -> 32
7 -> 32
8 -> 48
9 -> 48
...

This means you can understand how to find the correct data in the memory even if it is dynamic. But there is a minimum of 16. And that if( descriptorsLength < 2 ) allows to avoid 'if one element then + 16 instead of 8.

I hope I explained this well.

Oh no, I added this condition because I thought it had no sense to add only one descriptor. Sorry, subjective thought.

All good! I just checked and it seems like you can even pass zero descriptors and proc_open will still work:

$pipes = []; proc_open('echo "test123" > out', [], $pipes);

adamziel · 2024-01-22T14:11:13Z

This is awesome @mho22, thank you for working on this!

I left a few notes, mostly about the code style. It seems like preserving the multi-arg signature would make this PR much shorter. Otherwise, I think this PR makes a great addition to the codebase!

A quick note – I'd like to merge #957 today. It fixes some more sleep(1) cases, but will likely cause merge conflicts with this PR. I'm happy to help with rebasing once other kinks are ironed out.

mho22 · 2024-01-22T14:26:07Z

Hi @adamziel, I answered to your comments the best I could. Sorry if it is not clear. IMO, I think keeping the dynamic descriptors array would enable people to add other descriptors, for the future. But you decide, of course.

I actually also use a dynamic array in the args pull request #944.
This seems to be mandatory for that part. I suppose if it is valid for the args PR, this can be valid for this one ?

Don't hesitate to merge #957 and I will try to correct the merge conflicts in the two PRs.

adamziel · 2024-01-22T14:47:08Z

@mho22 Yeah I think you're right about using an array of descriptors as I mentioned in one of the discussion threads 👍

mho22 · 2024-01-22T18:27:05Z

@adamziel I encounter some problems with the new trunk, when initiating my 'custom' cli :

cli.js

const php = await NodePHP.load( '8.2' );


php.useHostFilesystem();


php.setSpawnHandler( cmd => spawn( cmd, [], { shell : true, stdio : [ 'pipe', 'pipe', 'pipe' ], timeout : undefined } ) );


const args = process.argv.slice( 2 );


php.cli( [ 'php', ...args ] ).catch( result => { throw result; } ).finally( () => setTimeout( process.exit( 0 ), 100 ) );

I use it to test custom php files outside the main repository.

here is the error :

failed to asynchronously prepare wasm: LinkError: WebAssembly.instantiate(): Import #109 module="a" function="bb": function import requires a callable
[LinkError: WebAssembly.instantiate(): Import #109 module="a" function="bb": function import requires a callable]
Aborted(LinkError: WebAssembly.instantiate(): Import #109 module="a" function="bb": function import requires a callable)
node:internal/process/esm_loader:34
     internalBinding('errors').triggerUncaughtException(
                               ^

[LinkError: WebAssembly.instantiate(): Import #109 module="a" function="bb": function import requires a callable]

Node.js v21.6.0

It appears when const php = await NodePHP.load( '8.2' );

Am I doing something wrong now ? The process has changed with #957 ?

adamziel · 2024-01-23T15:41:41Z

@mho22 the process should be the same. This looks either like a mismatch between a php.js and php.wasm OR like a build issue. How can I reproduce that? Also, how are you running the command? Sometimes it helps to run nx reset as nx will cache all the files involved in a build process which tends to cause version mismatch issues.

mho22 · 2024-01-23T15:51:05Z

@adamziel Sorry, I made a new pull request and I thought you commented on the new one. I will investigate the problem later.

adamziel · 2024-01-23T15:52:24Z

Ah, all good, let's just continue in the new one. Should this one be closed, then?

mho22 · 2024-01-23T15:54:53Z

@adamziel If it is ok for you, we should close this one and the older one too.

…969) Based on the issue #930 ### Support for passing an array of strings as a `$command` when calling `proc_open` Without this PR, this `proc_open` call doesn't work: ``` $process = proc_open( [ 'ls', '-a' ], $descriptorspec, $pipes ); ``` With this PR, it does and the `php.setSpawnHandler( )` callback receives all the information from the `[ 'ls', '-a' ]` array. ### Support for an arbitrary number of file descriptors in `proc_open` Without this PR, this `proc_open` call will error out because, internally, PHP-WASM always expects three file descriptors: ``` $process = proc_open( 'stty', [ 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes ); ``` This PR removes that limitation so that `proc_open()` can work when 0 or more file descriptors are passed ## Testing Instructions ### Support for passing an array of strings as a `$command` when calling `proc_open` `proc.php` ``` $process = proc_open( [ 'ls', '-a' ], [ 0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes ); ``` `cli.js` ``` import { NodePHP } from '@php-wasm/node'; const php = await NodePHP.load( '8.2' ); php.setSpawnHandler( (cmd, args) => console.log( cmd, args ) ); php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) ); ``` running : `node cli.js proc.php` returns : ``` ls [ '-a' ] Node.js v20.11.0 ``` ### Support for an arbitrary number of file descriptors in `proc_open` `proc.php` ``` <?php $command = 'ls -a'; $descriptorspec = [ 0 => [ 'pipe', 'r' ], 1 => [ 'pipe', 'w' ] ]; $process = proc_open( $command, $descriptorspec, $pipes ); var_dump( stream_get_contents( $pipes[ 1 ] ) ); ``` `cli.js` ``` import { NodePHP } from '@php-wasm/node'; import { spawn } from 'child_process'; const php = await NodePHP.load( '8.2' ); php.setSpawnHandler( (command) => spawn( command, [], { shell : true, stdio : [ 'pipe', 'pipe', 'pipe' ], timeout : undefined } ) ); php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) ); ``` running : `node cli.js proc.php` returns : ``` string(61) "cli.js node_modules package-lock.json package.json proc.php " ``` ___ Cfr : - [PHP : Dispatch available descriptor specs in js_open_process function](#963) - [PHP : Give access to command arguments if array type is given in php ^7.4 proc_open function](#944) --------- Co-authored-by: Adam Zieliński <[email protected]>

MHO and others added 3 commits January 20, 2024 13:15

Dispatch available descrpitor specs in proc open

aadd426

Dispatch available descriptor specs in proc open

f3af333

Merge branch 'trunk' into dispatch-available-descriptor-spec-in-proc-…

db2edf7

…open

mho22 marked this pull request as ready for review January 20, 2024 17:46

MHO added 2 commits January 21, 2024 14:52

Dispatch available descriptor specs in proc open

1fe1aed

Dispatch available descriptor specs in proc open

2d1674c

adamziel reviewed Jan 22, 2024

View reviewed changes

mho22 mentioned this pull request Jan 23, 2024

PHP : Add args and descriptors dynamic arrays in proc open function #969

Merged

adamziel closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP : Dispatch available descriptor specs in js_open_process function #963

PHP : Dispatch available descriptor specs in js_open_process function #963

mho22 commented Jan 20, 2024 •

edited

Loading

adamziel Jan 22, 2024

adamziel Jan 22, 2024

adamziel Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024

adamziel Jan 22, 2024

adamziel Jan 22, 2024

mho22 Jan 22, 2024

adamziel Jan 22, 2024

adamziel Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024

mho22 Jan 22, 2024

adamziel Jan 22, 2024 •

edited

Loading

adamziel commented Jan 22, 2024

mho22 commented Jan 22, 2024

adamziel commented Jan 22, 2024

mho22 commented Jan 22, 2024 •

edited

Loading

adamziel commented Jan 23, 2024

mho22 commented Jan 23, 2024

adamziel commented Jan 23, 2024

mho22 commented Jan 23, 2024 •

edited

Loading

PHP : Dispatch available descriptor specs in js_open_process function #963

PHP : Dispatch available descriptor specs in js_open_process function #963

Conversation

mho22 commented Jan 20, 2024 • edited Loading

What is this PR doing?

What problem is it solving?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

adamziel Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

mho22 Jan 22, 2024 • 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

adamziel Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

mho22 Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

adamziel commented Jan 22, 2024

mho22 commented Jan 22, 2024

adamziel commented Jan 22, 2024

mho22 commented Jan 22, 2024 • edited Loading

adamziel commented Jan 23, 2024

mho22 commented Jan 23, 2024

adamziel commented Jan 23, 2024

mho22 commented Jan 23, 2024 • edited Loading

mho22 commented Jan 20, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

mho22 commented Jan 22, 2024 •

edited

Loading

mho22 commented Jan 23, 2024 •

edited

Loading