Skip to content

refactor: replace SPC_LIBC with SPC_TARGET and update related logic #810

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

Merged
merged 24 commits into from
Jun 30, 2025

Conversation

crazywhalecc
Copy link
Owner

@crazywhalecc crazywhalecc commented Jun 28, 2025

What does this PR do?

replace SPC_LIBC with SPC_TARGET and update related logic. This fixes #809

Looks like after doing this, we can build musl libc based static-php using bin/spc-alpine-docker and SPC_TARGET=musl.

Checklist before merging

If your PR involves the changes mentioned below and completed the action, please tick the corresponding option.
If a modification is not involved, please skip it directly.

  • If you modified *.php or *.json, run them locally to ensure your changes are valid:
    • PHP_CS_FIXER_IGNORE_ENV=1 composer cs-fix
    • composer analyse
    • composer test
    • bin/spc dev:sort-config
  • If it's an extension or dependency update, please ensure the following:
    • Add your test combination to src/globals/test-extensions.php.
    • If adding new or fixing bugs, add commit message containing extension test or test extensions to trigger full test suite.

@crazywhalecc crazywhalecc added the kind/framework Issues related to CLI app framework label Jun 28, 2025
Copy link
Collaborator

@henderkes henderkes left a comment

Choose a reason for hiding this comment

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

I don't like this. It doesn't help us move away from the many different conditionals. I want to get rid of gcc, clang and musl-gcc completely. Using the SPC_TARGET instead of SPC_LIBC is essentially just renaming it. I want SPC_TARGET to be a target triple for zig specifically and the old logic keeps using SPC_LIBC.

I like the code of this PR though, don't grt me wrong. But please keep SPC_LIBC instead of using a new env variable.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

I don't like this. It doesn't help us move away from the many different conditionals.

In fact, given the current situation, I don't want to get rid of the conditional statement. This just makes the judgment logic clearer. Conditional statements are difficult to avoid, so why not do it as a complete target set?

As for whether to continue to use SPC_LIBC, I think it can continue to be used, but it should not use SPC_LIBC=musl but musl-static. This is why the shared build of musl can be achieved in this PR.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces the old SPC_LIBC environment variable with a more flexible SPC_TARGET model and updates build logic and toolchain selection accordingly.

  • Deprecates SPC_LIBC in favor of SPC_TARGET and adds conversion logic.
  • Introduces SPCTarget helper for target checks and refactors conditional flags across builders and libraries.
  • Adds a new dev:env command to inspect current SPC environment variables.

Reviewed Changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/SPC/util/GlobalEnvManager.php Deprecate SPC_LIBC, auto-convert to SPC_TARGET, hook into SPCTarget and toolchain init
src/SPC/util/SPCTarget.php Introduce target constants and helpers for toolchain init
src/SPC/builder/unix/UnixBuilderBase.php Replace inline SPC_LIBC checks with SPCTarget and call GlobalEnvManager::afterInit
src/SPC/builder/unix/library/imagemagick.php Refactor OpenMP, LDFLAGS and library flags to use SPCTarget
src/SPC/command/dev/EnvCommand.php New CLI command for displaying SPC environment variables
Comments suppressed due to low confidence (3)

src/SPC/command/dev/EnvCommand.php:27

  • The new EnvCommand does not have accompanying unit or integration tests. Add tests covering both successful retrieval and error conditions (unset variable) to ensure correct behavior.
    public function handle(): int

src/SPC/builder/unix/library/imagemagick.php:35

  • The refactored logic disables OpenMP for all glibc targets, but originally it only disabled OpenMP when using devtoolset-10. Consider restoring the condition to only disable OpenMP for the specific devtoolset scenario rather than all glibc builds.
                SPCTarget::isTarget(SPCTarget::GLIBC) ? '--disable-openmp' : '--enable-openmp',

src/SPC/builder/unix/library/imagemagick.php:41

  • The condition is inverted: it applies '-static -ldl' for non-musl-static targets instead of musl-static builds. Swap the predicate so that the static flag is only applied when SPCTarget::isTarget(SPCTarget::MUSL_STATIC) is true.
        $ldflags = !SPCTarget::isTarget(SPCTarget::MUSL_STATIC) ? ('-static -ldl') : '-ldl';

@henderkes
Copy link
Collaborator

As for whether to continue to use SPC_LIBC, I think it can continue to be used, but it should not use SPC_LIBC=musl but musl-static. This is why the shared build of musl can be achieved in this PR.

Yes, please. But keep SPC_TARGET reserved for actually specifying the target (which is only possible with zig).

@crazywhalecc
Copy link
Owner Author

Yes, please. But keep SPC_TARGET reserved for actually specifying the target (which is only possible with zig).

Then our understanding of SPC_TARGET may be different. The target you understand is more like a toolchain options, while the current SPC_TARGET is like the result of the build. The toolchain options should be specified in the variables or configurations at the next level of SPC_TARGET.

@henderkes
Copy link
Collaborator

henderkes commented Jun 28, 2025

No, I'm thinking of SPC_TARGET as an actual target triple/quadruple. x86_64-linux-gnu.2.17 for example. Or aarch64-linux-musl -dynamic.

It's what I want to reserve for Zig compilation, because when Zig is implemented, I want to deprecate (meaning, people have to explicitly configure their environment, rather than rely on our prebuilt toolchains or implicit behaviour) all other builds and only support Zig. It can do everything we need and we only need to support one platform, that behaves identical on alpine as it does on ubuntu or rhel, because it's statically compiled and completely ignores the system.

So I want SPC_LIBC's behaviour to stay the way it is now, for compatibility reasons, until 3.0.

People who want to switch to Zig now can use SPC_TARGET - which switches behaviour over to ignore SPC_LIBC and others.
With 3.0 we can then completely remove SPC_LIBC and the musl toolchain. We don't need them anymore. We don't need to check for (almost) any conditions, other than odd cases like openssl's internal target with the -clang postfix.

This gives everyone a clear "upgrade" path with ample time, but will in the end cut our maintenance effort in half. We won't even need docker anymore.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

So I want SPC_LIBC's behaviour to stay the way it is now, for compatibility reasons, until 3.0.

That's also why I created this PR. The SPC_LIBC is not clear and SPC_TARGET can clearly describe what we are building. At the same time, this SPC_TARGET is not only for Linux, but also for other systems (if new target is needed in the future).

I'm thinking of SPC_TARGET as an actual target triple/quadruple.

What you are talking about does not conflict with this PR. In 3.0, we can expand the parsing level of SPC_TARGET, which is not difficult based on this. But in the current state, we do not need to be too detailed for the time being, because we also don't support any way to specify musl version.

@henderkes
Copy link
Collaborator

What you are talking about does not conflict with this PR. In 3.0, we can expand the parsing level of SPC_TARGET, which is not difficult based on this. But in the current state, we do not need to be too detailed for the time being, because we also don't support any way to specify musl version.

Which is great in theory, but wouldn't that mean we need to keep supporting native gcc/musl toolchain/native gcc on glibc? Otherwise we change ABI if we switch over to Zig and don't have a clear upgrade path that doesn't mean switching exactly when we release 3.0.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

Since the existing toolchains and environments have been proven to be usable and partially stable and used by a lot of projects, it is not difficult to maintain them for us now. Secondly, the addition of the Zig toolchain will not make spc 3.0, because we didn't remove any ABI/API at present.

So this PR is more like a tidying up of the past messy toolchain judgments + supporting the future Zig or other toolchain more elegant with current toolchain.

In particular, musl-toolchain will not be downloaded when SPC_TOOLCHAIN ​​is not detected as musl. The Zig toolchain can also be doctor-fixed by simply replacing a string.

@henderkes
Copy link
Collaborator

Since the existing toolchains and environments have been proven to be usable and partially stable and used by a lot of projects, it is not difficult to maintain them for us now.

I disagree because there are a lot of patches for individual toolchains. Centos 7 gcc needs a Docker container and individual patches to like static -lstdc++ because the shared library doesn't contain all symbols we use. GCC under Ubuntu and under RHEL behave differently with different search paths. We need the musl toolchain that we compile ourselves, which is still on gcc 13. We have separate code branches in so many places.

And there's no reason to keep them. Zig-cc can do everything that the other toolchains can.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

Well, I think we don't have any disagreement on removing redundant toolchains, but I think deleting them is a 3.0 thing. I just said that the "current" PRs do not need to delete any toolchains and judgments, it is not difficult to maintain for now, not to say "it is not difficult to maintain them all the time".

So let's get back to the topic, do you have any reviews about what this PR does?

@henderkes
Copy link
Collaborator

I just said that the "current" PRs do not need to delete any toolchains and judgments, it is not difficult to maintain for now, not to say "it is not difficult to maintain them all the time".

Ah ok, I understand.

I think this is a good move, but I would like to keep it called SPC_LIBC for backwards compatibility. 'musl' needs to stay musl-static for compatibility reasons and we will add a musl-dynamic later.

I want SPC_TARGET to be reserved for zig builds.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

I want SPC_TARGET to be reserved for zig builds.

What do you think if I keep continue to use SPC_LIBC as the mapping of SPC_TARGET:

  • SPC_LIBC=musl -> SPC_TARGET=musl-static
  • SPC_LIBC=glibc -> SPC_TARGET=glibc
  • SPC_LIBC=musl-shared -> SPC_TARGET=musl

And for zig builds, I want to treat SPC_TARGET as glibc@zig-2.17 or glibc@2.17 for backward compatibility and forward compatibility. I don't know which symbol (or char?) is appropriate, but the specific selection symbol should always be at the end of the real "target" string. For practical purposes, zig is zig, and @{something} means we are telling SPC to tell the toolchain predicate a specific condition.

@henderkes
Copy link
Collaborator

henderkes commented Jun 28, 2025

SPC_LIBC=musl-dynamic -> SPC_TARGET=native-native-musl -dynamic
SPC_LIBC=musl -> SPC_TARGET=native-native-musl
SPC_LIBC=glibc -> SPC_TARGET=native-native-gnu
SPC_LIBC=glibc@2.17 -> not supported, use SPC_TARGET=native-native-gnu.2.17 instead!

but keep the usage of SPC_LIBC with gcc! Only when SPC_TARGET is actually used, it should always use zig.

@henderkes
Copy link
Collaborator

Also, the reason why I don't want SPC_TARGET to be an enum is that all of the following are valid x86_64 options and can be used safely.

-target x86_64-redhat-linux-gnu.2.17
-target x86_64-suse-linux-gnu.2.17 # centos 7+
-target x86_64-linux-gnu.2.17 # equals x86_64-unknown-linux-gnu.2.17 
-target x86_64-linux-gnu.2.25 # rhel 8+
-target x86_64-linux-musl # equals target x86_64-unknown-linux-musl -static 
-target x86_64-linux-musl.1.25
-target x86_64-linux-musl -dynamic
-target native-native-linux-gnu
-target native-native-redhat-linux-gnu
# and so on

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Jun 28, 2025

I'm sorry to have argued with you for so long. I understood your idea of ​​SPC_TARGET as a zig-only one from the beginning - it has little to do with the core solution of this PR. I may have overthought it a little, so I calmly read through the content we discussed and the zig implementation branch again, and concluded that this PR is generally reasonable, but it is indeed more reasonable to modify the SPC_TARGET part in your way. (Also sorry, I used some translations below, and I may be faster in typing some expressions in Chinese)

Part 1 Explanation

In other words, our goal of iterating to the new version is the same, but the iteration method is slightly different. My PR does not conflict with the Zig goal, but reduces the cost of Zig integration through architectural optimization. Through the three-step strategy of "modular toolchain → add Zig → eliminate the old toolchain", we can not only ensure the stability of the current project, but also lay a solid foundation for the cross-platform Zig compilation of version 3.0.

Of course, I haven't finished the current PR. The goal is to set patch points for almost all judgment parts and place such toolchain-specific patches in the corresponding Toolchain and Target. This will make it very easy to delete the toolchain in the future. Don't worry, I will complete this, and I will do it when refactoring error handling and patch points. The reason I haven't done it now is that I am still writing new documents, and for newly added patch points, I need to write documents before introducing them, otherwise it is difficult to infer which patch points there are from the code. I know this has little to do with SPC_TARGET. This PR is more like a decoupling of the toolchain, and by the way, SPC_LIBC is mapped to SPC_TARGET, and only this conflicts with your logic.

Part 2 SPC_TARGET Optimization in this PR

In addition, regarding the zig target, I just read the zig document. It is indeed more advantageous to use the zig format as a target and as a fill-in-the-blank question, so I think maybe the following changes can be made:

The current PR's Toolchain remains independent, but SPCTarget is changed to:

  1. Add a target parsing function for the zig format so that we can determine the target and version of the build.
  2. Before implementing zig, SPC_LIBC will continue to be used as a usable value and determine the musl-toolchain toolchain. When the SPC_LIBC variable is detected, the old toolchain will continue to be used (and a prompt will be prompted that SPC_LIBC will be deprecated).
  3. SPC_LIBC is still retained in the configuration but @deprecated is prompted. If you need to use zig, you need to explicitly specify SPC_TARGET and delete SPC_LIBC.

Part 3 3.0 Plan

Based on this and zig PRs, for 3.0, the following work is to be done:

  1. Delete SPC_LIBC support.
  2. Delete two toolchains support under Linux (gcc-native, musl).
  3. Replace Docker commands with error prompts, and the prompt content is the equivalent SPC_TARGET.
  4. Update the document section.
  5. Build glibc by default.

@henderkes
Copy link
Collaborator

I understood your idea of ​​SPC_TARGET as a zig-only one from the beginning - it has little to do with the core solution of this PR.

Yes! All I want changed is that we don't use the SPC_TARGET env variable for toolchain detection because I want it to be the clear migration path to using zig.

Add a target parsing function for the zig format so that we can determine the target and version of the build.

I'm not sure if we really need to. The only part we would probably have to parse out is -static (implied on musl) or -dynamic. Because frankenphp needs to pass -static -static-pie to the go linker too for static builds.

Build glibc by default.

I haven't thought about that yet. We could keep musl (static) the default as it's so simple to switch with SPC_TARGET.

We can build musl -dynamic on glibc systems (install musl) and I think glibc (-dynamic) on musl (install gcompat libstd++). I've tested a minimal example of the first.

@crazywhalecc
Copy link
Owner Author

Good. I will make some changes to this PR tomorrow and leave the zig part. You could complete it in the zig branch after this PR merged.

@henderkes
Copy link
Collaborator

henderkes commented Jun 29, 2025

zig branch is now ready, I can compile:

musl-dynamic >= 1.20 (maybe lower?)
musl-static (default for musl)
glibc (-dynamic, static is not possible) >= 2.17

all ZTS extensions (shared and static) except for grpc, which always fails for me, no matter the compiler

@crazywhalecc
Copy link
Owner Author

all ZTS extensions (shared and static) except for grpc, which always fails for me, no matter the compiler

I think we need to open a separate issue or branch to deal with grpc issues. Its official default static build method conflicts with our own dependencies. Maybe using CMake is a workaround, but it would be better to let grpc's dependencies use spc's source instead of its several gigabytes git submodule. Every time I pull it on a server behind THE firewall, I get errors over and over again, which is really annoying.

@henderkes
Copy link
Collaborator

Yeah I'm just leaving grpc alone. When a library fails to build from source with its given instructions, I'm not really interested in spending hours to fix it.

@henderkes
Copy link
Collaborator

Looks like the scl_enable isn't enough and we actually need to overwrite the path, weird.

Copy link
Collaborator

@henderkes henderkes left a comment

Choose a reason for hiding this comment

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

when you fix the musl toolchain install this should be good to be merged

I just realized native-windows and native-macos are actually valid targets, so yes, nothing speaks against using them

@henderkes
Copy link
Collaborator

no idea why the patches are failing on alpine now, I can't reproduce it on my local alpine vm

@crazywhalecc
Copy link
Owner Author

no idea why the patches are failing on alpine now, I can't reproduce it on my local alpine vm

Because current main branch has duplicate patch bug and #811 fixed it. The tests workflow will build twice (the second time is to test embed build on *nix), that's why only workflow fails and your local is good.

But that pr brings new bug for windows build and I haven't figured it out.

@henderkes
Copy link
Collaborator

looks like it introduces a runtime mismatch, maybe one of the patches that previously fixed that isn't running through successfully?

This is good to be merged in my eyes, after micro.

@crazywhalecc crazywhalecc merged commit 9190335 into main Jun 30, 2025
21 checks passed
@crazywhalecc crazywhalecc deleted the refactor/spc-target branch June 30, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/framework Issues related to CLI app framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: SPC_LIBC change to more meaningful vars
2 participants