add libacl for php-fpm compilation #611

henderkes · 2025-03-05T10:42:23Z

What does this PR do?

adds libacl for php-fpm compilation. php-fpm commonly uses listen.owner and listen.group, without acl, php-fpm will crash when encountering this config directive

Checklist before merging

add tests for library?
add MacOS compatibility. It should build without issues, but it might need a similar patch as LinuxBuilder adding -L/buildroot/libs
add BSD compatibility. It should build without issues, but it might need a similar patch as LinuxBuilder adding -L/buildroot/libs
doesn't exist on windows, so can skip that
instead of checking for the presence of the library, should we instead add a separate option for compilation with fpm?

henderkes · 2025-03-05T10:58:38Z

I'm not quite happy with the EXTRA_LDFLAGS_PROGRAM thing yet. The problem is that php's Makefile automatically adds -lacl at the end of a makefile when /path/to/buildroot/lib/libacl.a is already set in EXTRA_LIBS. I think it would be a better idea to patch that -lacl out of the command. Let me revert the changes.

How do we add a pathBeforeMake for a library that is not actually an extension?

henderkes · 2025-03-05T11:38:53Z

hmm, seems to not actually be an issue anymore after cleaning my env vars and downloading and building fresh.

No idea why the actions fail on intl now, given that the PR does not touch that

crazywhalecc · 2025-03-07T06:15:53Z

This seems to be the first time we encountered a non-extension dependency.

If it's an optional dependency, maybe making a fake extension like swoole-hook-mysql is a better choice? For example, fpm-acl

henderkes · 2025-03-07T07:54:17Z

I think that makes less sense than using --with-libs.

The real alternative I would see would be --build-fpm --with-fpm-acl. Bit then we'd need to add logic to conditionally build the library, rather than using the already existing getLib check.

src/SPC/builder/linux/LinuxBuilder.php

henderkes · 2025-03-07T09:19:15Z

Actually, thinking about it, is there a reason why we don't just default this behaviour? fpm with acl can do everything that fpm without acl can't do. If the directives aren't used, it makes no difference.

Maybe we should just default this lib for fpm builds?

crazywhalecc · 2025-03-07T09:58:50Z

Maybe we should just default this lib for fpm builds?

That's a good question. As you know, currently spc only depends on pkg-config by default on *nix. If it is enabled by default, it means that in addition to pkg-config, these two libraries must be downloaded.

If most distributions added and used this library, adding it as a default I think is feasible, but we should also add it to the list of precompiled libraries to speed up the build:

static-php-cli/config/source.json

Line 340 in 7febbc7

"provide-pre-built": false,

https://github.com/static-php/static-php-cli-hosted/blob/master/.github/workflows/pack-libs.yml

Also, with this change, I think we need to separate the way such libraries (pkg-config, libacl) are specified to support the continued addition of related libraries in the future. I'll think about where we should list them.

henderkes · 2025-03-07T10:27:05Z

If most distributions added and used this library, adding it as a default I think is feasible, but we should also add it to the list of precompiled libraries to speed up the build:

I doubt this will happen as SPC is still for static php, while all distros ship dynamic builds.

All php-fpm packages I know ship with acl, though. At least on Ubuntu, Debian and RHEL based distros. Not entirely sure about alpine, but alpine also has ACL as a utility.

Also, with this change, I think we need to separate the way such libraries (pkg-config, libacl) are specified to support the continued addition of related libraries in the future. I'll think about where we should list them.

Yes, I agree. Preferably the setting in source.json should be all that's needed to pre-build a lib and add them to the list of downloadable pre-built libs.

crazywhalecc

I've added root node for lib.json via #618 , and we can add fpm deps in php.

henderkes · 2025-03-10T05:08:13Z

Two questions:

How would we add fpm there?
Should we make libacl (--with-fpm-acl) the default?

crazywhalecc · 2025-03-10T05:16:48Z

We assume that all users want to build with fpm, we certainly need to download libacl every time, same as phpmicro.
I prefer to add acl by default and add it to lib-depends-unix of php, but the configure still keep if state whether this library is added, so that we can quickly build the result without libacl by modifying the config. Adding libacl in lib-depends-unix below looks good.

static-php-cli/config/lib.json

Line 13 in 36dc180

"lib-base"

{
    "php": {
        "type": "root",
        "source": "php-src",
        "lib-depends-windows": [
            "micro",
            "lib-base"
        ],
        "lib-depends-unix": [
            "micro",
            "lib-base",
            "libacl"
        ]
    }
}

henderkes · 2025-03-10T05:19:36Z

I like that idea. However, there's no php-fpm node, there's only the php node. If we add it there, it'll be required even for the CLI binary, which doesn't actually need it.

crazywhalecc · 2025-03-10T05:43:31Z

I like that idea. However, there's no php-fpm node, there's only the php node. If we add it there, it'll be required even for the CLI binary, which doesn't actually need it.

Since we currently have no way to pass the SAPI we want to build when using the download command, the simplest solution is to download them every time we build PHP on *nix.

Another solution is to use --with-fpm-acl, but this requires adding it to both download and build, and will increase command complexity.

Also, we can add some special deps patch, for example: building cli only will remove libacl. Or we add php-fpm and php-cli node first and building target SAPI will removes unused SAPI node from php node dynamically before compiling PHP to control which libraries will be built. But this requires modifying the dependency resolver.

We can also split the build command into build:cli, build:micro, etc., and then select different dependency nodes when building different targets, as you said, but in this way we cannot build multiple SAPIs at the same time and need refactor.

henderkes · 2025-03-10T05:52:34Z

Hmm, yeah I think you're right, it'd be fine to download and build libacl no matter what. It builds very fast so it shouldn't be much of an issue.

henderkes · 2025-03-10T08:26:44Z

Now this is getting interesting. the ubuntu runner on php 8.3 works, the one on php 8.4 doesn't work because it adds -lacl despite /path/to/buildroot/lib/libacl.a already being specified.

I had this issue on RHEL at first, but then it stopped happening and now I cannot reproduce it anymore. Might have to path before make.

crazywhalecc · 2025-03-10T08:35:33Z

Now this is getting interesting. the ubuntu runner on php 8.3 works, the one on php 8.4 doesn't work because it adds -lacl despite /path/to/buildroot/lib/libacl.a already being specified.

Seems 8.3 is using AC_CHECK_LIB(acl) while 8.4 is using AC_SEARCH_LIBS([acl_free], [acl]) in sapi/fpm/config.m4.

As for mac, just add -Wimplicit-function-declaration like pkg-config one:

$cflags = PHP_OS_FAMILY !== 'Linux' ? '-Wimplicit-function-declaration -Wno-int-conversion' : '';
$ldflags = !($this instanceof LinuxLibraryBase) || $this->builder->libc === 'glibc' ? '' : '--static';

shell()->cd($this->source_dir)
      ->setEnv(['CFLAGS' => "{$this->getLibExtraCFlags()} {$cflags}", 'LDFLAGS' => "{$this->getLibExtraLdFlags()} {$ldflags}", 'LIBS' => $this->getLibExtraLibs()])

henderkes · 2025-03-10T10:34:11Z

okay it works fine on alpine, rhel and ubuntu now.

I can't really work on macos with just runners as my reference, I think you'll have to take a look @crazywhalecc

henderkes · 2025-03-10T11:16:11Z

it looks like it's trying to use l-prefixed versions of the functions which aren't defined. Maybe patching opt_deref to true would fix the issue?

crazywhalecc · 2025-03-10T11:43:41Z

Oops, fpm-acl is more like a linux-only feature. I can't find a way to have fpm on a mac that has acl enabled. So did homebrew version. The acl and attr hosted by brew are also only for Linux and cannot be installed on MacOS.

https://github.com/php/php-src/blob/94f327219a5f85fc93f56ab72d1c940870ec4a0d/sapi/fpm/config.m4#L425

henderkes · 2025-03-10T11:49:32Z

That's strange, macos uses the posix permission model and supports acl. If you want I can disable it for mac os, but I don't understand why it wouldn't work.

Edit: oh the configure m4 you linked states that it doesn't even work on macos even if acl is installed.

Guess I'll remove it tomorrow.

crazywhalecc · 2025-03-10T12:20:16Z

More old information: php/php-src#8395

henderkes · 2025-03-10T12:24:33Z

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/acl.3.html

henderkes · 2025-03-10T12:31:44Z

The official macos documentation about acl contradicts the config.m4 comment. The code would compile and link fine on macos.

crazywhalecc · 2025-03-10T12:38:48Z

I tried this manually, and got undefined ACL_USER, and I cannot find any header file containing this identifier.

#include <sys/acl.h>
int main (void)
{
        acl_t acl;
        acl_entry_t user, group;
        acl = acl_init(1);
        acl_create_entry(&acl, &user);
        acl_set_tag_type(user, ACL_USER);
        acl_create_entry(&acl, &group);
        acl_set_tag_type(user, ACL_GROUP);
        acl_free(acl);
  ;
  return 0;
}

$ clang a.c -I/Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk/usr/include
a.c:9:32: error: use of undeclared identifier 'ACL_USER'
    9 |         acl_set_tag_type(user, ACL_USER);
      |                                ^
a.c:11:32: error: use of undeclared identifier 'ACL_GROUP'
   11 |         acl_set_tag_type(user, ACL_GROUP);
      |                                ^
2 errors generated.

This seems to verify that macOS does not support the user/group feature. If PHP think it does not have this feature, it is considered that acl is not supported.

henderkes · 2025-03-10T12:41:35Z

Okay, I'll remove it from MacOS then. We may need to split up lib-depends-unix to lib-depends-linux and lib-depends-macos x)

I'll do it tomorrow, though. It's 8pm already

Copilot

PR Overview

This pull request updates the build configuration to support the addition of libacl for PHP-FPM compilation by adjusting the PHP version used in Unix builds.

Updated the default PHP version from 8.3 to 8.4 in the GitHub Actions workflow
Aligns build configuration with the requirements of the new libacl dependency

Reviewed Changes

File	Description
.github/workflows/build-unix.yml	Updated default PHP version to 8.4 for Unix builds

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

.github/workflows/build-unix.yml:18

The default PHP version was updated to 8.4. Please confirm that PHP 8.4 is fully supported by all dependencies and that tests are updated to cover this version, or consider including 8.3 if support for both is required.

default: '8.4'

henderkes · 2025-03-11T04:45:13Z

is there a reason why there isn't an alpine image in the docker workflows btw? just realized it's missing and I need to check on my vm manually

ubuntu/debian and rhel behave quite similarly other than their package managers using different package names, but alpine behaves differently since we're using different build tools and they have different system headers installed

henderkes · 2025-03-11T05:12:30Z

How does providing pre builts work, by the way? If I change the line to true, what else needs to be done to actually have it automatically download a prebuilt version?

crazywhalecc · 2025-03-11T05:17:12Z

is there a reason why there isn't an alpine image in the docker workflows btw? just realized it's missing and I need to check on my vm manually

Seems I forgot to change test ci... But it's just working, with musl-toolchain.

How does providing pre builts work, by the way

I've created a PR in "hosted" repo: static-php/static-php-cli-hosted#7

Once this pr merged, I'll merge it soon. This side we only need to set provide-pre-built to true and downloader will automatically search pre-built libraries.

henderkes · 2025-03-11T05:54:01Z

huh, this closed automatically and I cannot reopen it

henderkes · 2025-03-11T05:54:08Z

wtf

crazywhalecc · 2025-03-11T05:58:44Z

What happened? Is there a problem with main branch merge?

henderkes · 2025-03-11T06:10:30Z

no idea, i opened a new PR

crazywhalecc added new feature New feature or request kind/dependency Issues related to dependencies labels Mar 7, 2025

crazywhalecc requested changes Mar 7, 2025

View reviewed changes

src/SPC/builder/linux/LinuxBuilder.php Show resolved Hide resolved

henderkes requested a review from crazywhalecc March 7, 2025 09:09

crazywhalecc mentioned this pull request Mar 7, 2025

Add php and lib-base as special libraries to add dependencies to the root node #618

Merged

3 tasks

crazywhalecc reviewed Mar 8, 2025

View reviewed changes

crazywhalecc requested a review from Copilot March 10, 2025 16:06

Copilot AI reviewed Mar 10, 2025

View reviewed changes

henderkes closed this Mar 11, 2025

Uh oh!

add libacl for php-fpm compilation #611

add libacl for php-fpm compilation #611

Uh oh!

Conversation

henderkes commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist before merging

Uh oh!

henderkes commented Mar 5, 2025

Uh oh!

henderkes commented Mar 5, 2025

Uh oh!

crazywhalecc commented Mar 7, 2025

Uh oh!

henderkes commented Mar 7, 2025

Uh oh!

Uh oh!

henderkes commented Mar 7, 2025

Uh oh!

crazywhalecc commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazywhalecc left a comment

Choose a reason for hiding this comment

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

crazywhalecc commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

crazywhalecc commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

crazywhalecc commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

crazywhalecc commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazywhalecc commented Mar 10, 2025

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

henderkes commented Mar 10, 2025

Uh oh!

crazywhalecc commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

henderkes commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 11, 2025

Uh oh!

crazywhalecc commented Mar 11, 2025

henderkes commented Mar 5, 2025 •

edited

Loading

crazywhalecc commented Mar 7, 2025 •

edited

Loading

henderkes commented Mar 7, 2025 •

edited

Loading

crazywhalecc commented Mar 10, 2025 •

edited

Loading

crazywhalecc commented Mar 10, 2025 •

edited

Loading

crazywhalecc commented Mar 10, 2025 •

edited

Loading

crazywhalecc commented Mar 10, 2025 •

edited

Loading

henderkes commented Mar 10, 2025 •

edited

Loading

crazywhalecc commented Mar 10, 2025 •

edited

Loading

henderkes commented Mar 10, 2025 •

edited

Loading

henderkes commented Mar 11, 2025 •

edited

Loading