Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @miker2, 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!
I've introduced Bazel build rules to the repository, aiming to modernize the build system and improve build performance and reproducibility. This change involves adding new Bazel configuration files and defining build targets for the C++ libraries, Python bindings, and all associated tests.
Highlights
- Build System Migration: Migrated the project's build system to Bazel, enabling more efficient and reproducible builds.
- C++ Build Rules: Defined comprehensive Bazel cc_library and cc_test rules for all C++ source files, headers, and unit tests, including setting C++20 as the default standard.
- Python Integration: Implemented Bazel pybind_extension and py_test rules to build Python bindings and run Python-based tests, leveraging pybind11_bazel and rules_python.
- Dependency Management: Configured Bazel Bzlmod for external dependency management, listing required third-party libraries like googletest, pegtl, magic_enum, fmt, and range-v3.
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 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 or fill out our survey 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.
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.
Code Review
This pull request introduces Bazel build support for the project, which is a great step towards modernizing the build system. The changes include adding MODULE.bazel for dependency management with bzlmod, .bazelrc for build configuration, and BUILD files for the source code, tests, and Python bindings.
My review focuses on applying Bazel best practices to improve maintainability and correctness. I've suggested:
- Consolidating compiler flags in
.bazelrc. - Reverting a potentially problematic change in
.gitignore. - Replacing a recursive
globwith an explicit file list in aBUILDfile for hermeticity. - Refactoring repetitive
cc_testdefinitions to make the test configuration more concise. - A minor cleanup in the Python test rule.
Overall, this is a solid contribution. Addressing these points will make the new Bazel setup even more robust.
| .ipynb_checkpoints | ||
|
|
||
| sandbox | ||
| build |
There was a problem hiding this comment.
The build directory is a common output directory for build systems like CMake. Since CMakeLists.txt is still present in the repository, it's likely that this directory should remain in .gitignore to avoid committing build artifacts.
.bazelrc
Outdated
| # Set C++20 as the default C++ standard | ||
| build --cxxopt='-std=c++20' | ||
|
|
||
| # Optional: Add other common compiler flags | ||
| build --cxxopt='-Wall' | ||
| build --cxxopt='-Wextra' |
There was a problem hiding this comment.
For conciseness, you can combine multiple options for the same command onto a single line. This makes the configuration easier to read and manage.
# Set C++20 as the default C++ standard and add other common compiler flags
build --cxxopt='-std=c++20' --cxxopt='-Wall' --cxxopt='-Wextra'
| cc_library( | ||
| name = "flexi_cfg_hdrs", | ||
| hdrs = glob(["flexi_cfg/**/*.h"]), | ||
| include_prefix = "flexi_cfg", | ||
| visibility = ["//visibility:public"], | ||
| ) |
There was a problem hiding this comment.
Using recursive glob is discouraged in Bazel as it can lead to non-hermetic builds and can be less performant. It's better to list the header files explicitly for better reproducibility and clarity. Sorting the list makes it easier to manage.
cc_library(
name = "flexi_cfg_hdrs",
hdrs = sorted([
"flexi_cfg/config/actions.h",
"flexi_cfg/config/classes.h",
"flexi_cfg/config/exceptions.h",
"flexi_cfg/config/grammar.h",
"flexi_cfg/config/helpers.h",
"flexi_cfg/config/parser-internal.h",
"flexi_cfg/config/selector.h",
"flexi_cfg/config/trace-internal.h",
"flexi_cfg/logger.h",
"flexi_cfg/math/actions.h",
"flexi_cfg/math/grammar.h",
"flexi_cfg/math/helpers.h",
"flexi_cfg/parser.h",
"flexi_cfg/reader.h",
"flexi_cfg/utils.h",
"flexi_cfg/visitor.h",
"flexi_cfg/visitor-internal.h",
"flexi_cfg/visitor-json.h",
]),
include_prefix = "flexi_cfg",
visibility = ["//visibility:public"],
)
| py_test( | ||
| name = "test_flexi_cfg", | ||
| srcs = ["test_flexi_cfg.py"], | ||
| deps = [":flexi_cfg_py"], | ||
| main = "test_flexi_cfg.py", | ||
| ) |
There was a problem hiding this comment.
The main attribute is redundant here. When srcs contains a file named after the target with a .py extension (i.e., test_flexi_cfg.py), it is automatically selected as the main entry point for the test. You can remove it for cleaner code.
py_test(
name = "test_flexi_cfg",
srcs = ["test_flexi_cfg.py"],
deps = [":flexi_cfg_py"],
)
| cc_test( | ||
| name = "config_exception_test", | ||
| srcs = ["config_exception_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "config_grammar_test", | ||
| srcs = ["config_grammar_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "config_helpers_test", | ||
| srcs = ["config_helpers_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "config_parse_test", | ||
| srcs = ["config_parse_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "math_test", | ||
| srcs = ["math_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "utils_test", | ||
| srcs = ["utils_test.cpp"], | ||
| deps = [ | ||
| "//src:flexi_cfg", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
To reduce repetition and improve maintainability, you can use a list comprehension to generate these similar cc_test targets. This makes it easier to add or remove tests in the future.
[cc_test(
name = name,
srcs = [name + ".cpp"],
deps = [
"//src:flexi_cfg",
"@com_google_googletest//:gtest_main",
],
) for name in sorted([
"config_exception_test",
"config_grammar_test",
"config_helpers_test",
"config_parse_test",
"math_test",
"utils_test",
])]
This commit completes the migration of the project to the Bazel build system, following modern best practices. The key changes include: - Cleaning up the old, incomplete Bazel configuration. - Migrating from a WORKSPACE-based setup to a modern bzlmod setup for dependency management. - Defining all external dependencies in `MODULE.bazel`. - Creating a well-structured set of `BUILD` files for the main library (`src`), C++ tests (`tests`), and Python bindings (`python`). - Using native Bazel rules (`cc_library`, `cc_test`, `pybind_extension`, `py_test`) for all targets. - Resolving various build issues related to dependency versions and target definitions. This change provides a clean, modern, and maintainable Bazel build for the project.
This commit completes the migration of the project to the Bazel build system, following modern best practices. The key changes include: - Cleaning up the old, incomplete Bazel configuration. - Migrating from a WORKSPACE-based setup to a modern bzlmod setup for dependency management. - Defining all external dependencies in `MODULE.bazel`. - Creating a well-structured set of `BUILD` files for the main library (`src`), C++ tests (`tests`), and Python bindings (`python`). - Using native Bazel rules (`cc_library`, `cc_test`, `pybind_extension`, `py_test`) for all targets. - Resolving various build issues related to dependency versions and target definitions. This change provides a clean, modern, and maintainable Bazel build for the project.
This commit completes the migration of the project to the Bazel build system, following modern best practices. The key changes include: - Cleaning up the old, incomplete Bazel configuration. - Migrating from a WORKSPACE-based setup to a modern bzlmod setup for dependency management. - Defining all external dependencies in `MODULE.bazel`. - Creating a well-structured set of `BUILD` files for the main library (`src`), C++ tests (`tests`), and Python bindings (`python`). - Using native Bazel rules (`cc_library`, `cc_test`, `pybind_extension`, `py_test`) for all targets. - Resolving various build issues related to dependency versions and target definitions. This change provides a clean, modern, and maintainable Bazel build for the project.
miker2
force-pushed
the
feat/bazel-migration
branch
from
e91faac to
e256258
Compare
1 participant
No description provided.