Docker: Unable to build images for PHP 8.3.0RC6 using packages Makefile about unit HOT 17 OPEN

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

from unit.

Yes with a high chance because of this

from unit.

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.

from unit.

from unit.

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.

from unit.

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

from unit.

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.

from unit.

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

from unit.

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

from unit.

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

from unit.

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

from unit.

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...

from unit.

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

from unit.

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

from unit.

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.

from unit.

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 <[email protected]> (@nginx)"
        @echo "GitRepo: https://github.com/nginx/unit.git"

from unit.

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

from unit.