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.

Sign up for GitHub

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

Fixing build regression [BISON COMPILE] #871

Closed
wants to merge 25 commits into from
Closed

Fixing build regression [BISON COMPILE] #871

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3a6e9d3
Fixing build regression for PHP <=7.3
seanmorris Dec 14, 2023
fafa9e6
Removing Bison source files
seanmorris Dec 15, 2023
eebb534
remving extra formatting change
seanmorris Dec 15, 2023
2f00b16
Support multiple architechtures.
seanmorris Dec 18, 2023
cfeca7a
Support arbitrary arch.
seanmorris Dec 18, 2023
080d9b7
Support arbitrary arch.
seanmorris Dec 18, 2023
be40eae
Support arbitrary arch.
seanmorris Dec 18, 2023
2651f82
Proper indentation.
seanmorris Dec 18, 2023
7bc8d83
Don't build each version of Bison more than once
seanmorris Dec 19, 2023
fec45de
Don't build each version of Bison more than once
seanmorris Dec 19, 2023
09c6329
Phony targets.
seanmorris Dec 19, 2023
03ee0b0
Reverting artifacts
seanmorris Dec 19, 2023
a4c3ad0
Reverting artifacts
seanmorris Dec 19, 2023
e202c14
Removing extra copy.
seanmorris Dec 19, 2023
69388f7
Centralizing logic.
seanmorris Dec 20, 2023
8371941
Centralizing logic.
seanmorris Dec 20, 2023
6744e0c
Centralizing logic.
seanmorris Dec 20, 2023
a53d842
Centralizing logic.
seanmorris Dec 20, 2023
cefc9fa
Reverting artifacts.
seanmorris Dec 20, 2023
ae2d9f4
Merge conflict.
seanmorris Dec 20, 2023
0c4fa2e
Adding error checking for bison verison.
seanmorris Dec 20, 2023
1ef26cc
Adding error checking for bison verison.
seanmorris Dec 20, 2023
18b1884
HTTPS url.
seanmorris Dec 20, 2023
677e85a
Rebuilding artifacts on OSX.
seanmorris Dec 25, 2023
bf645ec
Merge branch 'trunk' into sm-fixing-build-regression
seanmorris Dec 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/php-wasm/compile/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
.PHONY: base-image bison3 bison2.7

base-image: base-image/.ready
base-image/.ready:
cd base-image; docker build -f ./Dockerfile -t playground-php-wasm:base .
Expand Down Expand Up @@ -28,7 +30,7 @@ libzip/dist/1.9.2/root/lib/lib/libzip.a: base-image libz
libpng16: libpng16/dist/root/lib/lib/libpng16.a
libpng16/dist/root/lib/lib/libpng16.a: base-image libz
mkdir -p ./libpng16/dist/root/lib
docker build -f ./libpng16/Dockerfile -t playground-php-wasm:libpng .
docker build -f ./libpng16/Dockerfile -t playground-php-wasm:libpng .
docker cp $$(docker create playground-php-wasm:libpng):/root/install/lib ./libpng16/dist/root/lib
docker cp $$(docker create playground-php-wasm:libpng):/root/install/include ./libpng16/dist/root/lib

Expand Down Expand Up @@ -76,11 +78,14 @@ libedit/dist/root/lib/lib/libedit.a: base-image libncurses
docker cp $$(docker create playground-php-wasm:libedit):/root/install/include ./libedit/dist/root/lib

bison2.7: bison2.7/dist/usr/local/bison/bin/bison
bison2.7/dist/usr/local/bison/bin/bison: base-image
bison2.7/dist/usr/local/bison/bin/bison: base-image bison2.7/Dockerfile
mkdir -p ./bison2.7/dist/usr/local/bison
docker build -f ./bison2.7/Dockerfile -t playground-php-wasm:bison2.7 . --progress=plain
docker cp $$(docker create playground-php-wasm:bison2.7):/usr/local/bison ./bison2.7/dist/usr/local


bison3: base-image bison3/Dockerfile
docker build -f ./bison3/Dockerfile -t playground-php-wasm:bison3 . --progress=plain

all: libz libzip libpng16 libxml2 libopenssl libsqlite3 libiconv libncurses libedit bison2.7
clean:
rm -rf ./libz/dist
Expand Down
3 changes: 2 additions & 1 deletion packages/php-wasm/compile/base-image/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ RUN set -euxo pipefail;\
wget \
unzip \
cmake \
python3
python3 \
sysvbanner
adamziel marked this conversation as resolved.
Show resolved Hide resolved

# Install Emscripten from the repository. We'd use the official
# Docker image, but there is no arm64 image available which makes
Expand Down
20 changes: 12 additions & 8 deletions packages/php-wasm/compile/bison2.7/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
FROM playground-php-wasm:base

COPY ./bison2.7/bison27.patch /root/bison27.patch
RUN wget http://ftp.gnu.org/gnu/bison/bison-2.7.tar.gz && \
tar -xvf bison-2.7.tar.gz && \
rm bison-2.7.tar.gz && \
cd bison-2.7 && \
git apply --no-index /root/bison27.patch && \
./configure --prefix=/usr/local/bison --with-libiconv-prefix=/usr/local/libiconv/ && \
make && \
make install
RUN set -euxo pipefail; \
wget http://ftp.gnu.org/gnu/bison/bison-2.7.tar.gz; \
adamziel marked this conversation as resolved.
Show resolved Hide resolved
tar -xvf bison-2.7.tar.gz; \
rm bison-2.7.tar.gz; \
cd bison-2.7 && { \
git apply --no-index /root/bison27.patch; \
./configure --prefix=/usr/local/bison --with-libiconv-prefix=/usr/local/libiconv/; \
make; \
make install; \
}; \
ln -s /usr/local/bison/bin/bison /usr/bin/bison; \
ln -s /usr/local/bison/bin/yacc /usr/bin/yacc;
3 changes: 3 additions & 0 deletions packages/php-wasm/compile/bison3/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
FROM playground-php-wasm:base

RUN apt install -y bison;
24 changes: 20 additions & 4 deletions packages/php-wasm/compile/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const platformDefaults = {
const platform = args.PLATFORM;

/* eslint-disable prettier/prettier */
const getArg = (name) => {
const getArg = (name, wrapped = true) => {
let value =
name in args
? args[name]
Expand All @@ -146,7 +146,10 @@ const getArg = (name) => {
if (name === 'PHP_VERSION') {
value = fullyQualifiedPHPVersion(value);
}
return `${name}=${value}`;
if (wrapped) {
return `${name}=${value}`;
}
return value;
};

const requestedVersion = getArg('PHP_VERSION');
Expand All @@ -156,10 +159,21 @@ if (!requestedVersion || requestedVersion === 'undefined') {
process.exit(1);
}

const [majorPhpVersion, minorPhpVersion] = getArg('PHP_VERSION', false).split(
'.'
);

let bisonVersion = '3';

if (majorPhpVersion <= 7 && minorPhpVersion <= 3) {
adamziel marked this conversation as resolved.
Show resolved Hide resolved
bisonVersion = '2.7';
}

const sourceDir = path.dirname(new URL(import.meta.url).pathname);

// Build the base image
await asyncSpawn('make', ['base-image'], { cwd: sourceDir, stdio: 'inherit' });
// Build the base image & bison
const targets = ['base-image', 'bison' + bisonVersion];
await asyncSpawn('make', targets, { cwd: sourceDir, stdio: 'inherit' });

await asyncSpawn(
'docker',
Expand Down Expand Up @@ -202,6 +216,8 @@ await asyncSpawn(
getArg('WITH_WS_NETWORKING_PROXY'),
'--build-arg',
`EMSCRIPTEN_ENVIRONMENT=${platform === 'node' ? 'node' : 'web'}`,
'--build-arg',
`BISON_VERSION=${bisonVersion}`,
],
{ cwd: sourceDir, stdio: 'inherit' }
);
Expand Down
43 changes: 18 additions & 25 deletions packages/php-wasm/compile/php/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
FROM playground-php-wasm:base
# The Bison version to use to build PHP with.
# Must be 2.7 for PHP <= 7.3
# Must be 3 for PHP >= 7.4
ARG BISON_VERSION
FROM playground-php-wasm:bison${BISON_VERSION}

Copy link
Collaborator

@adamziel adamziel Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any way to compute the bison version in this Dockerfile here instead of in build.js?

Copy link
Contributor Author

@seanmorris seanmorris Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@adamziel If we do that, we can't cache it and we'll be forced to rebuild on every iteration.

I think we should move as much of this calculation into the build JS as possible, then think about re-ordering the dockerfile with the implicit layer-caching mechanism in mind.

If we can cache the identical steps between different builds, and re-use them, we should be able to get the build to run a lot faster.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that logic lives in build.js, then the Dockerfile is no longer self-contained – you need prior knowledge about the mapping between specific PHP versions and Bison versions to be able to use it. Is there a way to keep that mapping inside the Dockerfile and still benefit from caching?

Copy link
Contributor Author

@seanmorris seanmorris Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@adamziel Nope. If Docker considers a PHP_VERSION arg that is different from that of a previous build, then the layer-hash will differ and the cache will not be used for that build.

I think that the value we gain from a shorter build is greater than the value we lose from compromising a self-contained dockerfile. Our build system architecture is already relatively highly coupled, and the existing dockerfile already has dependencies it cannot invoke on its own.

I think this still represents a step forward.

Edit: I can put some error-checking logic in to clarify things a bit.

Copy link
Contributor Author

@seanmorris seanmorris Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Errors will print in red if incompatible versions are provided:

wordpress-playground/packages/php-wasm/compile/php/Dockerfile

Lines 24 to 39 in 1ef26cc

FROM playground-php-wasm:bison${BISON_VERSION}
ARG BISON_VERSION
ARG PHP_VERSION
RUN set -euxo pipefail; \
if [[ "${PHP_VERSION:0:1}" -le "7" && "${PHP_VERSION:2:1}" -le "3" ]]; then \
if [[ "${BISON_VERSION}" != "2.7" ]]; then \
echo -e "\e[31mBISON_VERSION must be 2.7 for PHP "${PHP_VERSION}". Got" ${BISON_VERSION} "\e[0m"; \
exit 128; \
fi; \
else \
if [[ "${BISON_VERSION}" != "3" ]]; then \
echo -e "\e[31mBISON_VERSION must be 3 for PHP "${PHP_VERSION}". Got" ${BISON_VERSION} "\e[0m"; \
exit 128; \
fi; \
fi;

Copy link
Collaborator

Choose a reason for hiding this comment

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

If Docker considers a PHP_VERSION arg that is different from that of a previous build, then the layer-hash will differ and the cache will not be used for that build.

Using the --download-only flag for apt would do the trick here, though. Both Bison 2.7 and Bison 3.0 may then exist in the docker cache as static files, and then a bash if could just plug-in the correct version depending on the PHP_VERSION.

the value we gain from a shorter build

How much shorter is it, though? Are we talking about minutes or seconds?

Our build system architecture is already relatively highly coupled, and the existing dockerfile already has dependencies it cannot invoke on its own.

Oh, interesting, I haven't thought of it like that. What would be an example of such a dependency?

# The PHP version to build.
# This value must point to an existing branch of the
Expand All @@ -9,7 +13,7 @@ FROM playground-php-wasm:base
ARG PHP_VERSION

# Clone PHP
RUN git clone https://github.com/php/php-src.git php-src \
RUN git clone https://github.com/php/php-src.git php-src \
--branch PHP-$PHP_VERSION \
--single-branch \
--depth 1;
Expand All @@ -18,17 +22,16 @@ RUN cd php-src && ./buildconf --force

# Bring in the libraries
RUN mkdir -p /root/lib/include /root/lib/lib /libs
COPY ../bison2.7/dist/ /libs/bison2.7
COPY ../libedit/dist/root/lib /root/lib
COPY ../libiconv/dist/root/lib /root/lib
COPY ../libncurses/dist/root/lib /root/lib
COPY ../libopenssl/dist/root/lib /root/lib
COPY ../libpng16/dist/root/lib /root/lib
COPY ../libsqlite3/dist/root/lib /root/lib
COPY ../libxml2/dist/root/lib /root/lib
COPY ../libz/dist/root/lib /root/lib
COPY ../libzip/dist/1.2.0/root/lib /libs/libzip/1.2.0
COPY ../libzip/dist/1.9.2/root/lib /libs/libzip/1.9.2
COPY ./libedit/dist/root/lib /root/lib
COPY ./libiconv/dist/root/lib /root/lib
COPY ./libncurses/dist/root/lib /root/lib
COPY ./libopenssl/dist/root/lib /root/lib
COPY ./libpng16/dist/root/lib /root/lib
COPY ./libsqlite3/dist/root/lib /root/lib
COPY ./libxml2/dist/root/lib /root/lib
COPY ./libz/dist/root/lib /root/lib
COPY ./libzip/dist/1.2.0/root/lib /libs/libzip/1.2.0
COPY ./libzip/dist/1.9.2/root/lib /libs/libzip/1.9.2

# Build PHP

Expand All @@ -55,17 +58,6 @@ RUN touch /root/.configure-flags && \
touch /root/.emcc-php-wasm-flags && \
touch /root/.EXPORTED_FUNCTIONS


# PHP <= 7.3 requires Bison 2.7
# PHP >= 7.4 and Bison 3.0
RUN if [[ "${PHP_VERSION:0:1}" -le "7" && "${PHP_VERSION:2:1}" -le "3" ]]; then \
mv /libs/bison2.7/usr/local/bison /usr/local/bison && \
ln -s /usr/local/bison/bin/bison /usr/bin/bison && \
ln -s /usr/local/bison/bin/yacc /usr/bin/yacc; \
else \
apt install -y bison; \
fi;
adamziel marked this conversation as resolved.
Show resolved Hide resolved

# Add libzip and zlib files if needed
RUN if [ "$WITH_LIBZIP" = "yes" ]; then \
if [[ "${PHP_VERSION:0:1}" -le "7" && "${PHP_VERSION:2:1}" -le "3" ]]; then \
Expand Down Expand Up @@ -186,7 +178,8 @@ RUN if [ "$WITH_MYSQL" = "yes" ]; then \

# Build the patched PHP
WORKDIR /root/php-src
RUN source /root/emsdk/emsdk_env.sh && \
RUN banner "PHP ${PHP_VERSION}" && \
source /root/emsdk/emsdk_env.sh && \
emconfigure ./configure \
PKG_CONFIG_PATH=$PKG_CONFIG_PATH \
# Fibers are a PHP 8.1+ feature. They are compiled as
Expand Down
Loading