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

feat: add support for libphp and the embed SAPI #153

Merged
merged 13 commits into from Sep 12, 2023

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Aug 29, 2023

This patch enables the creation of static libphp builds thanks to a new --with-embed flag.

libphp can be used to create static programs using the embed SAPI as well as programs using other SAPIs such as FrankenPHP (the reason why I'm working on this: dunglas/frankenphp#198), NGINX Unit and various other projects.

Downloads

Currently, I have not updated GitHub Actions workflows to build libphp, however, this could be interesting to distribute a pre-compiled version of libphp to speed up builds of FrankenPHP and of other projects depending on it. This will require to zip the buildroot/ directory, or at least all the .a libraries and the headers.

This can be done in a subsequent PR if relevant.

Known issue on macOS

The static library works perfectly well on Linux, but only a limited subset of extensions are currently supported on macOS. Basically, all extensions that depend on third-party static libraries (e.g. curl or pdo_sqlite) fail to be built properly.

Building libphp.a works but a weird linking error occurs when linking it with the final program (in my case, FrankenPHP):

ld: warning: ignoring file /static-php-cli/buildroot/lib/libphp.a, building for macOS-arm64 but attempting to link with file built for macOS-arm64

This may be related to the linker used to build .a libraries (the GNU linker must not be used) or to the use of the -mmacosx-version-min flag, but I didn't manage to find the exact reason yet. If anyone with Apple-specific knowledge has a better clue of what's going on, I would be happy to get some help!

@crazywhalecc crazywhalecc added enhancement New feature or request kind/php-src Issues related to php source need test This PR has not been tested yet, cannot merge now labels Aug 29, 2023
@crazywhalecc
Copy link
Owner

It's a huge update. I tried to support embed sapi before but failed as you said (but it was a long time ago).

Using this INSTALL_ROOT trick may be useful to build other SAPIs too. It would simplify the code and make static-php-cli more future-proof.

The reason why I don't use make install is actually very simple: to keep buildroot from including redundant files. But obviously it may not suitable for SAPI such as embed (a lot of header files).

For macOS, I'm also learning some system library and executable things.

@dunglas
Copy link
Contributor Author

dunglas commented Aug 29, 2023

I've found a hack to workaround the MacOS error:

./bin/spc build --with-embed "curl"
mkdir tmp-libphp-o
cd tmp-libphp-o
ar x ../buildroot/lib/libphp.a
ar rcs ../buildroot/lib/libphp.a *.o

Basically, I extract all .o files from the generated archive and then recreate it.

Then, tweaking the LDFLAGS environment variable like this allows building: -L/static-php-cli/buildroot/lib -lresolv -lnetwork -lm -lpthread -lcurl -lssl -lcrypto -framework SystemConfiguration -lz

I'm not sure of what's going on exactly, but I suspect that the libtool version bundled with PHP doesn't pass the appropriate flags to ar to build a static macOS archive.

We could include this hack in static-php-cli, but I'm not sure if it's a good idea. Maybe the PHP build system should be fixed instead.

@crazywhalecc
Copy link
Owner

crazywhalecc commented Aug 30, 2023

I think we can accept using some hacks to solve this first, and then submit the issue to other projects (if other projects are slower to update).

BTW, tweaking the LDFLAGS, LIBS variables before compiling, is a typical problem. Someone mentioned before that the modification here is to manually inject variable values through pkg-config. The intention of this is to solve the dependency problem of those libraries that cannot automatically use pkg-config, and reduce a lot of manual replacement of -lxx to path/ to/libxx.a file. I'm not sure if this bug has anything to do with this, while I didn't fully agree to do it at that time because we also need to additionally record the pkg-config file name for each dependency. #78 (e.g. LDFLAGS=$(pkg-config --libs-only-L --static xxx yyy))

@dunglas
Copy link
Contributor Author

dunglas commented Aug 30, 2023

Workaround implemented. The problem with -lz is not related (this is probably an issue in licurl config, it is added when adding other extensions).

This is ready to be merged on my side!

@crazywhalecc
Copy link
Owner

Thanks, I'll review and run it on my local machine.

Copy link
Owner

@crazywhalecc crazywhalecc left a comment

Choose a reason for hiding this comment

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

It works very well, I'm glad you can bring the new SAPI in.

By the way, I found that embed can also use hardcoded ini parameters: --with-hardcoded-ini or -I. (method in SourcePatcher::patchHardcodedINI)

src/SPC/builder/linux/LinuxBuilder.php Outdated Show resolved Hide resolved
src/SPC/builder/linux/LinuxBuilder.php Show resolved Hide resolved
src/SPC/builder/macos/MacOSBuilder.php Outdated Show resolved Hide resolved
@crazywhalecc
Copy link
Owner

crazywhalecc commented Aug 31, 2023

And there are some additional changes that need to be made:

  • embed SAPI documentation
  • embed in GitHub Action
  • test each extension and sapi that is currently supported in the list. If the embed mode is not supported, it needs to be listed and documented

If you have no idea or have no time to do, I can handle 1 and 2 after merging this pull request. 3 is not urgent now, sooner or later I will need to write some test extensions and SAPI scripts.

@crazywhalecc crazywhalecc removed the need test This PR has not been tested yet, cannot merge now label Aug 31, 2023
@dunglas
Copy link
Contributor Author

dunglas commented Aug 31, 2023

Thanks for your quick review (and for this awesome project).

I'm on vacation for 10 days, but I can work on both items when I'll be back (any help is also welcome of course!).

embed SAPI documentation

I started something here that can probably be adapted for inclusion in this project: https://github.com/dunglas/frankenphp/pull/198/files#diff-8affca5993d9a57b7337aa153dfbae8c6b8b2ffe6c01e1954415ffdd254e2a6bR1

embed in GitHub Action

That would be awesome and would allow us to improve the build times of FrankenPHP. Currently, it takes 1h of machine time to build libphp for Linux and Mac, but this could be done only 1 time in this project: https://github.com/dunglas/frankenphp/actions/runs/6030196478/
I'm not sure exactly how your GHA setup works, so guidance or help on this would be very much appreciated :)

What could be awesome is to be able to build automatically the latest version of libphp for popular OS and architectures each time a new version of PHP is tagged, or when a dependency has a security issue.

test each extension and sapi that is currently supported in the list. If the embed mode is not supported, it needs to be listed and documented

This is touchy because libphp is used by the embed SAPI but also by other SAPIs. Some extensions may not work for embed, but work with other SAPIs. For instance, FrankenPHP and NGINX Unit provide their own SAPI that are supported by opcache (https://github.com/php/php-src/blob/dda6b8f682e6d81dce8f6ef37dd8351c85807a5c/ext/opcache/ZendAccelerator.c#L2819-L2820), and I tested that it works with this patch. However, embed itself isn't supported by opcache (on purpose, having compiled causes no issue but it isn't used).

These extensions seem to work well: https://github.com/dunglas/frankenphp/pull/198/files#diff-03075e9917c9760de11a71ccf816416084b36ee75c60f595d865da73619a3258R6
At least, libphp compiles and the FrankenPHP test suite (which is quite extensive) pass. The only issue we had so far under high load is with apcu: krakjoe/apcu#493 (but this issue is very unlikely to happen in prod as it only occurs when displaying phpinfo().

Maybe could we add some automated tests to check extension support (this could also be done for CLI, FPM etc.)?

@crazywhalecc
Copy link
Owner

crazywhalecc commented Sep 1, 2023

Maybe could we add some automated tests to check extension support (this could also be done for CLI, FPM etc.)?

Yeah, that's what I thought. So far I've written several tests:

@crazywhalecc
Copy link
Owner

crazywhalecc commented Sep 1, 2023

I'm not sure exactly how your GHA setup works, so guidance or help on this would be very much appreciated :)

I'm also writing documentation in static-php-cli-docs, including contributing guide.

Actually, I only wrote Actions that need manually trigger compilation. I have no plan to build it automatically, because people need different extensions, not about security reason. (but I think we can change it. After all, security issues are also very important)

And for my self-hosted server artifacts, I also trigger it manually (because my server has a strict whitelist and firewall). My self-hosted server artifacts are just example files and typical out-of-the-box files, I prefer people to use static-php-cli to manually build the binaries they need.

@crazywhalecc
Copy link
Owner

crazywhalecc commented Sep 5, 2023

How can I easily check if libphp.a is working as intended?

@dunglas
Copy link
Contributor Author

dunglas commented Sep 5, 2023

The easiest way is probably to create a sample program using the embed SAPI like this one: https://github.com/php/php-src/tree/master/sapi/embed#basic-example

Alternatively, linking FrankenPHP with libphp.a works well too.

@jingjingxyk
Copy link
Contributor

666

@crazywhalecc crazywhalecc added the need test This PR has not been tested yet, cannot merge now label Sep 7, 2023
@crazywhalecc
Copy link
Owner

crazywhalecc commented Sep 10, 2023

During the testing process, I found that if I want to quickly obtain the compilation parameters for using libphp, it is best to use pkg-config. For libraries that will generate pc files after compilation, they can be recorded in lib.json. For libraries that do not use pkg-config, we may need to manually record the .a file and headers of the corresponding library.

What do you think about add the pkgconf file of each library and writing an output function that can directly obtain the LIBS parameters of the corresponding library?

@dunglas
Copy link
Contributor Author

dunglas commented Sep 11, 2023

@crazywhalecc using php-config seems to work too: https://github.com/dunglas/frankenphp/pull/198/files#diff-03075e9917c9760de11a71ccf816416084b36ee75c60f595d865da73619a3258R67-R68

That being said, one way or another it would be great to be able to retrieve the list of libs to pass to the compiler and linker for using static-php-cli in scripts!

@dunglas
Copy link
Contributor Author

dunglas commented Sep 12, 2023

I had to disable the Opcache JIT because linking breaks on Linux when enabled:

 > [stage-0 15/16] RUN cd caddy/frankenphp &&     CGO_CFLAGS="$(/static-php-cli/buildroot/bin/php-config --includes | sed s#-I/#-I/static-php-cli/buildroot/#g)"     CGO_LDFLAGS="$(/static-php-cli/buildroot/bin/php-config --ldflags) $(/static-php-cli/buildroot/bin/php-config --libs | sed -e 's/-lgcc_s//g')"     go build -buildmode=pie -tags "cgo netgo osusergo static_build" -ldflags "-linkmode=external -extldflags -static-pie -s -w -X 'github.com/caddyserver/caddy/v2.CustomVersion=FrankenPHP 198/merge Caddy'":
71.26 # github.com/dunglas/frankenphp/caddy/frankenphp
71.26 /usr/local/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
71.26 /usr/lib/gcc/x86_64-alpine-linux-musl/12.2.1/../../../../x86_64-alpine-linux-musl/bin/ld: /static-php-cli/buildroot/lib/pkgconfig/../../lib/libphp.a(zend_jit.o): TLS transition from R_X86_64_TLSGD to R_X86_64_GOTTPOFF against `_tsrm_ls_cache' at 0x74b16 in section `.text' failed
71.26 /usr/lib/gcc/x86_64-alpine-linux-musl/12.2.1/../../../../x86_64-alpine-linux-musl/bin/ld: failed to set dynamic section sizes: bad value
71.26 collect2: error: ld returned 1 exit status

Everything is green now, and I'm able to build static builds of FrankenPHP compiled with the following extensions: bcmath,calendar,ctype,curl,dba,dom,exif,filter,fileinfo,gd,iconv,mbstring,mbregex,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_pgsql,pdo_sqlite,pgsql,phar,posix,readline,redis,session,simplexml,sockets,sqlite3,tokenizer,xml,xmlreader,xmlwriter,zip,zlib,apcu

Do you think we can merge this PR @crazywhalecc? We'll be able to iterate on the other topics in subsequent PRs.

@crazywhalecc
Copy link
Owner

Yeah of course. I'm working on exporting the libs and cflags, in order to make a easier sanity check for libphp.a. But I think it could be done later.

@crazywhalecc crazywhalecc removed the need test This PR has not been tested yet, cannot merge now label Sep 12, 2023
@crazywhalecc crazywhalecc merged commit 3183ecc into crazywhalecc:main Sep 12, 2023
4 checks passed
@crazywhalecc crazywhalecc mentioned this pull request Sep 12, 2023
8 tasks
@dunglas dunglas deleted the feat/embed branch September 12, 2023 15:50
@jingjingxyk jingjingxyk mentioned this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request kind/php-src Issues related to php source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants