-
Notifications
You must be signed in to change notification settings - Fork 1k
V3.0 add centos10 and almalinux10 builds #4970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @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!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request.
This pull request primarily focuses on adding support for building ProxySQL on newer Linux distributions, specifically CentOS Stream 10 and AlmaLinux 10. It also includes updates to support Fedora 42 builds. To achieve this, several dependencies were updated or patched, and the build infrastructure (Makefiles and Docker Compose configurations) was adjusted to accommodate these new environments and dependency changes.
Highlights
- New Build Environments: Added build targets and Docker Compose services for CentOS Stream 10, AlmaLinux 10, and Fedora 42, including standard, clang, and debug variants for amd64 and arm64 architectures where applicable.
- Dependency Updates and Patches: Updated dependency versions (ClickHouse C++, libconfig, PostgreSQL) and added new patches (
fix_ciso646.patch
for ClickHouse C++,fix_bool.patch
for MariaDB Connector/C) to ensure compatibility with newer build environments and compilers. - Standardized Dependency Builds: Modified dependency build steps in
deps/Makefile
to use version-agnostic tar commands (*.tar.gz
) and standardized symbolic link creation (ln -fsT
). - Makefile Refactoring: Refactored dependency path variables and usage in
lib/Makefile
,src/Makefile
, andtest/tap/tests/Makefile
for better organization and consistency. - Build System Changes: Switched the build system for the
libconfig
dependency from autoconf (configure
) to CMake.
Changelog
Click here to see the changelog
- Makefile
- Added build targets for CentOS 10, AlmaLinux 10, and Fedora 42 for both amd64 and arm64 architectures.
- deps/Makefile
- Moved OpenSSL include directives.
- Updated tar commands for several dependencies (libinjection, libdaemon, jemalloc, mariadb-connector-c) to use version wildcards (
*.tar.gz
). - Updated ClickHouse C++ build target name from
libclickhouse-cpp-lib-static.a
tolibclickhouse-cpp-lib.a
. - Changed libconfig build process from
./configure
tocmake
. - Updated libconfig output path from
lib/.libs
toout
. - Added
ln -fsT
step for clickhouse-cpp, libconfig, and postgresql dependencies. - Replaced
wire_format.patch
withfix_ciso646.patch
for ClickHouse C++. - Added
fix_bool.patch
for MariaDB Connector/C. - Corrected cleanup command wildcard for clickhouse-cpp.
- deps/clickhouse-cpp/clickhouse-cpp
- Updated version marker to
clickhouse-cpp-2.5.1/
.
- Updated version marker to
- deps/clickhouse-cpp/fix_ciso646.patch
- Added new patch to conditionally include
<version>
or<ciso646>
in ClickHouse C++ absl dependency.
- Added new patch to conditionally include
- deps/clickhouse-cpp/wire_format.patch
- Removed old patch file.
- deps/libconfig/libconfig
- Updated version marker to
libconfig-1.8/
.
- Updated version marker to
- deps/mariadb-client-library/fix_bool.patch
- Added new patch to rename
bool
typedef tobbool
in MariaDB Connector/C to avoid conflicts.
- Added new patch to rename
- deps/postgresql/postgresql
- Updated version marker to
postgresql-16.9/
.
- Updated version marker to
- docker-compose.yml
- Added new service definitions for CentOS 10, AlmaLinux 10, and Fedora 42 builds (standard, clang, dbg).
- include/btree.h
- Added
#include <cstdint>
.
- Added
- lib/Makefile
- Standardized and expanded dependency path variables (IDIRs, LDIRs).
- Updated references to dependency paths to use the new variables.
- Updated clickhouse-cpp library name reference in linker command.
- Updated sqlite3 path reference.
- src/Makefile
- Standardized and expanded dependency path variables (IDIRs, LDIRs).
- Updated references to dependency paths to use the new variables.
- Updated clickhouse-cpp library name reference in linker command.
- Updated libconfig output path reference.
- Updated microhttpd path variables.
- Renamed postgresql path variables.
- test/tap/tests/Makefile
- Standardized and expanded dependency path variables (IDIRs, LDIRs).
- Updated references to dependency paths to use the new variables.
- Updated mysql connector export path variables.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
New systems arrive,
CentOS and Alma thrive,
Builds now take flight.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively adds build support for CentOS 10, AlmaLinux 10, and Fedora 42, which is a valuable expansion of platform compatibility. The changes also include updates to several key dependencies (ClickHouse CPP, libconfig, PostgreSQL) and introduce necessary patches for them. The Makefiles across deps
, lib
, src
, and test/tap/tests
have been refactored for better consistency in path definitions, and the build process for libconfig
has been updated to use CMake.
Overall, the changes are well-structured and aim to improve maintainability and compatibility. I have a couple of questions regarding the libconfig
build change and modifications in src/Makefile
's include/link paths that would be good to clarify.
Summary of Findings
- Libconfig Build System Change: The build system for the
libconfig
dependency was changed from autotools to CMake. While this can be an improvement, it's important to ensure that the new build process produces a library that is functionally equivalent for ProxySQL's needs across all supported platforms. (Commented) - src/Makefile Include/Link Path Modifications: Modifications were made to
IDIRS
(addedLIBUSUAL_IDIR
) andLDIRS
(addedSQLITE3_LDIR
) insrc/Makefile
. Clarification on the necessity and impact of these changes would be beneficial. (Commented) - Wildcard Tarball Extraction in deps/Makefile: Several dependencies now use wildcards (e.g.,
libname-*.tar.gz
) for tarball extraction. This improves flexibility for version bumps but relies on the assumption that only one matching tarball exists in the directory to avoid ambiguity. This is a common and generally acceptable practice if the download/preparation process ensures this condition. (Not commented due to review settings - low severity) - Makefile Formatting: Some Makefiles had minor formatting changes, such as the addition of blank lines. These are stylistic and do not affect functionality. (Not commented due to review settings - low severity)
- Dependency Updates: Versions for
clickhouse-cpp
,libconfig
, andpostgresql
have been updated. This is generally good for keeping dependencies current but requires careful testing. The PR includes new patches which likely address issues found with these newer versions. - Makefile Path Variable Refactoring: Path variables (
*_IDIR
,*_LDIR
) in various Makefiles have been refactored for consistency. This improves readability and maintainability.
Merge Readiness
This pull request makes good progress in expanding OS compatibility and updating dependencies. The Makefile refactorings are also a positive step towards better maintainability.
Before merging, I recommend addressing the questions raised in the review comments, particularly regarding the libconfig
build system change and the modifications to src/Makefile
's include and library paths. Ensuring these changes are well understood and thoroughly tested will help maintain the stability of the build process across all supported environments.
As an AI, I am not authorized to approve pull requests. Please ensure further review and approval from other team members before merging.
cd libconfig && rm -rf libconfig-*/ || true | ||
cd libconfig && tar -zxf libconfig-*.tar.gz | ||
cd libconfig/libconfig && ./configure --disable-examples | ||
cd libconfig && ln -fsT libconfig-*/ libconfig | ||
cd libconfig/libconfig && cmake -DBUILD_SHARED_LIBS=0 -DBUILD_EXAMPLES=0 -DBUILD_TESTS=0 -DBUILD_FUZZERS=0 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build system for libconfig
has been changed from autotools (./configure
) to CMake. This is a significant modification for a dependency.
While the CMake flags (-DBUILD_SHARED_LIBS=0 -DBUILD_EXAMPLES=0 -DBUILD_TESTS=0 -DBUILD_FUZZERS=0
) seem to correctly disable optional components, could you confirm if this new CMake-based build has been thoroughly tested across different environments ProxySQL supports?
Specifically:
- Are there any features enabled by the previous
./configure
script (implicitly or explicitly) that might be missed by the new CMake setup, which could affect ProxySQL's usage oflibconfig
? - Is the resulting
libconfig++.a
functionally equivalent for ProxySQL's needs compared to the one built with autotools?
IDIRS := -I$(PROXYSQL_IDIR) -I$(JEMALLOC_IDIR) -I$(MARIADB_IDIR) -I$(LIBCONFIG_IDIR) -I$(LIBDAEMON_IDIR) -I$(RE2_IDIR) -L$(PCRE_IDIR) -I$(MICROHTTPD_IDIR) -I$(LIBHTTPSERVER_IDIR) -I$(LIBINJECTION_IDIR) -I$(CURL_IDIR) -I$(EV_IDIR) -I$(PROMETHEUS_IDIR) -I$(POSTGRES_IDIR) -I$(LIBSCRAM_IDIR) -I$(LIBUSUAL_IDIR) -I$(SQLITE3_IDIR) -I$(CLICKHOUSE_CPP_IDIR) -I$(SSL_IDIR) -I$(0) | ||
LDIRS := -L$(PROXYSQL_LDIR) -L$(JEMALLOC_LDIR) -L$(MARIADB_LDIR) -L$(LIBCONFIG_LDIR) -L$(LIBDAEMON_LDIR) -L$(RE2_LDIR) -L$(PCRE_LDIR) -L$(MICROHTTPD_LDIR) -L$(LIBHTTPSERVER_LDIR) -L$(LIBINJECTION_LDIR) -L$(CURL_LDIR) -L$(EV_LDIR) -L$(PROMETHEUS_LDIR) -L$(POSTGRES_LDIR) -L$(LIBSCRAM_LDIR) -L$(LIBUSUAL_LDIR) -L$(SQLITE3_LDIR) -L$(CLICKHOUSE_CPP_LDIR) -L$(SSL_LDIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice a couple of changes in the IDIRS
and LDIRS
definitions for compiling the main proxysql
executable:
$(LIBUSUAL_IDIR)
has been added toIDIRS
.$(SQLITE3_LDIR)
has been added toLDIRS
.
Could you provide some context for these additions?
- For
$(LIBUSUAL_IDIR)
: Was this added to fix a situation wherelibusual
headers were needed by thesrc
code but not explicitly included, potentially causing build issues on some systems? - For
$(SQLITE3_LDIR)
: Thelib/Makefile
archivessqlite3.o
intolibproxysql.a
. Is there a specific reasonsrc/Makefile
now also needsSQLITE3_LDIR
for linking the final executable? Does this address a particular linking scenario, or is it for general robustness?
implements ticket #4958
adds centos10 and almalinux10 on top of PR #4963