crystal-lang / distribution-scripts

40 stars 24 forks source link

Add alpine.Dockerfile and refactor docker Makefile #29

Closed straight-shoota closed 4 years ago

straight-shoota commented 5 years ago

With https://github.com/crystal-lang/crystal/pull/7479 merged, the generic linux binaries with gnu default target also work on Alpine linux with musl. So we can use the tar.gz created in the linux build to provide an alpine-based docker image.

Benefits:

RX14 commented 5 years ago

shouldn't you need to rebuild libcrystal.a?

straight-shoota commented 5 years ago

@RX14 No, why? It's included in the tar.gz and should be portable on linux systems. It doesn't matter which libc it will eventually be linked against.

ysbaddaden commented 5 years ago

You should recompile it nonetheless, I doubt it's really portable —otherwise we wouldn't have distinct bindings for linux-gnu and linux-musl.

straight-shoota commented 5 years ago

If it's not portable, should we distribute it with the generic linux package? And what about the other included binary libraries, libgc.a and llvm_ext.o?

straight-shoota commented 5 years ago

I just verified: libcrystal.a is exactly the same, whether it's build on alpine or ubuntu. Same goes for llvm_ext.o.

ysbaddaden commented 5 years ago

That's the thing: the "generic" linux package isn't generic but targets linux-gnu.

The provided libgc.a is built against glibc, same for libcrystal.a (I'm not sure llvm_ext.o should be distributed). If a built libcrystal.a happens to be identical for both linux-gnu and linux-musl, then it's just luck, without any guarantee that it will stay that way.

straight-shoota commented 5 years ago

Hm okay, so I'll make sure to build them locally, in order to make sure it won't break at some point.

It's not super trivial though, because the Makefile is not included in the binary package for make clean deps. Otherwise we need to copy it separately into the docker image, or duplicate the instructions.

libgc.a also requires a custom patch. But it's already available for the linux build instructions. EDIT: It seems the patch is only required for gnu.

An alternative would be to build the binary libs for musl in the linux build and either include them into the generic linux tar.gz (beside the gnu ones) or provide two separate downloads for musl and gnu.

j8r commented 5 years ago

I don't know why a libgc.a is shipped – the libgc.a that should be used is the one of the OS – problem solved :)

j8r commented 5 years ago

For libcrystal.a, I agree with @ysbaddaden. However I doubt there is a real need for a dedicated archive for musl. Usually people on musl are on Alpine or Gentoo, and most through Docker. This OSes are usually up-to-date, they use the package manager to get a ~more/less up-to-date Crystal release.

What I hear the most is people waiting an Alpine Docker image with the latest Crystal release. Let's do so: we can build Crystal directly in the Dockerfile.

straight-shoota commented 5 years ago

We want libgc with --enable-large-config and AFAIK gc-dev doesn't provide that. That's why we need to build it ourselves.

Let's do so: we can build Crystal directly in the Dockerfile.

We do. It's just a different dockerfile. I don't think there would be any benefit from duplicating the build.

j8r commented 5 years ago

I mean, building Crystal on the same OS as the target – it can be dynamically linked.

straight-shoota commented 5 years ago

But why? We need a somewhat recent compiler build anyway in order to that. So we can just use the most recent one and use it without recompiling, because it already works.

bcardiff commented 5 years ago

https://github.com/crystal-lang/distribution-scripts/pull/29#issuecomment-471044881 I don't know why a libgc.a is shipped – the libgc.a that should be used is the one of the OS – problem solved :)

We do need to ship the libgc.a. Shortly we will probably end up linking it statically by default since we will need some custom patch to MT ;-)


The files in the ./docker/ folder should only include the package/installer made in ./linux/. If the .tar.gz is able to run in alpine directly then something more like the first commit only is better.

If we can/want to produce a different downloadable asset in the release for alpine that will be available in the github releases, then we should add that process in the ./linux/ and package it in the ./docker.

bcardiff commented 5 years ago

Closed in favor of #44

straight-shoota commented 5 years ago

44 only updates the Dockerfile for building the compiler on alpine. It doesn't provide a Dockerfile for running Crystal on alpine.

These use cases are similar, maybe we can use the same Dockerfile. I'm not sure. There are still different goals and you don't need the entire build environment for a runtime container. And of course we'd need to make the Dockerfile available in the docker build workflow.

straight-shoota commented 4 years ago

So to move forward with this PR, we could resolve the static libraries issues:

Alternatively, we could build upon the linux build Dockerfile from #44. The last steps for packaging the targz would need to be moved to a separate build stage. But then we could use the main part of it as build image, just need to put the crystal files in the appropriate folders in /usr to make it usable. A slimmer runtime image could probably be created by uninstalling the build dependencies specific to the compiler (llvm, maybe some of the build tools).

What do you think?

straight-shoota commented 4 years ago

I've updated this branch to install gc-dev from apk and build libcrystal.a and llvm_ext.o using the Makefile which is now included in the tarball.

bcardiff commented 4 years ago

Cool, before merging there needs to be a branch named ci/update or something that will trigger the maintenance build.

The publish_docker job should be updated to push the -alpine docker image.

Once that is there, we can merge this and update the ci/update with the new master sha1 of distribution-scripts.

straight-shoota commented 4 years ago

Workflow looks good: https://circleci.com/workflow-run/7384ac99-a207-4a98-8c21-038e0710f6c0

Docker images can be tested:

bcardiff commented 4 years ago

@straight-shoota if you can squash/rebase the ci/test branch with the new sha in the main repo we can merge that.

bcardiff commented 4 years ago

@straight-shoota I see you updated the ci/test branch but I did not find a PR with it. Did I miss it?

straight-shoota commented 4 years ago

Seems like I forgot the PR. Now it's here: crystal-lang/crystal#8708