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.

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

Combine templates to a single file #1193

Merged
merged 2 commits into from
Aug 25, 2021
Merged

Conversation

yosifkit
Copy link
Member

I tried to keep the resulting changes to the generated Dockerfiles minimal but also minimize the template.

from="debian:$suite-slim"
fi
export from
export from alpineVer
Copy link
Member Author

Choose a reason for hiding this comment

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

I figured since we extracted it, we could use it and not re-extract it via from, but so far it was only needed once

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Generally, I like it! Just a few comments 👀

7.3/bullseye/apache/Dockerfile Outdated Show resolved Hide resolved
Dockerfile-linux.template Outdated Show resolved Hide resolved
Dockerfile-linux.template Outdated Show resolved Hide resolved
Dockerfile-linux.template Outdated Show resolved Hide resolved
# oniguruma is part of mbstring in php 7.4+
( if (.version | version_id) >= ("7.4" | version_id) then "libonig-dev" else empty end )
end )
] | sort[] | (
Copy link
Member

Choose a reason for hiding this comment

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

This block is very clever, but IMO will be more complex to maintain over time than just having the package list, especially since there are only three packages who actually share the same name between Debian and Alpine (and we have to duplicate the version checks anyways). 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind moving the "common" packages into both alpine and debian lists. What I like about it is that the list in the resulting Dockerfile is always sorted without having to manually sort the list along with {{ if .version then ( -}} packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

To put it clearly, do we want to have maintain the sort on this:

{{ if is_alpine then ( -}}
	apk add --no-cache --virtual .build-deps \
		$PHPIZE_DEPS \
		argon2-dev \
		coreutils \ 
		curl-dev \
		libedit-dev \
{{ if (.version | version_id) >= ("7.2" | version_id) then ( -}}
		libsodium-dev \
{{ ) else "" end -}}
		libxml2-dev \
{{ if (.version | version_id) >= ("7.4" | version_id) then ( -}}
		linux-headers \
{{ ) else "" end -}}
{{ if (.version | version_id) >= ("7.4" | version_id) then ( -}}
		# oniguruma is part of mbstring in php 7.4+
		oniguruma-dev \
{{ ) else "" end -}}
		openssl-dev \
		sqlite-dev \
		# https://github.com/docker-library/php/issues/888
{{ ) else ( -}}
	\
	savedAptMark="$(apt-mark showmanual)"; \
	apt-get update; \
	apt-get install -y --no-install-recommends \
		${PHP_EXTRA_BUILD_DEPS:-} \
		libargon2-dev \
		libcurl4-openssl-dev \
		libedit-dev \
{{ if (.version | version_id) >= ("7.4" | version_id then ( -}}
		# oniguruma is part of mbstring in php 7.4+
		libonig-dev \
{{ ) else "" end -}}
{{ if (.version | version_id) >= ("7.2" | version_id) then ( -}}
		libsodium-dev \
{{ ) else "" end -}}
		libsqlite3-dev \
		libssl-dev \
		libxml2-dev \
		zlib1g-dev \
{{ ) end -}}

Or this (maybe some love to reduce indentation):

{{
	[
		[
			( if is_alpine then
				"$PHPIZE_DEPS",
				"argon2-dev",
				"coreutils", 
				"curl-dev",
				"libedit-dev",
				"libxml2-dev",
				"openssl-dev",
				"sqlite-dev",
				( if (.version | version_id) >= ("7.2" | version_id) then "libsodium-dev" else empty end ),
				# https://github.com/docker-library/php/issues/888
				( if (.version | version_id) >= ("7.4" | version_id) then "linux-headers" else empty end ),
				# oniguruma is part of mbstring in php 7.4+
				( if (.version | version_id) >= ("7.4" | version_id) then "oniguruma-dev" else empty end )
			else
				# debian packages
				"${PHP_EXTRA_BUILD_DEPS:-}",
				"libargon2-dev",
				"libcurl4-openssl-dev",
				"libedit-dev",
				"libsqlite3-dev",
				"libssl-dev",
				"libxml2-dev",,
				"zlib1g-dev"
				( if (.version | version_id) >= ("7.2" | version_id) then "libsodium-dev" else empty end ),
				# oniguruma is part of mbstring in php 7.4+
				( if (.version | version_id) >= ("7.4" | version_id) then "libonig-dev" else empty end )
			end )
		] | sort[] | (
-}}
		{{ . }} \
{{
		)
	] | add
-}}
	; \

Copy link
Member

Choose a reason for hiding this comment

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

hmm, I prefer the former (I've been moving the versioned bits to the end too, if they include a comment that otherwise breaks up the package list), but I defer to your preference 😄

Dockerfile-linux.template Show resolved Hide resolved
Comment on lines +146 to +147
ENV PHP_EXTRA_BUILD_DEPS apache2-dev
ENV PHP_EXTRA_CONFIGURE_ARGS --with-apxs2 --disable-cgi
Copy link
Member

Choose a reason for hiding this comment

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

TODO push these down instead of adding extra ENV (they're quirks of the previous templating engine); I think we can do this as a later improvement so that the Dockerfile changes here stay more minimal

@tianon tianon merged commit 2047c68 into docker-library:master Aug 25, 2021
@tianon tianon deleted the combine branch August 25, 2021 22:54
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Aug 26, 2021
Changes:

- docker-library/php@2047c68: Merge pull request docker-library/php#1193 from infosiftr/combine
- docker-library/php@0630924: Apply templates using single template
- docker-library/php@b5f8bba: Combine templates to a single template to reduce duplication
- docker-library/php@ed2fe97: Merge pull request docker-library/php#1187 from infosiftr/readline
- docker-library/php@eee43f6: Switch from libedit back to readline
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Aug 26, 2021
Changes:

- docker-library/php@e81ce1c: Update 8.0 to 8.0.10
- docker-library/php@cdb853c: Update 7.3 to 7.3.30
- docker-library/php@2047c68: Merge pull request docker-library/php#1193 from infosiftr/combine
- docker-library/php@0630924: Apply templates using single template
- docker-library/php@b5f8bba: Combine templates to a single template to reduce duplication
- docker-library/php@ed2fe97: Merge pull request docker-library/php#1187 from infosiftr/readline
- docker-library/php@eee43f6: Switch from libedit back to readline
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Aug 26, 2021
Changes:

- docker-library/php@496cd2f: Update 7.4 to 7.4.23
- docker-library/php@e81ce1c: Update 8.0 to 8.0.10
- docker-library/php@cdb853c: Update 7.3 to 7.3.30
- docker-library/php@2047c68: Merge pull request docker-library/php#1193 from infosiftr/combine
- docker-library/php@0630924: Apply templates using single template
- docker-library/php@b5f8bba: Combine templates to a single template to reduce duplication
- docker-library/php@ed2fe97: Merge pull request docker-library/php#1187 from infosiftr/readline
- docker-library/php@eee43f6: Switch from libedit back to readline
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.

None yet

2 participants