Skip to content

Conversation

@henderkes
Copy link
Contributor

@henderkes henderkes commented Nov 19, 2025

edit: also adding a .editorconfig, my ides keeps turning tabs into spaces

@henderkes henderkes requested a review from dunglas November 19, 2025 16:29
@henderkes henderkes marked this pull request as ready for review November 19, 2025 16:29
Comment on lines -171 to -174
GITHUB_TOKEN=$(cat /run/secrets/github-token) ./build-static.sh && \
if [ -n "${BUILD_PACKAGES}" ]; then \
./build-packages.sh; \
fi; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getting rid of this entirely all together in another PR


WORKDIR /usr/local/src/php
RUN git clone --branch=PHP-8.4 https://github.com/php/php-src.git . && \
# --enable-embed is only necessary to generate libphp.so, we don't use this SAPI directly
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we use libphp.so embed SAPI directly and therefore directly rely on --enable-embed.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is still relevant. We don't use the embed SAPI, but we need to use this flag to build because it builds libphp as a side effect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
# --enable-embed is only necessary to generate libphp.so, we don't use this SAPI directly
# --enable-embed is necessary to generate libphp.so, but we don't use the SAPI directly

.editorconfig Outdated

[*.sh]
indent_style = tab
tab_width = 2
Copy link
Member

Choose a reason for hiding this comment

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

Why not 4 as for Dockerfiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'm happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

I've no strong opinion on this but maybe should we use the same value everywhere for consistency?

@henderkes henderkes merged commit 49e98cc into php:main Nov 20, 2025
66 of 68 checks passed
@henderkes henderkes deleted the fix/static-size branch November 20, 2025 10:49
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.

2 participants