search

nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.41k stars 331 forks source link

Docker: Unable to build images for PHP 8.3.0RC6 using packages Makefile #1009

Open tippexs opened 11 months ago

tippexs commented 11 months ago

Problem

According to the documentation it should be possible to build custom container images using official Docker base images using make in pkg/docker.

Issuing

make build-php8.3.0RC6 VERSIONS_php=8.3.0RC6 VARIANT_php=cli

will create a Dockerfile BUT it will not include the information to build the Units php language module. Here is a diff from the two Dockerfiles generated by make.

diff Dockerfile.php8.3.0RC6 Dockerfile.php8.2

$> diff Dockerfile.php8.3.0RC6 Dockerfile.php8.2
1c1
< FROM php:8.3.0RC6-cli-bullseye
---
> FROM php:8.2-cli
3c3
< LABEL org.opencontainers.image.title="Unit (php8.3.0RC6)"
---
> LABEL org.opencontainers.image.title="Unit (php8.2)"
48c48
<     &&  \
---
>     && /bin/true \
50,51c50,51
<     && ./configure  \
<     && make -j $NCPU  \
---
>     && ./configure php \
>     && make -j $NCPU php-install \
54,55c54,55
<     && ./configure  \
<     && make -j $NCPU  \
---
>     && ./configure php \
>     && make -j $NCPU php-install \
63c63
<     &&  \
---
>     && ldconfig \

Expected Outcome

The make command would have created a customized Unit Container accepting the php verison 8.3.0RC6

Why do we need to fix this?

To test application code and Unit against early versions / Release-Candidates / Alpha-Versions of a language is super important and should NOT require extra work to build custom Dockerfiles.

ac000 commented 11 months ago

It seems to break as soon as it hits anything that isn't [0-9.] in the version.

tippexs commented 11 months ago

Yes with a high chance because of this https://github.com/nginx/unit/blob/3fdf8c63a2dd4b3d676754d2e605877a8689e4c9/pkg/docker/Makefile#L112

tippexs commented 11 months ago

I think we can make this better in terms of parsing.

cat template.Dockerfile | sed \
            -e 's,@@VERSION@@,$(VERSION),g' \
            -e 's,@@PATCHLEVEL@@,$(PATCHLEVEL),g' \
            -e 's,@@CONTAINER@@,$(CONTAINER_$*),g' \
            -e 's,@@CONFIGURE@@,$(CONFIGURE_$(call modname, $*)),g' \
            -e 's,@@INSTALL@@,$(INSTALL_$(call modname, $*)),g' \
            -e 's,@@RUN@@,$(RUN_$(call modname, $*)),g' \
            -e 's,@@MODULE_PREBUILD@@,$(MODULE_PREBUILD_$(call modname, $*)),g' \
            -e 's,@@MODULE@@,$*,g' \
            > $@

This can not get the php out of php8.3.0RC6 as it will only strip out .-0-9 and will leave phpRC as a module which will not work.

I am flexible what exactly we should do here but not beeing able to building a Container Image that is based on a name different from php:8.3 is a missing feature. Calling it a feature because the Makefile was never designed to work with such releases.

ac000 commented 11 months ago

On Fri, 24 Nov 2023 07:19:08 -0800 Timo Stark @.***> wrote:

I am flexible what exactly we should do here but not beeing able to building a Container Image that is based on a name different from php:8.3 is a missing feature. Calling it a feature because the Makefile was never designed to work with such releases.

It seems we only need to find the right thing for the configure command and it can only be one of a set number of items; php, python, perl etc...

I don't see why we can't just take the contents of the CONFIGURE_ variable splitting on whitespace to remove any configure options.

ac000 commented 11 months ago

Ok, so that's not so simple.

This will also break with "wasm-wasi-http" as a module name.

I can't see a clean way of solving this without introducing a new MODULE= variable.

ac000 commented 11 months ago 
diff --git a/pkg/docker/Makefile b/pkg/docker/Makefile
index 237228a9..366e2a4e 100644
--- a/pkg/docker/Makefile
+++ b/pkg/docker/Makefile
@@ -6,10 +6,12 @@ include ../shasum.mak
 DEFAULT_VERSION := $(NXT_VERSION)

 VERSION ?= $(DEFAULT_VERSION)
 PATCHLEVEL ?= 1

+MODULE =
+
 MODULES ?= go jsc node perl php python ruby wasm

 VARIANT ?= bullseye

 VERSIONS_minimal ?=
@@ -107,11 +109,11 @@ endef
 default:
        @echo "valid targets: all build dockerfiles library clean"

 MODVERSIONS = $(foreach module, $(MODULES), $(foreach modversion, $(shell for v in $(VERSIONS_$(module)); do echo $$v; done | sort -r), $(module)$(modversion))) wasm minimal

-modname = $(shell echo $1 | /usr/bin/tr -d '.01234567890-')
+modname = $(MODULE)

 dockerfiles: $(addprefix Dockerfile., $(MODVERSIONS))
 build: $(addprefix build-, $(MODVERSIONS))

 Dockerfile.%: ../../version template.Dockerfile
$ make build-php8.3.0RC6 MODULE=php VERSIONS_php=8.3.0RC6 VARIANT_php=cli-bullseye CONFIGURE_php=php INSTALL_php=php-install RUN_php=ldconfig MODULE_PREBUILD_php=/bin/true
tippexs commented 11 months ago

I would implement it in a similar way to other functions. if MODULE is defined use it, if not go back to the default way. This will also maintain backwards compatibility. But as you mentioned, this is probably the best solution for now.

ac000 commented 11 months ago

As it stands it's fundamentally flawed, but there is a better way to fix it without introducing a MODULE variable...

ac000 commented 11 months ago

Looking like a MODULE variable is the only sane way to do this...

tippexs commented 11 months ago

I am fine with the module variable. Will do some testing with the patch and let you know. Thanks @ac000

ac000 commented 11 months ago

It would need some extra work as it likely breaks some other targets.

ac000 commented 11 months ago

There seems to be another issue with this thing. Doing a make clean removes a bunch of dockerfiles which are tracked by git... this seems wrong

$ git status
On branch master
Your branch is up to date with 'upstream/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
...
$ make clean
rm -f Dockerfile.*
$ git status
On branch master
Your branch is up to date with 'upstream/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    Dockerfile.go1.20
        deleted:    Dockerfile.go1.21
        deleted:    Dockerfile.jsc11
        deleted:    Dockerfile.minimal
        deleted:    Dockerfile.node18
        deleted:    Dockerfile.node20
        deleted:    Dockerfile.perl5.36
        deleted:    Dockerfile.perl5.38
        deleted:    Dockerfile.php8.2
        deleted:    Dockerfile.python3.11
        deleted:    Dockerfile.ruby3.2
        deleted:    Dockerfile.wasm

Untracked files:
  (use "git add <file>..." to include in what will be committed)
...

Hmm and then...

$ make all
===> Building Dockerfile.go1.21
...
$ git status
On branch master
Your branch is up to date with 'upstream/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   Dockerfile.go1.20
        modified:   Dockerfile.go1.21
        modified:   Dockerfile.jsc11
        modified:   Dockerfile.minimal
        modified:   Dockerfile.node18
        modified:   Dockerfile.node20
        modified:   Dockerfile.perl5.36
        modified:   Dockerfile.perl5.38
        modified:   Dockerfile.php8.2
        modified:   Dockerfile.python3.11
        modified:   Dockerfile.ruby3.2
        modified:   Dockerfile.wasm

Untracked files:
  (use "git add <file>..." to include in what will be committed)
...

Hmm, OK, so that's all down to a version change, 1.31.1 -> 1.32.0

I'd say these dockerfiles shouldn't be tracked by git...

ac000 commented 11 months ago

Generally speaking, things which are generated artefacts should not be tracked by version control.

ac000 commented 11 months ago

Actually the above patch does look like the best fix for now...

thresheek commented 11 months ago

Generally speaking, things which are generated artefacts should not be tracked by version control.

The generated Dockerfiles are later used to build Docker Official images, so we need to store them in our version control.

ac000 commented 11 months ago

This is about the best I can come up with while fixing it for this and not breaking it for the other targets.

NOTE: This introduces a new target, gen-<target>, specifically for this case, e.g

$ make gen-php8.3.0RC6 MODULE=php VERSIONS_php=8.3.0RC6 VARIANT_php=cli
diff --git a/pkg/docker/Makefile b/pkg/docker/Makefile
index 237228a9..954623b3 100644
--- a/pkg/docker/Makefile
+++ b/pkg/docker/Makefile
@@ -6,10 +6,12 @@ include ../shasum.mak
 DEFAULT_VERSION := $(NXT_VERSION)

 VERSION ?= $(DEFAULT_VERSION)
 PATCHLEVEL ?= 1

+MODULE   =
+
 MODULES ?= go jsc node perl php python ruby wasm

 VARIANT ?= bullseye

 VERSIONS_minimal ?=
@@ -103,11 +105,11 @@ export RUST_VERSION=1.71.0 \\\n \
 \ \ \ \&\& make -C pkg/contrib .wasmtime \\\n \
 \ \ \ \&\& install -pm 755 pkg/contrib/wasmtime/target/release/libwasmtime.so /usr/lib/\$$\(dpkg-architecture -q DEB_HOST_MULTIARCH\)/
 endef

 default:
-       @echo "valid targets: all build dockerfiles library clean"
+       @echo "valid targets: all build gen-<target> dockerfiles library clean"

 MODVERSIONS = $(foreach module, $(MODULES), $(foreach modversion, $(shell for v in $(VERSIONS_$(module)); do echo $$v; done | sort -r), $(module)$(modversion))) wasm minimal

 modname = $(shell echo $1 | /usr/bin/tr -d '.01234567890-')

@@ -129,10 +131,31 @@ Dockerfile.%: ../../version template.Dockerfile

 build-%: Dockerfile.%
        docker pull $(CONTAINER_$*)
        docker build --no-cache -t unit:$(VERSION)-$* -f Dockerfile.$* .

+gen-%: ../../version template.Dockerfile
+ifeq ($(MODULE), )
+       @echo "MODULE not set, set it to one of the following:"
+       @echo "    $(MODULES)"
+       @false
+else
+       @echo "===> Building Dockerfile.$*"
+       cat template.Dockerfile | sed \
+               -e 's,@@VERSION@@,$(VERSION),g' \
+               -e 's,@@PATCHLEVEL@@,$(PATCHLEVEL),g' \
+               -e 's,@@CONTAINER@@,$(CONTAINER_$*),g' \
+               -e 's,@@CONFIGURE@@,$(CONFIGURE_$(MODULE)),g' \
+               -e 's,@@INSTALL@@,$(INSTALL_$(MODULE)),g' \
+               -e 's,@@RUN@@,$(RUN_$(MODULE)),g' \
+               -e 's,@@MODULE_PREBUILD@@,$(MODULE_PREBUILD_$(MODULE)),g' \
+               -e 's,@@MODULE@@,$*,g' \
+               > Dockerfile.$*
+       docker pull $(CONTAINER_$*)
+       docker build --no-cache -t unit:$(VERSION)-$* -f Dockerfile.$* .
+endif
+
 library:
        @echo "# this file is generated via https://github.com/nginx/unit/blob/$(shell git describe --always --abbrev=0 HEAD)/pkg/docker/Makefile"
        @echo ""
        @echo "Maintainers: Unit Docker Maintainers <docker-maint@nginx.com> (@nginx)"
        @echo "GitRepo: https://github.com/nginx/unit.git"
tippexs commented 9 months ago

Coming back to the issue to discuss this here. Please tell me if the PR is the better place. Maybe we should consider some re-design if the current approach is not working as expected. I have updated the issue a little bit to reflect what we need.

The Makefile is super important to build or Dockerfiles automatically. So we have to find a way to create an implementation that will be able to handle future builds.

My Question basically

make build-php8.3.0RC6 VERSIONS_php=8.3.0RC6 VARIANT_php=cli

Why do we need build-php8.3.0RC and a VERSIONS_php=8.3.0RC6 at the same time. I will have to look in the Makefile but wouldn't it be better to define the Unit Language Module you want to build in build-php if not defined anything else we are using the latest, hard-coded version from the Makefile. If there was a VERSIONS tag defined we are going to use this as the Docker base image.

My Investigations: The VERSIONS and build-php Version must match

make build-php8.2 VERSIONS_php=8.2.0
docker pull
"docker pull" requires exactly 1 argument.
See 'docker pull --help'.

Usage:  docker pull [OPTIONS] NAME[:TAG|@DIGEST]

Download an image from a registry
make: *** [build-php8.2] Error 1

using 8.2.0 in build and VERSIONS will make it work

What I think the user should be able to use

So - for me the best option would be:

make build-php VERSIONS_php=[WHATEVER Tag for php is available on hub.docker.com]

In case the use does not specify a VERSIONS - we are going to use the one set in the Makefile

>> VERSIONS_php ?=      8.2
VARIANT_php ?=      cli-$(VARIANT)
$(foreach phpversion, $(VERSIONS_php), $(eval CONTAINER_php$(phpversion) = php:$(phpversion)-$(VARIANT_php)))
CONFIGURE_php ?=    php
INSTALL_php ?=      php-install
RUN_php ?=          ldconfig
MODULE_PREBUILD_php ?= /bin/true