[PHP] fix PHP build system #9571

zeriyoshi · 2022-03-03T04:41:52Z

protobuf is not compatible with the PHP core build system. If you try to build PHP built-in protobuf, you will get the following error

$ docker run --rm -it alpine:latest
# apk add --no-cache ca-certificates curl tar xz openssl git curl autoconf automake libtool make g++ unzip
# mkdir "/php"
# cd "/php"
# curl -fsSL "https://www.php.net/distributions/php-8.0.16.tar.xz" | tar Jxvf - --strip-components=1
# git clone --depth=1 --branch=v3.19.4 --recursive "https://github.com/protocolbuffers/protobuf" "/protobuf"
# cd "/protobuf"
# ./autogen.sh
# ./configure
# make -j$(nproc)
# cp -R "./php/ext/google/protobuf" "/php/ext"
# cd "/php"
# ./buildconf --force
# ./configure --disable-all --enable-cli --enable-protobuf
# make -j$(nproc)
main/internal_functions_cli.c:40:2: error: 'phpext_protobuf_ptr' undeclared here (not in a function); did you mean 'phpext_date_ptr'?
   40 |  phpext_protobuf_ptr,
      |  ^~~~~~~~~~~~~~~~~~~
      |  phpext_date_ptr

This PR will be minimally supported and adapted to the PHP build system.

elharo · 2022-03-03T18:22:46Z

php/ext/google/protobuf/php_protobuf.h

@@ -0,0 +1,40 @@
+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 Google Inc.  All rights reserved.


// Copyright 2022 Google LLC

is the new form we use here

fixed thanks

haberman · 2022-03-03T18:30:27Z

This PR will be minimally supported and adapted to the PHP build system.

Could you explain more what you mean by this?

I'm reluctant to add something to our repo that is only minimally supported. It seems like the kind of thing that could work differently for different PHP versions, and would need more tweaking in the future.

Do other third-party PHP extensions support this? Can you point to an example?

Since it's such a small amount of code, I'm inclined to think it would be better to patch via a .patch file instead.

zeriyoshi · 2022-03-04T05:24:14Z

@haberman
PHP supports building extensions into the binary. However, it requires the appropriate header files.
https://www.php.net/manual/en/install.pecl.static.php

This PR adds that header file.

Normally, header files would include declarations such as MINIT, but we did not add them because they are not needed in this case.

See the PHP extension skeleton for more information:
https://github.com/php/php-src/tree/master/ext/skeleton

You may be right that patches are better.

zeriyoshi · 2022-03-06T09:12:32Z

@acozzette I fixed EXTRA_DIST, please readd kokoro:run

haberman · 2022-03-06T17:05:27Z

Can you point to any other popular PHP extensions that are not distributed with PHP but include this zend_module_entry line to allow for in-tree builds?

haberman · 2022-03-06T17:08:02Z

php/ext/google/protobuf/php_protobuf.h

+#  include "config.h"
+# endif
+
+extern zend_module_entry protobuf_module_entry;


Will this file get included by PHP directly? Is that why it needs to be its own file?

Is the name of this file significant? Does it need to be named php_protobuf.h?

No, not included. This file always exists only in the protobuf repository.

Names are important. The PHP build system automatically references header files according to the naming conventions.

haberman · 2022-03-06T17:08:34Z

php/ext/google/protobuf/protobuf.c

@@ -33,6 +33,8 @@
 #include <php.h>
 #include <Zend/zend_interfaces.h>

+#include "php_protobuf.h"


Why does this need to be included here? Will one of the other macros reference phpext_protobuf_ptr?

Oops, Deleted. Thanks

haberman · 2022-03-06T17:09:51Z

php/ext/google/protobuf/php_protobuf.h

+# endif
+
+extern zend_module_entry protobuf_module_entry;
+# define phpext_protobuf_ptr &protobuf_module_entry


Does this work across a wide variety of PHP versions, or will it need to be changed from version to version?

This mechanism has been around for a long time and should work with all supported PHP versions.

haberman · 2022-03-06T17:12:51Z

php/ext/google/protobuf/php_protobuf.h

+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


Please add a note that this file exists only for the purpose of in-tree builds, and is unused by normal builds of protobuf. Also it is user-supported, and we do not have tests for in-tree builds.

fixed, thanks

zeriyoshi · 2022-03-07T09:36:10Z

Can you point to any other popular PHP extensions that are not distributed with PHP but include this zend_module_entry line to allow for in-tree builds?

Most PECL extensions meet that requirement.
I think it's a good thing that in-tree builds are available, don't you?

zeriyoshi · 2022-03-07T15:52:40Z

@acozzette sorry, i fixed Makefile, please kokoro:run again.

This reverts commit e462966.

zeriyoshi · 2022-03-08T02:43:55Z

@acozzette
Re-fixing and rebased, please add kokoro:run again, really sorry.

zeriyoshi · 2022-03-08T04:47:03Z

@acozzette
It appears that the mirror acquisition has failed.
I don't know the detailed behavior of kokoro, but how can I solve this problem?

+ docker run --rm -e 'BUILD_COMMANDS=build_wheel protobuf/python' -e PYTHON_VERSION=3.6 -e MB_PYTHON_VERSION=3.6 -e UNICODE_WIDTH=32 -e BUILD_COMMIT=e5f37e5bddefe04829f4c80edb4d19cc2690647a -e CONFIG_PATH= -e ENV_VARS_PATH= -e WHEEL_SDIR=wheelhouse -e MANYLINUX_URL= -e BUILD_DEPENDS= -e USE_CCACHE= -e REPO_DIR=protobuf/python -e PLAT=x86_64 -e MB_ML_VER=1 -v /tmpfs/src/github/protobuf:/io -v /home/kbuilder:/parent-home quay.io/pypa/manylinux1_x86_64 /io/multibuild/docker_build_wrap.sh
/usr/bin/python
http://archive.kernel.org/centos-vault/5.11/os/x86_64/repodata/primary.xml.gz: [Errno 12] Timeout: <urlopen error timed out>
Trying other mirror.
http://archive.kernel.org/centos-vault/5.11/os/x86_64/repodata/primary.xml.gz: [Errno 7] HTTP Exception (BadStatusLine):
Trying other mirror.
Error: failure: repodata/primary.xml.gz from base: [Errno 256] No more mirrors to try.


[ID: 9921750] Build finished after 104 secs, exit value: 1```

acozzette · 2022-03-08T16:05:18Z

@zeriyoshi I reran the test and it passed, so I think it must have been a temporary network timeout.

acozzette added php release notes: yes labels Mar 3, 2022

acozzette requested a review from haberman March 3, 2022 17:19

protobuf-kokoro removed the kokoro:run label Mar 3, 2022

elharo reviewed Mar 3, 2022

View reviewed changes

acozzette added the wait for user action label Mar 3, 2022

acozzette added kokoro:run and removed wait for user action labels Mar 4, 2022

protobuf-kokoro removed the kokoro:run label Mar 4, 2022

zeriyoshi force-pushed the master branch from d9ae366 to 1a540f2 Compare March 6, 2022 09:11

acozzette added the kokoro:run label Mar 6, 2022

protobuf-kokoro removed the kokoro:run label Mar 6, 2022

haberman reviewed Mar 6, 2022

View reviewed changes

acozzette added the kokoro:run label Mar 7, 2022

protobuf-kokoro removed the kokoro:run label Mar 7, 2022

acozzette added the kokoro:run label Mar 7, 2022

protobuf-kokoro removed the kokoro:run label Mar 7, 2022

zeriyoshi and others added 3 commits March 8, 2022 11:42

[PHP] fix PHP extension build system

ed2b1fb

fix year

8ec090b

fix EXTRA_DIST

bb6f15c

zeriyoshi and others added 5 commits March 8, 2022 11:42

remove include header

0bde133

add comment

75269d0

Revert "add comment"

cace7d9

This reverts commit e462966.

add comment

3e8dadf

fix dangling commit

082f8db

zeriyoshi force-pushed the master branch from 174a7db to 082f8db Compare March 8, 2022 02:43

acozzette added the kokoro:run label Mar 8, 2022

protobuf-kokoro removed the kokoro:run label Mar 8, 2022

haberman merged commit 1722b16 into protocolbuffers:master Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHP] fix PHP build system #9571

[PHP] fix PHP build system #9571

zeriyoshi commented Mar 3, 2022 •

edited

Loading

elharo Mar 3, 2022

zeriyoshi Mar 4, 2022

haberman commented Mar 3, 2022

zeriyoshi commented Mar 4, 2022 •

edited

Loading

zeriyoshi commented Mar 6, 2022

haberman commented Mar 6, 2022

haberman Mar 6, 2022

zeriyoshi Mar 7, 2022

haberman Mar 6, 2022

zeriyoshi Mar 7, 2022

haberman Mar 6, 2022

zeriyoshi Mar 7, 2022

haberman Mar 6, 2022

zeriyoshi Mar 7, 2022

zeriyoshi commented Mar 7, 2022

zeriyoshi commented Mar 7, 2022

zeriyoshi commented Mar 8, 2022

zeriyoshi commented Mar 8, 2022

acozzette commented Mar 8, 2022

		@@ -0,0 +1,40 @@
		// Protocol Buffers - Google's data interchange format
		// Copyright 2008 Google Inc. All rights reserved.

[PHP] fix PHP build system #9571

[PHP] fix PHP build system #9571

Conversation

zeriyoshi commented Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberman commented Mar 3, 2022

zeriyoshi commented Mar 4, 2022 • edited Loading

zeriyoshi commented Mar 6, 2022

haberman commented Mar 6, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeriyoshi commented Mar 7, 2022

zeriyoshi commented Mar 7, 2022

zeriyoshi commented Mar 8, 2022

zeriyoshi commented Mar 8, 2022

acozzette commented Mar 8, 2022

zeriyoshi commented Mar 3, 2022 •

edited

Loading

zeriyoshi commented Mar 4, 2022 •

edited

Loading