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

Fix test when session extension is shared #9643

Closed

Conversation

andypost
Copy link
Contributor

When extension is build as shared then testing fpm configuration gets no extensions from EXTENSIONS section

Actual order in which extensions are loaded is defined by distro-builders (often via numeric prefix 00_ 10_) and I see no good "grepable" way to figure order from .m4 and .w32 files (they need clean-up) only few hard dependencies are found using ZEND_MOD_REQUIRED (will provide PR later)

Ref #9523 (comment)

@cmb69
Copy link
Contributor

cmb69 commented Sep 30, 2022

Hmm, this looks somewhat hackish to me. Can't we just detect the extension_dir and all loaded extensions in Adoy\FastCGI\Client\Tester and set it there when starting the server? At the very least, we should use ini_get('extension_dir') instead of hard-coding the path.

@andypost
Copy link
Contributor Author

Thanks, will fix extension_dir as it could be inherited

But I see no way to get access to list of dependencies for test

@cmb69
Copy link
Contributor

cmb69 commented Sep 30, 2022

But I see no way to get access to list of dependencies for test

Since these should already be loaded, you could use get_loaded_extensions(), but applying that might be a fiddly.

When extension is build as shared then testing fpm configuration gets no extensions from EXTENSIONS section
@andypost andypost force-pushed the 77780-fix-shared-ext-session-test branch from 148a173 to d1a8988 Compare September 30, 2022 14:53
@andypost
Copy link
Contributor Author

@cmb69 There's TEST_PHP_EXTRA_ARGS env variable but it needs clean-up to be passed to fpm... maybe there's a way to get EXTENSIONS from env or by template from run-tests

@andypost
Copy link
Contributor Author

andypost commented Sep 30, 2022

Check via get_loaded_extensions() will not work because test has all extensions already and has no knowledge about it the extension shared or bundled (this runners are different SAPI)

@andypost
Copy link
Contributor Author

andypost commented Oct 1, 2022

Ah, that's not the same process, the test starts new fpm process and missed to provide proper config to it

should already be loaded

@andypost andypost changed the title Fix test when session extesion is shared Fix test when session extension is shared Oct 5, 2022
@andypost
Copy link
Contributor Author

Using this as patch for 8.1 and 8.2 in Alpinelinux hope to get it in

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be done differently. We could for example introduce a new environment variable and update CMD if there are set in a similar way like TEST_FPM_RUN_AS_ROOT. It should be enough to have it for extension dir (e.g. TEST_FPM_EXTENSION_DIR) and then have new parameter for required extension. If extension dir is set, it would be then provided as a list of additional parameters.

@@ -24,7 +24,7 @@ echo str_repeat('asdfghjkl', 150000) . "\n";
EOT;

$tester = new FPM\Tester($cfg, $code);
$tester->start();
$tester->start(['-dextension=session']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces warning if the session is statically compiled.

@@ -392,7 +392,7 @@ class Tester
$configFile = $this->createConfig();
$desc = $this->outDesc ? [] : [1 => array('pipe', 'w'), 2 => array('redirect', 1)];

$cmd = [self::findExecutable(), '-F', '-y', $configFile];
$cmd = [self::findExecutable(), '-F', '-y', $configFile, '-dextension_dir=modules'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just specific to your setup where your extension dir is in modules so it cannot be applied in general. For example it produces warning in my local setup as I use some shared extensions that are not obviously in this directory.

@bukka
Copy link
Member

bukka commented Oct 30, 2022

@andypost I though that it might be easier to just implement it as that description might have not been that clear. Here it is: #9854 . Not tested so please test it. :)

@andypost
Copy link
Contributor Author

andypost commented Nov 5, 2022

Closing in favour of #9854

@andypost andypost closed this Nov 5, 2022
@andypost andypost deleted the 77780-fix-shared-ext-session-test branch November 5, 2022 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants