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

Introduced initial ability to generate zend_module_deps #1900

Merged
merged 12 commits into from
Aug 5, 2019

Conversation

sergeyklay
Copy link
Contributor

@sergeyklay sergeyklay commented Aug 4, 2019

Hello!

In raising this pull request, I confirm the following:

  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I updated the CHANGELOG

Small description of change:

Introduced initial ability to generate zend_module_deps by using requires from project configuration (config.json).

To do so, add to your config.json configuration like this:

{
    "requires": {
        "extensions": [
            "module1",
            "module2",
            "module3"
        ]
    }
}

With such a config section Zephir will generate C-code similar to follows:

zend_function_entry php_test1_functions[] = {
	ZEND_FE_END

};
+
+static const zend_module_dep php_test1_deps[] = {
+	ZEND_MOD_REQUIRED("module1")
+	ZEND_MOD_REQUIRED("module2")
+	ZEND_MOD_REQUIRED("module3")
+	ZEND_MOD_END
+};

zend_module_entry test1_module_entry = {
	STANDARD_MODULE_HEADER_EX,
	NULL,
-	NULL,
+	php_test1_deps,
	PHP_TEST1_EXTNAME,
	php_test1_functions,
	PHP_MINIT(test1),
#ifndef ZEPHIR_RELEASE
	PHP_MSHUTDOWN(test1),
#else
	NULL,
#endif
	PHP_RINIT(test1),
	PHP_RSHUTDOWN(test1),
	PHP_MINFO(test1),
	PHP_TEST1_VERSION,
	ZEND_MODULE_GLOBALS(test1),
	PHP_GINIT(test1),
	PHP_GSHUTDOWN(test1),
#ifdef ZEPHIR_POST_REQUEST
	PHP_PRSHUTDOWN(test1),
#else
	NULL,
#endif
	STANDARD_MODULE_PROPERTIES_EX
};

From documentation:

The system checks for the eventually attached zend_module_deps to the zend_module_entry. This structure basically allows a PHP extension to declare extensions it depends on, and dependencies have types :

  • A PHP extension can tell the system that it is not compatible with another already registered extension, so it will fail loading itself into the engine
  • A PHP extension can tell the system that it requires another PHP extension to be loaded before itself

Right now we're going to use the second option.

Here, only conflicts are checked, and as there is no particular registration order, a PHP extension A could declare being in conflict with B, but B could be loaded after A, hence the check won't work and you'll probably get some trouble. When dealing with conflicts, it is better to sort the extensions in the php.ini file as a FIFO. PHP extensions will be registered in the exact same order they've been declared in php.ini.

Thanks

Library/Compiler.php Outdated Show resolved Hide resolved
@ruudboon
Copy link
Contributor

ruudboon commented Aug 4, 2019

Is it possible to add the extension version?

@sergeyklay
Copy link
Contributor Author

@ruudboon Yes, but I would prefer to refrain from this at this stage. I'll think about a better implementation a bit later.

@codecov
Copy link

codecov bot commented Aug 4, 2019

Codecov Report

Merging #1900 into development will decrease coverage by 0.35%.
The diff coverage is 30.37%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1900      +/-   ##
=================================================
- Coverage          33.93%   33.57%   -0.36%     
- Complexity          8233     8236       +3     
=================================================
  Files                560      559       -1     
  Lines              44694    44463     -231     
=================================================
- Hits               15166    14930     -236     
- Misses             29528    29533       +5
Impacted Files Coverage Δ Complexity Δ
ext/test/oo/oodestruct.zep.c 3.25% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/spectralnorm.zep.c 1.22% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/trytest.zep.c 7.28% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/optimizers/strreplace.zep.c 17.33% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/resourcetest.zep.c 79.31% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/range.zep.c 9.67% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/quantum.zep.c 1.79% <0%> (ø) 0 <0> (ø) ⬇️
ext/test/sort.zep.c 100% <100%> (ø) 0 <0> (ø) ⬇️
ext/test/router.zep.c 31.71% <100%> (ø) 0 <0> (ø) ⬇️
ext/test/scall.zep.c 72.86% <100%> (ø) 0 <0> (ø) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b9a272...85472af. Read the comment docs.

@sergeyklay sergeyklay merged commit 2dc87e0 into development Aug 5, 2019
@sergeyklay sergeyklay deleted the feature/zend-module-deps branch August 5, 2019 08:43
@danhunsaker
Copy link
Contributor

Looks good. I would definitely prefer to support "conflicts with" values as well, and to be able to list version numbers in both sections. But as a starting point, this is still really helpful!

dreamsxin pushed a commit to dreamsxin/zephir that referenced this pull request Nov 6, 2019
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.

4 participants