Install system packages required by OCaml 5.1+ in the ocaml stage

[MisterDA]

Different releases of OCaml require different system libraries: libX11 isn't needed since 4.09, when the Graphics library was moved from the core distribution to a separate package. On the opposite side, OCaml 5.1 can optionally use libzstd for compressing marshalled data. Install these packages accordingly in the ocaml image.

[MisterDA]

Now based on top of #153.

[kit-ty-kate]

That sounds good in principle but I'm not sure why the dev_packages function wasn't changed instead of creating a new function that users have to call manually. Is there a downside to adding a new mandatory parameter to the dev_packages function? Is this just to allow optional dependencies to be decided by the user (if so I think the function name could be more explicit about that). I would personally be more in favour of simply adding a new mandatory parameter to dev_packages and enforce a set of packages in an opinionated way.

[MisterDA]

The goal is really to avoid installing unneeded packages, like libx11 and all its dependencies in the shared opam image. We're going to have to call a function anyway when creating the ocaml stage.

With this new function, the patch to docker-base-images is trivial:
https://github.com/ocurrent/docker-base-images/pull/213/files#diff-5f0eea7390672da3c255b6d2e394932d240c9129a0db3eb95df34ed94a098a43

I think there would be two new parameters: one indicating whether we're in the opam or ocaml (installing the ocaml switch) stage, and the other the ocaml version (the name of the switch). IMO that's better expressed with a new function, but maybe the name could be better. Perhaps ocaml_dev_depexts, or ocaml_opt_packages?

[MisterDA]

hmm, passing the switch in an option would work too, I'll try that.
Thanks for the idea!

[kit-ty-kate]

i was more thinking of something like this

Suggested change

	match version with
	| None ->
	install
	"sudo passwd bzip2 unzip patch rsync nano gcc-c++ git tar curl xz \
	which m4 diffutils findutils%s"
	extra
	| Some v ->
	if Ocaml_version.compare v Ocaml_version.Releases.v4_08 <= 0 then
	install "libX11-devel%s" extra
	else if Ocaml_version.compare v Ocaml_version.Releases.v5_1_0 >= 0 then
	install "zstd%s" extra
	else empty
	install
	"sudo passwd bzip2 unzip patch rsync nano gcc-c++ git tar curl xz \
	which m4 diffutils findutils%s%s"
	extra
	(if Ocaml_version.compare v Ocaml_version.Releases.v4_08 <= 0
	then "libX11-devel"
	else if Ocaml_version.compare v Ocaml_version.Releases.v5_1_0 >= 0
	then "zstd"
	else ""

[MisterDA]

I see, but I want for instance to remove libx11 from ocaml/opam:alpine-3.17-opam:

	RUN apk update && apk upgrade
-	RUN apk add build-base patch tar ca-certificates git rsync curl sudo bash libx11-dev nano coreutils xz ncurses-dev bubblewrap
+	RUN apk add build-base patch tar ca-certificates git rsync curl sudo bash nano coreutils xz ncurses-dev bubblewrap
	COPY --from=0 [ "/usr/local/bin/opam-2.0", "/usr/bin/opam-2.0" ]
	RUN ln /usr/bin/opam-2.0 /usr/bin/opam
	COPY --from=0 [ "/usr/local/bin/opam-2.1", "/usr/bin/opam-2.1" ]

and have it installed as part of ocaml/opam:alpine-3.17-ocaml-4.02 to ocaml/opam:alpine-3.17-ocaml-4.07:

	# syntax=docker/dockerfile:1
	FROM ocurrent/opam-staging:alpine-3.17-opam-amd64
	ENV OPAMYES="1" OPAMCONFIRMLEVEL="unsafe-yes" OPAMERRLOGLEN="0" OPAMPRECISETRACKING="1"
+	RUN apk update && apk upgrade
+	RUN apk add libX11-dev
	RUN opam switch create 4.02 --packages=ocaml-base-compiler.4.02.3
	RUN opam pin add -k version ocaml-base-compiler 4.02.3
	RUN opam install -y opam-depext

The function dev_packages is called when building the first stage, so we can't test the ocaml version at this step. We could reinstall all the packages in the second step but that would be wasted work.
Am I missing something?

[MisterDA]

I'm also supposing that libx11 and zstd are not needed when bootstrapping opam.

[MisterDA]

As @kit-ty-kate noticed privately, removing libx11 from the -opam images might break workflows of people using them, especially for older versions of OCaml, so I've (for now) constrained this change to only installing zstd on -ocaml-5.1 (and onwards) images.
I've also changed the name of the original function to ocaml_depexts.
I'll squash before merging.
Thoughts?

[kit-ty-kate]

On Alpine the name of packages are case sensitive

Suggested change

	coreutils xz ncurses-dev libX11-dev%s"
	coreutils xz ncurses-dev libx11-dev%s"

I would also suggest to keep the same ordering as before to limit the diff and avoid such issues elsewhere

[MisterDA]

Thanks for the reviews!