GEOS-DEV / thirdPartyLibs

Repository to build the GEOSX third party libraries
3 stars 12 forks source link

fix `TPL` builds with `clang >= 15` #260

Closed tbeltzun closed 8 months ago

tbeltzun commented 9 months ago

As discussed on slack, this PR:

Reference https://www.redhat.com/en/blog/new-warnings-and-errors-clang-16.

TODO: we could probably move installing dependencies in Dockerfile to a common shell script ...

tbeltzun commented 9 months ago

Thanks, done in https://github.com/GEOS-DEV/GEOS/pull/3024.

Checking also on Pangea III (=> no issue with this PR).

tbeltzun commented 8 months ago

@CusiniM, requesting your review again, since lots of changes were made in order to implement https://github.com/GEOS-DEV/GEOS/pull/3024#issuecomment-1989539601.

GEOS build validated in https://github.com/GEOS-DEV/GEOS/pull/3024: https://github.com/GEOS-DEV/GEOS/actions/runs/8262526620.

tbeltzun commented 8 months ago

Now that you have been through this part of the CI, do you have a REX (a few bullet points ) of what we could improve? And maybe points that you think were nice as well?

I have very limited experience with docker, hence the issues encountered were only due to the docker build system.

I tried to reduce code duplication by writing common scripts for docker images, and I think this is something we should tend to, as it is easier for maintenance and when someone discovers the build scripts.

TotoGaz commented 8 months ago

Yes, we had some duplications when installing tools like sccache. It's good that you were able to fix this. Now, the global pattern is still duplicated accros the Dockerfiles, and some package names as well. In the very very very beginning we had a pattern with a lot of scripts, allowing not to duplicated "anything". But in the end, docker being what it is (i.e. not real programming language), the situation was extremely complicated to handle and understand. I believe that the current situation is a not-so-bad compromise given the size of the project. 🤷