Fix issues with phpt and EXTENSIONS block on windows (BUG 75042)

[jbboehr]

• Commands are not properly escaped for windows
• Specifying "-n" to check loaded modules causes "Module already loaded"
  warning
• Extensions to be loaded need the "php_" prefix on Windows

Bug: https://bugs.php.net/bug.php?id=75042

[weltling]

Are you sure this doesn't hurt the non windows side? Some shells could extrapolate double quotes and consequently complain about parentheses. IMO it would be safer to isolate the call by platform.

Thanks.

[jbboehr]

I mainly do development on Ubuntu, and this should work on that platform, since there's no variable expansion in the argument. I can't speak for any other platform. Initially, I tried to use escapeshellarg (which in theory would be the safest cross-platform solution), but according to the documentation:

On Windows, escapeshellarg() instead replaces percent signs, exclamation marks (delayed variable substitution) and double quotes with spaces and adds double quotes around the string.

Which doesn't work at all because the code then becomes: echo ini_get(extension_dir); and echo implode( , , get_loaded_extensions());

I will refactor it to a branch if you'd prefer.

[weltling]

@jbboehr you told in the ticket a repro is possible. Could you add a test for that please?

Thanks.

[jbboehr]

@weltling It's a little tricky because it requires one extension to be built shared. I'll see if I can figure out a way to do this without causing tests to fail if the extension isn't built shared.

[weltling]

@jbboehr there are several exts that are always built shared for Windows in the core, for instance ext/openssl. In general, the test can be adapted later, if the standard build changes. But a test is essential we see there are no regressions.

Thanks.

[jbboehr]

@weltling I have added three tests:

1. Tries to load openssl as a shared module. This test is skipped on the current TravisCI builds as openssl appears to be compiled statically. This is currently failing on Windows because the EXTENSIONS block is evaluated before before the SKIPIF block, so I can't get the test to skip if openssl is not available or is compiled statically, and nmake install has not been run and the the extension_dir ini directive is pointing to the target install location, so the openssl dll is not there yet.
2. Tries to load SPL even though it's compiled statically. Were SPL not always compiled statically, this would not behave as expected on some systems because get_loaded_extensions returns SPL in uppercase, however extension module filenames are typically lowercase.
3. Attempts to load an extension that does not exist and checks the error message.

If you have any advice on how to proceed with (1), it would be much appreciated, however I believe just tests (2) and (3) should be sufficient to test the functionality of the EXTENSIONS block.

[weltling]

@jbboehr the tests 2. and 3. are fine. Furthermore - SPL is never shared, so that's reliable. With the number 1 - i see a complication now. Namely, there are different constellations of shared/static in the standard build on Windows, based on specifics. It looks to me, that the way running php -n is less suitable nowadays for teh Windows part. It is true, that there's no nmake install on appveyor, but the reason is also that it's not needed. When nmake test is invoked, it uses a generated INI file and sets up the environment. That exact file should be used for the checks, but not directly. It looks like some completely different call is needed in this case, something like nmake run ARGS=-m. Or, of course, get_loaded_extensions() can be passed to the args inline like it is done now, to not to change the current mechanics. Not using this mechanism means default ini values are used, that's what you see on AppVeyor.

With the part about the evaluation order - so this likely needs to be fixed as well. If some item in EXTENSIONS is not available, the test should skip as it is missing a required prerequisite to run.

Thanks.

[jbboehr]

@weltling My latest commit attempts to pass in the same options to those two shell commands that are passed to the php files for the SKIPIF and FILE blocks, albeit minus the requested extensions since those haven't been resolved yet. This appears to cause test 1 to pass on windows (linux is still skipped because openssl is static).

[weltling]

@jbboehr yeah, that should work in most case, if run-tests.php is invoked within the makefile. Otherwise it'll need some extra config anyway. I need to test this part on Linux yet, should come to it next days.

What about the second part about skip if dynamic dependents are not available? Should be at least evaluated.

Thanks.

[jbboehr]

@weltling tests 2 and 3 should be running on linux.

I think it's fine for the skipping to be provided by the SKIPIF block, personally, although maybe something like this would work?

	// Additional required extensions
	if (array_key_exists('EXTENSIONS', $section_text)) {
		$ext_params = array();
		settings2array($ini_overwrites, $ext_params);
		settings2params($ext_params);
		$ext_dir=`$php $pass_options $ext_params -d display_errors=0 -r "echo ini_get('extension_dir');"`;
		$extensions = preg_split("/[\n\r]+/", trim($section_text['EXTENSIONS']));
		$loaded = explode(",", `$php $pass_options $ext_params -d display_errors=0 -r "echo implode(',', get_loaded_extensions());"`);
		$ext_prefix = substr(PHP_OS, 0, 3) === "WIN" ? "php_" : "";
		foreach ($extensions as $req_ext) {
			$req_ext_file = $ext_dir . DIRECTORY_SEPARATOR . $ext_prefix . $req_ext . '.' . PHP_SHLIB_SUFFIX;
			if (!file_exists($req_ext_file)) {
				$message = "ext/$req_ext required";
				show_result('SKIP', $tested, $tested_file, "reason: $message", $temp_filenames);
				junit_mark_test_as('SKIP', $shortname, $tested, null, $message);
				return 'SKIPPED';
			}
			if (!in_array($req_ext, $loaded)) {
				if ($req_ext == 'opcache') {
					$ini_settings['zend_extension'][] = $req_ext_file;
				} else {
					$ini_settings['extension'][] = $req_ext_file;
				}
			}
		}
	}

[weltling]

@jbboehr thinking a bit more about it, i'd go with you saying SKIPIF section is already does the job and is enough. While it would be a nice piece, it'd introduce a redundant handling which is not necessary good. With that, lets wait for possibly more comments and i still should come to the Linux test.

Thanks.

[weltling]

Merged as 59558ff, follow up f1c664d. Perhaps lower branches could be targeted, too, but the patch didn't apply there. @jbboehr if you are interested, it still could be backported, but imo not critical.

Thanks!

[jbboehr]

@weltling The resolution is simple enough for 7.0. Someone changed join -> implode:

https://github.com/php/php-src/compare/PHP-7.0...jbboehr:run-tests-windows-EXTENSIONS-7.0?expand=1

[maaarghk]

Hello and sorry to necro a 4 year old patch, but isn't this an undocumented (and I would suspect undesirable) behaviour change in the intended purpose of the --EXTENSIONS-- block in the phpt file?

According to this patch, $ini_overwrites is now being passed to the code which calculates $ext_dir. If you are building your own module, make test always passes -d extension_dir=$(top_builddir)/modules/ and -d extension=myextension.so. So now ext_dir is always calculated as being part of the build root, and any external shared extensions you require can't be found.

So it seems the behaviour, when using make-tests.php on your own module, has changed --

BEFORE:

• default behaviour of make test is to disable all shared extensions
• you use extensions section to list shared extensions which are required for your test to run
• the default shared extension path is loaded and the absolute path to the shared extension is added to the test argv
• your test now runs with the required shared extension loaded

AFTER:

• default behaviour of make test is to disable all shared extensions
• you use extensions section to list shared extensions which are required for your test to run
• the shared extension path that your Makefile specified is checked for those extensions using the generate temporary php.ini
• your test fails because the shared extension you required was not found

The "before" behaviour is essential when building an extension. For example I am currently writing a test where xdebug_dump_zval is required to verify that the C code is running properly, or I might write a test where mbstring to check that strings are being returned with valid encoding, whatever. So I add xdebug or mbstring to EXTENSIONS area expecting (according to all docs) that I will then have that extension loaded in my test.

The reason I suggest that this behaviour change is undesirable is it conflicts with the existing documentation - Here is where --EXTENSIONS-- is documented as being used to load extensions which aren't loaded by the test. Not only that, but it conflicts with the presumed behaviour by current maintainers - Here is where @MaxSem writes: "If an extension [listed in --EXTENSION--] is missing, PHP will try to find it in a shared module and skip the test if it's not there." That is technically true because it will indeed try, but it will always fail as of this PR being merged.

Funnily enough, there is another unintended side effect of this patch, which is that it enables you to specify the current extension as being required. That is, myextension.so ends up being added to loaded. This is what enables the changes made by @MaxSem to change SKIPIF to EXTENSIONS - before this patch you'd never see the current extension being loaded and it would always skip. I think that's technically also a bug, but at this point, it should be kept because it is compatible both with the main php source tree and also building extensions outside the php source tree.

Addressing both of these considerations, I've submitted a PR #6910 to revert the change to ext_dir but preserve the change to loaded, which should both fix the broken behaviour introduced in this patch whilst preserving the new "feature" that is now relied on.