-
Notifications
You must be signed in to change notification settings - Fork 1k
V3.0 add fedora42 builds #4963
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?
V3.0 add fedora42 builds #4963
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 team, gemini-code-assist here to provide a summary of this pull request. This PR, authored by @mirostauder, aims to implement ticket #4957 by adding support for building ProxySQL on Fedora 42. This involves updating the build infrastructure, including Makefiles and Docker Compose configurations, to include targets and services for the new Fedora version. Additionally, several dependencies have been updated, and their build processes adjusted to ensure compatibility with the new build environments.
Highlights
- Add CentOS 10 Builds: Adds build targets and Docker Compose services for CentOS 10, including standard, clang, and debug builds, alongside the Fedora 42 changes.
- Dependency Updates: Updates several key dependencies: clickhouse-cpp (2.3.0 -> 2.5.1), libconfig (1.7.3 -> 1.8), and postgresql (16.3 -> 16.9).
- Build Process Adjustments: Modifies the build steps for updated dependencies in
deps/Makefile
(e.g., switching libconfig to CMake, adjusting clickhouse-cpp build commands) and updates include/library paths inlib/Makefile
andsrc/Makefile
.
Changelog
Click here to see the changelog
- Makefile
- Added
fedora42
andcentos10
targets toamd64-fedora
andamd64-centos
respectively. - Added
fedora42
andcentos10
targets toarm64-fedora
andarm64-centos
respectively.
- Added
- deps/Makefile
- Updated the build target name for clickhouse-cpp static library from
libclickhouse-cpp-lib-static.a
tolibclickhouse-cpp-lib.a
. - Changed the clickhouse-cpp tar extraction command to use a wildcard (
clickhouse-cpp-*.tar.gz
). - Added
sed
commands to the clickhouse-cpp build process to replace<ciso646>
with<version>
in absl headers. - Commented out the patch application for
clickhouse/base/wire_format.h
in the clickhouse-cpp build. - Added
sed
commands to the mariadb-client build process to replacebool
withbbool
inma_global.h
andssl.c
. - Commented out the
autoreconf
and./configure
steps for libconfig. - Switched the libconfig build process to use
cmake
with specific options (-DBUILD_SHARED_LIBS=0
,-DBUILD_EXAMPLES=0
,-DBUILD_TESTS=0
,-DBUILD_FUZZERS=0
). - Updated the libconfig build target name from
libconfig/libconfig/lib/.libs/libconfig++.a
tolibconfig/libconfig/out/libconfig++.a
. - Added a symbolic link creation step (
ln -fsT
) for the postgresql source directory. - Corrected the
rm -rf
command for clickhouse-cpp in thecleanall
target to useclickhouse-cpp-*/
.
- Updated the build target name for clickhouse-cpp static library from
- deps/clickhouse-cpp/clickhouse-cpp
- Updated the clickhouse-cpp version from
clickhouse-cpp-2.3.0
toclickhouse-cpp-2.5.1/
.
- Updated the clickhouse-cpp version from
- deps/libconfig/libconfig
- Updated the libconfig version from
libconfig-1.7.3
tolibconfig-1.8
.
- Updated the libconfig version from
- deps/postgresql/postgresql
- Updated the postgresql version from
postgresql-16.3
topostgresql-16.9/
.
- Updated the postgresql version from
- docker-compose.yml
- Added
centos10_build
,centos10_clang_build
, andcentos10_dbg_build
services. - Added
fedora42_build
,fedora42_clang_build
, andfedora42_dbg_build
services.
- Added
- include/btree.h
- Added
#include <cstdint>
.
- Added
- lib/Makefile
- Added
CLICKHOUSE_CPP_PATH
andCLICKHOUSE_CPP_IDIR
variables for clickhouse-cpp include paths. - Updated variable assignments for
LIBUSUAL_PATH
,LIBUSUAL_IDIR
,LIBUSUAL_LDIR
,LIBSCRAM_PATH
,LIBSCRAM_IDIR
,LIBSCRAM_LDIR
,POSTGRES_PATH
,POSTGRES_IDIR
, andPOSTGRES_LDIR
to use:=
instead of=
and adjusted path formats. - Updated the
IDIRS
variable to use$(CLICKHOUSE_CPP_IDIR)
instead of$(CLICKHOUSE_CPP_DIR)
and$(CLICKHOUSE_CPP_DIR)/contrib/
.
- Added
- src/Makefile
- Updated
LIBCONFIG_LDIR
to point to theout
directory instead oflib/.libs
. - Updated variable assignments for
LIBUSUAL_PATH
,LIBUSUAL_IDIR
,LIBUSUAL_LDIR
,LIBSCRAM_PATH
,LIBSCRAM_IDIR
, andLIBSCRAM_LDIR
to use:=
instead of=
and adjusted path formats.
- Updated
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 builds arrive,
Fedora, CentOS thrive,
Dependencies update.
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 primarily introduces build configurations for Fedora 42 and CentOS 10, which is a valuable addition for broader OS support. The changes also include updates to several key dependencies like clickhouse-cpp
, libconfig
, and postgresql
, along with modifications to their build processes within the deps/Makefile
.
Overall, the additions for new OS builds in Makefile
and docker-compose.yml
seem consistent and well-structured. The updates to dependency versions and their build steps are more involved and raise a few points for discussion, particularly around modifications made via sed
.
Good job on updating the build system to support these new distributions and for keeping dependencies up-to-date!
Summary of Findings
- MariaDB
bool
tobbool
Renaming: Thesed
commands indeps/Makefile
that renamebool
tobbool
in MariaDB client library files are a significant modification to a dependency. This needs clarification on its necessity and potential risks. - Clarity of Makefile Modifications: Several patches and
sed
commands indeps/Makefile
are commented out. It would be beneficial to understand if these are temporarily disabled or obsolete (in which case they could be removed). Activesed
commands could also benefit from comments explaining their purpose. - Dependency Updates and Build System Changes: The PR updates several dependencies (
clickhouse-cpp
,libconfig
,postgresql
) and changes howlibconfig
is built (from autotools to CMake). These are substantial changes that accompany the new OS support. - Makefile Best Practices: The switch from
=
to:=
for variable assignments inlib/Makefile
andsrc/Makefile
is a good improvement.
Merge Readiness
This pull request makes good progress in adding support for Fedora 42 and CentOS 10. The changes to Makefile
and docker-compose.yml
for the new build targets appear solid.
However, the modifications to dependency build processes, especially the bool
to bbool
renaming for the MariaDB client library, raise concerns that should be addressed. Clarifying the reasons for commented-out code and adding comments to sed
commands in deps/Makefile
would also improve long-term maintainability.
I recommend addressing the high
severity issue regarding the MariaDB modification and considering the medium
severity suggestions before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from the team.
deps/Makefile
Outdated
cd mariadb-client-library/mariadb_client && sed -i 's/\tbool;/\tbbool;/' ./include/ma_global.h | ||
cd mariadb-client-library/mariadb_client && sed -i 's/ bool / bbool /' ./unittest/libmariadb/ssl.c |
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.
These sed
commands rename bool
to bbool
in MariaDB client library source files (ma_global.h
and ssl.c
). This seems like a significant workaround.
- Could you elaborate on why this renaming is necessary? Is it due to a conflict with C++
bool
when compiling C code, or a specific issue with the MariaDB connector version on newer distros/compilers like Fedora 42/CentOS 10? - Are there potential side effects or risks associated with this change? It might mask underlying type issues or affect compatibility if not handled carefully.
Consider adding a comment explaining the rationale for this modification.
deps/Makefile
Outdated
cd clickhouse-cpp/clickhouse-cpp && patch clickhouse/base/wire_format.h < ../wire_format.patch | ||
cd clickhouse-cpp && tar -zxf clickhouse-cpp-*.tar.gz | ||
cd clickhouse-cpp && ln -fsT clickhouse-cpp-*/ clickhouse-cpp | ||
# cd clickhouse-cpp/clickhouse-cpp && patch clickhouse/base/wire_format.h < ../wire_format.patch |
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.
deps/Makefile
Outdated
cd clickhouse-cpp/clickhouse-cpp && sed -i 's/<ciso646>/<version>/' contrib/absl/absl/base/options.h | ||
cd clickhouse-cpp/clickhouse-cpp && sed -i 's/<ciso646>/<version>/' contrib/absl/absl/base/config.h | ||
cd clickhouse-cpp/clickhouse-cpp && sed -i 's/<ciso646>/<version>/' contrib/absl/absl/numeric/int128.h | ||
cd clickhouse-cpp/clickhouse-cpp && sed -i 's/<ciso646>/<version>/' contrib/absl/absl/numeric/int128.cc |
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.
These sed
commands modify absl
headers within clickhouse-cpp
to replace <ciso646>
with <version>
. This is a common fix for C++20 compatibility, as <ciso646>
is deprecated/removed.
Could we add a brief comment here explaining why these sed
commands are necessary (e.g., "Workaround for C++20 compatibility in absl")? This would improve maintainability for anyone looking at this in the future.
deps/Makefile
Outdated
# cd clickhouse-cpp/clickhouse-cpp && sed -i 's|absl/numeric/int128.h|contrib/absl/absl/numeric/int128.h|' clickhouse/types/types.h | ||
# cd clickhouse-cpp/clickhouse-cpp && sed -i 's|absl/base/config.h|../base/config.h|' contrib/absl/absl/numeric/int128.h | ||
# cd clickhouse-cpp/clickhouse-cpp && sed -i 's|absl/base/options.h|../base/options.h|' contrib/absl/absl/base/config.h |
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.
These sed
commands, which appear to adjust include paths within clickhouse-cpp
, are commented out. Are these changes no longer necessary with the updated clickhouse-cpp
version or the new CLICKHOUSE_CPP_IDIR
definition in lib/Makefile
? If they are indeed obsolete, perhaps they could be removed for clarity?
deps/Makefile
Outdated
# cd libconfig/libconfig && autoreconf | ||
# cd libconfig/libconfig && ./configure --disable-examples |
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 autoreconf
and ./configure
commands for libconfig
are commented out, and a cmake
build process is now used (line 274). This is a clear switch in the build system for this dependency.
If the autotools build is no longer an option or intended path for libconfig-1.8
, would it be clearer to remove these commented-out lines entirely?
lib/Makefile
Outdated
LIBUSUAL_PATH := $(DEPS_PATH)/libusual/libusual | ||
LIBUSUAL_IDIR := $(LIBUSUAL_PATH) | ||
LIBUSUAL_LDIR := $(LIBUSUAL_PATH)/.libs | ||
|
||
LIBSCRAM_PATH=$(DEPS_PATH)/libscram/ | ||
LIBSCRAM_IDIR=$(LIBSCRAM_PATH)/include/ | ||
LIBSCRAM_LDIR=$(LIBSCRAM_PATH)/lib/ | ||
LIBSCRAM_PATH := $(DEPS_PATH)/libscram | ||
LIBSCRAM_IDIR := $(LIBSCRAM_PATH)/include | ||
LIBSCRAM_LDIR := $(LIBSCRAM_PATH)/lib | ||
|
||
POSTGRES_PATH=$(DEPS_PATH)/postgresql/postgresql/src/ | ||
POSTGRES_IFACE=$(POSTGRES_PATH)/interfaces/libpq/ -I$(POSTGRES_PATH)/include | ||
POSTGRES_LDIR=$(POSTGRES_IFACES)/libpq/ | ||
POSTGRES_PATH := $(DEPS_PATH)/postgresql/postgresql/src | ||
POSTGRES_IDIR := $(POSTGRES_PATH)/interfaces/libpq -I$(POSTGRES_PATH)/include | ||
POSTGRES_LDIR := $(POSTGRES_PATH)/interfaces/libpq |
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.
src/Makefile
Outdated
LIBUSUAL_PATH := $(DEPS_PATH)/libusual/libusual | ||
LIBUSUAL_IDIR := $(LIBUSUAL_PATH) | ||
LIBUSUAL_LDIR := $(LIBUSUAL_PATH)/.libs | ||
|
||
LIBSCRAM_PATH=$(DEPS_PATH)/libscram/ | ||
LIBSCRAM_IDIR=$(LIBSCRAM_PATH)/include/ | ||
LIBSCRAM_LDIR=$(LIBSCRAM_PATH)/lib/ | ||
LIBSCRAM_PATH := $(DEPS_PATH)/libscram | ||
LIBSCRAM_IDIR := $(LIBSCRAM_PATH)/include | ||
LIBSCRAM_LDIR := $(LIBSCRAM_PATH)/lib |
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.
implements ticket #4957