Conversation
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 refactors the project's build system by centralizing Makefile variable definitions and updating several key third-party dependencies. The primary goal is to enhance the maintainability and readability of the Makefiles, while also ensuring the project benefits from the latest features and bug fixes in its external libraries. A new JSON parsing library has also been integrated, streamlining JSON handling throughout the codebase. 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 does a great job of refactoring the Makefiles to centralize control and path variables, which significantly improves readability and maintainability. The introduction of makefiles_vars.mk and makefiles_paths.mk is a good approach. I've found a few issues related to the completeness of the refactoring and some duplication that crept in. Addressing these will make the new structure more robust and consistent. Overall, this is a valuable cleanup.
include/makefiles_paths.mk
Outdated
|
|
||
| SYS_LOC_IDIR := /usr/local/include | ||
|
|
||
| PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd) |
There was a problem hiding this comment.
This PROXYSQL_PATH definition is duplicated in all Makefiles that include this file. This goes against the goal of this PR, which is to reduce duplication. The last definition of a variable is what make uses, so this definition will override the one in the including Makefile. This can lead to subtle bugs if the logic ever diverges.
Please remove this definition from this file. The individual Makefiles (lib/Makefile, src/Makefile, etc.) already define it before including this file, which is the correct approach to ensure the path is resolved from the correct starting directory.
|
|
||
| GIT_VERSION ?= $(shell git describe --long --abbrev=7) | ||
| ifndef GIT_VERSION | ||
| $(error GIT_VERSION is not set) | ||
| endif | ||
|
|
||
| UNAME_S := $(shell uname -s) | ||
| UNAME_M := $(shell uname -m) | ||
|
|
||
| DISTRO := $(shell grep '^ID=' /etc/os-release | cut -d= -f2 | tr -d '"') | ||
|
|
||
| CENTOSVER := Unknown | ||
| ifneq (,$(wildcard /etc/system-release)) | ||
| CENTOSVER := $(shell rpm --eval %rhel) | ||
| endif | ||
| include $(PROXYSQL_PATH)/include/makefiles_vars.mk |
There was a problem hiding this comment.
This Makefile should also include makefiles_paths.mk to make use of the centralized path variables. Currently, it seems the refactoring is incomplete for this file, as some rules still use hardcoded relative paths instead of the new variables.
For example, the libscram rule uses POSTGRESQL_DIR=\"$(shell pwd)/postgresql/postgresql/\". After including makefiles_paths.mk, this and other rules should be updated to use variables like $(POSTGRESQL_PATH).
include $(PROXYSQL_PATH)/include/makefiles_vars.mk
include $(PROXYSQL_PATH)/include/makefiles_paths.mk
deps/Makefile
Outdated
| json/json/include/nlohmann/json.hpp: | ||
| cd json && rm -rf json-*/ || true | ||
| cd json && tar -zxf json-*.tar.gz | ||
| cd json/json && cmake . |
There was a problem hiding this comment.
The nlohmann/json library is header-only. The required header files are extracted by the tar command in the previous line. This cmake . command only generates local build files and doesn't produce any artifacts needed for the build. It's unnecessary and could be a point of failure if cmake isn't available. It's best to remove this line.
e675de0 to
31d716a
Compare
ea9c83f to
0a74d98
Compare
0a74d98 to
d020f57
Compare
|
no functional changes
control and path variables are defined over and over in makefiles
avoid duplication, help readability and maintainability
extracting control and path variables into separate include files