search

ocaml / opam-repository

Main public package repository for opam, the source package manager of OCaml.
https://opam.ocaml.org
Creative Commons Zero v1.0 Universal
516 stars 1.12k forks source link

OCaml compiler patches for GCC 14 #26177

Closed mtelvers closed 3 months ago

mtelvers commented 3 months ago

With the release of GCC 14, which is included in Fedora 40 and the latest Arch Linux releases, we are seeing a number of failures to build older releases of OCaml, specifically < 4.08. See https://github.com/ocurrent/docker-base-images/issues/279. The configuration scripts for the compiler prior to 4.08 test for the presence of various library functions by attempting to compile minimal C programs using these functions. These test builds now fail because GCC 14 makes implicit function declarations an error rather than a warning. The additional compiler flag -Wno-implicit-function-declaration is needed.

This is not the first example of a patch being necessary to support the old compiler. Previously, we have had the -fno-common patch required for GCC 10 compatibility.

Working on this issue with @dra27, and accepting that future patches will be required to maintain the older compilers, this PR set a precedence for holding compiler patches in ocaml-opam/ocaml using the branches x.yy-patches.

The first commit updates the existing GCC 10 patch into this format, and the second commit adds the GCC 14 patch.

kit-ty-kate commented 3 months ago

Closing temporarily to give a chance for other PRs to use the CI

mseri commented 3 months ago

@dra27 why can't we keep the patches in the opam-source-archive with the other ones instead of linking a fork of the comal compiler? Unless we can link the official ocaml repository, I think it would be better to reduce the changes uploading fix-gcc14.patch on the opam-source-archive and link it from there, like with do with the gcc10 patch

kit-ty-kate commented 3 months ago

@mseri feel free to reopen whenever

dra27 commented 3 months ago

opam-source-archive was a hack we created to deal with disappearing upstream archives - it's not a principled place to keep the primary sources of actual code/patches.

This is the third of these major compiler/c-library changes:

There are more patches stacked on the older compiled (restore by me in #18240 and the intention at some point (it's not exactly high priority!) is to stack those patches in the same, principled way, rather than as a random collection of patch files.

mseri commented 3 months ago

Thanks for the clarification. Sounds reasonable then. Is there no way to have these in alternative branches of the main compiler repository instead?

mseri commented 3 months ago

In any case, I am fine with this suggestion. I suggest that once this is ready, you open the PR and as soon as the linter is fine, we merge it straight away.

dra27 commented 3 months ago

I was guessing that my core dev colleagues wouldn't be too thrilled, but I hadn't actually asked😊

@Octachron - what do you think? The context here would be carrying the *-patches branches from https://github.com/ocaml-opam/ocaml in https://github.com/ocaml/ocaml instead?

dra27 commented 3 months ago

In general, I guess it means that for every release branch, there potentially ends up being a release-patches branch.

Octachron commented 3 months ago

In principle, I would be rather in favour to have the release-patches available on the main git repository since this is also useful for compiler developers to not have to hunt compatibility patches when investigating a regression/bug/behavior difference in old versions.

dra27 commented 3 months ago

Can I just check I understand correctly - this would be, say, when bisecting and needing the patches, right? The tips of the release branches on ocaml/ocaml do already (mostly) hold these fixes?

dra27 commented 3 months ago

(that said, I'm certainly not against these extra branches being carried on ocaml/ocaml!)

Octachron commented 3 months ago

The tips of the release branches tend to also contain CI fixes or bug fixes that might be useful in later patch release. I was agreeing with the idea of having branches that only contains the release-patches (in contiguous commits).

shonfeder commented 1 week ago

Just circling back here: has it been decided to put this into https://githup.com/ocaml/ocaml? If so, do we need/have a tracking issue or PR for that? (I haven't found one after a quick search).

dra27 commented 1 week ago

This one's cooking for a bit longer - the broad agreement was to put the patches into ocaml/ocaml, but there is to be churn on compiler packages when 4.08-4.12 are updated for Windows (which should be before the end of the calendar year), so it seems better to put this update in at that point. The base image builder is working around this in https://github.com/ocurrent/docker-base-images/pull/298.

shonfeder commented 1 week ago

Sounds good! I didn't mean to rush things, but just wanted to make sure that this is tracked in the correct place, since this PR is closed and the original issue https://github.com/ocurrent/docker-base-images/issues/279 is arguably resolved for the base image builder by https://github.com/ocurrent/docker-base-images/pull/298. IIUC, this is something that should probably be tracked in ocaml/ocaml now.