V3.0.3 add debian13 and opensuse16 builds#5141
Conversation
…n/proxysql into v3.0.3-add_centos10_builds
…n/proxysql into v3.0.3-add_centos10_builds
Summary of ChangesHello @mirostauder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the project's build compatibility by introducing support for several newer operating system versions, including Debian 13, openSUSE 16, AlmaLinux 10, CentOS 10, and Fedora 42. It also modernizes key dependency management by upgrading libconfig and PostgreSQL versions, and transitioning libconfig's build process to CMake. Furthermore, a specific patch was added to ensure the MariaDB client library compiles correctly with contemporary compilers, reinforcing overall build stability and future-proofing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
There was a problem hiding this comment.
Code Review
This pull request adds build configurations for Debian 13, openSUSE 16, and other new OS versions. The changes look good and are consistent with the existing structure. I've added a few suggestions to improve the maintainability and readability of the Makefiles and docker-compose.yml, mainly by reducing code duplication and splitting long lines.
| amd64-debian: debian12 debian12-clang debian12-dbg | ||
| amd64-fedora: fedora40 fedora40-clang fedora40-dbg fedora41 fedora41-clang fedora41-dbg | ||
| amd64-opensuse: opensuse15 opensuse15-clang opensuse15-dbg | ||
| amd64-almalinux: almalinux8 almalinux8-clang almalinux8-dbg almalinux9 almalinux9-clang almalinux9-dbg almalinux10 almalinux10-clang almalinux10-dbg |
There was a problem hiding this comment.
This line is very long, which harms readability. Consider splitting it across multiple lines using \ for better maintainability.
amd64-almalinux: almalinux8 almalinux8-clang almalinux8-dbg \
almalinux9 almalinux9-clang almalinux9-dbg \
almalinux10 almalinux10-clang almalinux10-dbg
| amd64-almalinux: almalinux8 almalinux8-clang almalinux8-dbg almalinux9 almalinux9-clang almalinux9-dbg almalinux10 almalinux10-clang almalinux10-dbg | ||
| amd64-centos: centos9 centos9-clang centos9-dbg centos10 centos10-clang centos10-dbg | ||
| amd64-debian: debian12 debian12-clang debian12-dbg debian13 debian13-clang debian13-dbg | ||
| amd64-fedora: fedora40 fedora40-clang fedora40-dbg fedora41 fedora41-clang fedora41-dbg fedora42 fedora42-clang fedora42-dbg |
There was a problem hiding this comment.
This line is very long, which harms readability. Consider splitting it across multiple lines using \ for better maintainability.
amd64-fedora: fedora40 fedora40-clang fedora40-dbg \
fedora41 fedora41-clang fedora41-dbg \
fedora42 fedora42-clang fedora42-dbg
| cd postgresql && rm -rf postgresql-*/ || true | ||
| cd postgresql && rm -rf postgres-*/ || true |
There was a problem hiding this comment.
These two rm commands can be combined into a single line for conciseness by passing both directory patterns to rm.
cd postgresql && rm -rf postgresql-*/ postgres-*/ || true
| cd postgresql && rm -rf postgresql-*/ || true | ||
| cd postgresql && rm -rf postgres-*/ || true |
There was a problem hiding this comment.
These two rm commands can be combined into a single line for conciseness by passing both directory patterns to rm.
cd postgresql && rm -rf postgresql-*/ postgres-*/ || true
| centos10_build: | ||
| extends: | ||
| service: _build | ||
| image: proxysql/packaging:build-centos10-v3.0.3 | ||
| volumes: | ||
| - ./docker/images/proxysql/rhel-compliant/rpmmacros/rpmbuild/:/root/rpmbuild/ | ||
| - ./docker/images/proxysql/rhel-compliant/rpmmacros/.rpmmacros:/root/.rpmmacros | ||
| - ./docker/images/proxysql/rhel-compliant/entrypoint/:/opt/entrypoint/ | ||
| - ./:/opt/proxysql/ | ||
| environment: | ||
| - PKG_RELEASE=centos10 | ||
| - PROXYSQL_BUILD_TYPE=clickhouse | ||
|
|
||
| centos10_clang_build: | ||
| extends: | ||
| service: centos10_build | ||
| image: proxysql/packaging:build-clang-centos10-v3.0.3 | ||
| environment: | ||
| - PKG_RELEASE=centos10-clang | ||
|
|
||
| centos10_dbg_build: | ||
| extends: | ||
| service: centos10_build | ||
| environment: | ||
| - PKG_RELEASE=dbg-centos10 | ||
| - PROXYSQL_BUILD_TYPE=debug | ||
|
|
There was a problem hiding this comment.
This docker-compose.yml file has a lot of duplication. The new services for centos10, fedora42, debian13, opensuse16, and almalinux10 are large blocks of mostly repeated configuration. This makes the file hard to maintain.
To improve this, you could define abstract base services for each OS family (rhel-compliant, deb-compliant, suse-compliant) and use extends to inherit common configuration. This would significantly reduce duplication and make future updates easier.
For example, you could define a _rhel_build service with the common volumes and environment variables:
_rhel_build:
extends:
service: _build
volumes:
- ./docker/images/proxysql/rhel-compliant/rpmmacros/rpmbuild/:/root/rpmbuild/
- ./docker/images/proxysql/rhel-compliant/rpmmacros/.rpmmacros:/root/.rpmmacros
- ./docker/images/proxysql/rhel-compliant/entrypoint/:/opt/entrypoint/
- ./:/opt/proxysql/
environment:
- PROXYSQL_BUILD_TYPE=clickhouseThen centos10_build could be simplified to:
centos10_build:
extends:
service: _rhel_build
image: proxysql/packaging:build-centos10-v3.0.3
environment:
- PKG_RELEASE=centos10This pattern can be applied to all rhel-compliant, deb-compliant, and suse-compliant build services to make the file much more concise and maintainable.
builds on top of PR #5139
adds debian13 builds
adds opensuse16 builds