mstorsjo / llvm-mingw

An LLVM/Clang/LLD based mingw-w64 toolchain
Other
1.75k stars 176 forks source link

docker: update the base image to ubuntu 22.04? #403

Closed naveen521kk closed 4 months ago

naveen521kk commented 4 months ago

I'm working on https://github.com/msys2-contrib/cpython-mingw/pull/165 and Python 3.12 requires autoconf 2.71 or higher to build. It's not directly available through apt though.

That's the reason why I think it would be better to update the base image to Ubuntu 22.04. Probably, other users may be interested in new versions of things too. Are there any objections to it, if not I'll open a PR. Thanks!

mstorsjo commented 4 months ago

Oh, so you're using the the llvm-mingw docker images for this? Interesting, I wasn't aware of much use of the docker images.

Before switching the release building over to github actions, I used the docker images for making the release tarballs as well - during that period, I wanted to keep the docker images at the oldest possible base image, so that the built tarball would run on as many different versions as possible. As the release building is decoupled from that now, I guess there's less limitations on what base version we use in the images - and users of the images may benefit from newer software in the images. (That's also why we've earlier installed e.g. a newer version of git in the container, see 277b8c4c27ad2eb3ce6db00f735e9399cc71cd4d.)

I've already had to work around autoconf 2.71 for libffi, see a711bb4ed07ad4fa3c5da6875e7f7c11747a0050 - but that's not part of the images that are released.

Thanks for offering to open a PR, but I went ahead and started testing this out, see the topmost two commits at https://github.com/mstorsjo/llvm-mingw/commits/docker-ubuntu-22.04; the second one should do this change, and the topmost one is experimental, trying to test out Dockerfile.cross (which no longer really is used in the current release flow, but I want to keep it functional still).

mati865 commented 4 months ago

Alternatively CI could build images with two bases, similarly to how Rust prepares images with different Debian bases. Don't know if it's worth the effort though.

naveen521kk commented 4 months ago

Thanks for offering to open a PR, but I went ahead and started testing this out, see the topmost two commits at https://github.com/mstorsjo/llvm-mingw/commits/docker-ubuntu-22.04; the second one should do this change, and the topmost one is experimental, trying to test out Dockerfile.cross (which no longer really is used in the current release flow, but I want to keep it functional still).

Looks good, thanks! Let me know if I can help anywhere.

mstorsjo commented 4 months ago

Alternatively CI could build images with two bases, similarly to how Rust prepares images with different Debian bases. Don't know if it's worth the effort though.

Yeah, I don't think it's worth the effort here - I wouldn't market the docker image as the primary way to use the toolchain in any case, but I guess it's a nice extra convenience in some cases. (Originally, I used docker more as a way to reproducibly build things, so that I could be sure that whatever I did, was possible to build for someone else as well.)

Looks good, thanks! Let me know if I can help anywhere.

Thanks for offering - I don't think there's much more to do here. I started another test build, as I realized we should bump Dockerfile.toolchain similarly as well.

As a side note, I'm not entirely satisfied with the amount of hackery that has to be done in docker.yml to do a manual test run like this, though. The design so far, is that I'd run this as a manual job, after building a regular release, pointing it at a commit with a previous pipeline.

But for testing like this, I'd essentially like to run the pipeline on push, not on workflow_dispatch, but I have references to inputs in a number of places, e.g. ${{inputs.push}}. So ideally, I'd like to have the least amount of modifications, for hardcoding the values for inputs. And for checkout, I normally have with: ref: ${{ (inputs.commit != '' && inputs.commit) || inputs.branch }}, i.e. make it check out whatever other branch/commit I point it at. But for this kind of testing, I obviously want the default (just check out the current commit), and I'm not sure how easy it is to adapt to that, other than just removing the with: ref: in the testing commit.

mstorsjo commented 4 months ago

I pushed this change now, as 373c32f91a62bcb8b726728f3c45929bb7dafa02. The next LLVM 18.1.0 release candidate is planned to be made today (practically speaking, tomorrow European time), so if everything goes well, the images built then should be based on the new version.

mstorsjo commented 4 months ago

The new release is out now, and the latest version of the docker image, with LLVM 18.1.0 RC 3, is now out, based on Ubuntu 22.04.

mstorsjo commented 4 months ago

Actually, the new multi-platform images didn’t work out as expected, so the current latest image doesn’t work on x86_64, see #405 and #399. I’ll try to fix it when I get a chance.

dtretyakov commented 4 months ago

@mstorsjo I suspect that there is a typo on the following line: https://github.com/mstorsjo/llvm-mingw/blob/master/.github/workflows/docker.yml#L105

it should be like that:

platforms: linux/amd64,linux/arm64
mstorsjo commented 4 months ago

@mstorsjo I suspect that there is a typo on the following line: https://github.com/mstorsjo/llvm-mingw/blob/master/.github/workflows/docker.yml#L105

it should be like that:

platforms: linux/amd64,linux/arm64

No, that's not a typo. See when I reworked your branch, I mentioned this here: https://github.com/mstorsjo/llvm-mingw/pull/399#issuecomment-1934928397

Additionally, I tested a few other modifications. While we can create the regular x86_64 docker image this way, I'd prefer to keep producing it with the full dockerfile like we used to. If you don't mind, I would prefer to update this PR with those changes; see the changes in https://github.com/mstorsjo/llvm-mingw/compare/master...9a402d8af586fe184aaee0183f2d83fc3f04127f.

So this is what breaks things; when we're not building both platforms in the same step, but push them separately in two steps, I had expected that both images for each different platform would be added, but instead the last push overwrites the earlier ones, discarding whatever platforms were supported in the earlier push. I guess this makes sense though.

I'll try to wrap my head around how to merge images built in separate steps, as outlined in https://docs.docker.com/build/ci/github-actions/multi-platform/#distribute-build-across-multiple-runners, if I can do that with a reasonable amount of effort, otherwise I'll revert to your suggestion and build both platforms in that one job.

mstorsjo commented 4 months ago

I fixed the issue for now with https://github.com/mstorsjo/llvm-mingw/commit/608919ce4593b99f73b97a3fd0679f31f0c595ea - I didn't merge it to master yet, but I'll try to see if I have time to figure out merging separately pushed images by the time of the next release or if I'll go with this.

mstorsjo commented 4 months ago

I fixed the issue for now with 608919c - I didn't merge it to master yet, but I'll try to see if I have time to figure out merging separately pushed images by the time of the next release or if I'll go with this.

FYI, I did set up building/merging of a multiplatform image by building/pushing the different platforms separately, and then creating a tag that points to both of them - see https://github.com/mstorsjo/llvm-mingw/commit/8f421127194ea0ca2325bbe6648a783612718619. It's kinda complex, but retains the intent I had, that the amd64 version of the image still is a regular build of Dockerfile. (Not that it should matter much, the tarball built and used in Dockerfile.toolchain should be almost as good. When built in the docker image, it gets built with whatever newer system compiler is used in the ubuntu 22.04 base image, instead of the baseline ubuntu 20.04 used for the tarballs.) But if it becomes more complex at some point, I might just as well switch over to doing all architectures with Dockerfile.toolchain.

That's the reason why I think it would be better to update the base image to Ubuntu 22.04.

In any case, this main objective of this issue has been resolved now, so I'll close this issue.