Improve build performance by preserving test dependencies during 'make clean'#5289
Improve build performance by preserving test dependencies during 'make clean'#5289renecannao merged 4 commits intov3.0from
Conversation
- Remove test/deps from 'make clean' to prevent accidental 2.8GB cleanup - 'make clean' now preserves test dependencies, speeding up development workflow - 'make cleanall' still removes test dependencies as intended - In test/deps/Makefile, make 'clean' an alias of 'cleanall' since incremental clean was practically redundant due to build targets forcing full rebuilds This change: - Reduces typical 'make clean' time by ~30 minutes - Prevents accidental loss of compiled test dependencies (2.8GB) - Makes development workflow more efficient while preserving explicit control Resolves issue with repeated MySQL connector compilation during development.
…rmance - Remove cpp-dotenv source cleanup from 'make clean' in test/tap/tap/Makefile - This prevents accidental removal of 213MB of extracted cpp-dotenv source - Build targets force full rebuild anyway if libraries are missing - Typical 'make clean' operation is now faster and preserves compiled artifacts Similar optimization as test/deps: - Regular development workflow preserves expensive-to-rebuild dependencies - Only 'cleanall' removes everything including source directories - Reduces repeated downloads and extraction of cpp-dotenv sources This change complements the previous test/deps optimization, further improving development workflow performance.
…upport - Fix: Exclude cpp-dotenv directories from find commands in 'make clean' (was removing build artifacts even when preserving source directories) - Add: proper 'cleanall' target in test/tap/tap/Makefile that removes cpp-dotenv source - Update: test/tap/Makefile cleanall to call tap's cleanall instead of clean - Now cpp-dotenv directories are properly preserved during development This fixes a bug where: 1. cpp-dotenv build artifacts were still being deleted during 'make clean' 2. There was no way to properly clean cpp-dotenv except manually 3. The preservation optimization was ineffective due to the above Behavior now: - 'make clean': preserves cpp-dotenv source and build artifacts (~213MB saved) - 'make cleanall': removes everything including cpp-dotenv source
Summary of ChangesHello @renecannao, 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 improves the development workflow performance by optimizing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 significantly improves the development workflow by optimizing the make clean command to preserve test dependencies and cpp-dotenv artifacts, which are expensive to rebuild. The changes are well-structured, with clean providing a fast cleanup and cleanall handling the complete removal of all build artifacts and dependencies. The logic in the Makefiles is sound and aligns with the stated goals. I have one suggestion to further improve the script in one of the Makefiles.
test/tap/tap/Makefile
Outdated
| find . -path ./cpp-dotenv -prune -o -name '*.a' -delete || true | ||
| find . -path ./cpp-dotenv -prune -o -name '*.o' -delete || true | ||
| find . -path ./cpp-dotenv -prune -o -name '*.so' -delete || true | ||
| find . -path ./cpp-dotenv -prune -o -name '*.so.*' -delete || true |
There was a problem hiding this comment.
For better readability and efficiency, you can combine these four find commands into a single one. This avoids traversing the directory structure multiple times.
find . -path ./cpp-dotenv -prune -o \( -name '*.a' -o -name '*.o' -o -name '*.so' -o -name '*.so.*' \) -delete || true
Combine multiple find commands into a single command to reduce directory traversal from 4 passes to 1 pass, improving both performance and readability. Suggestion from gemini-code-assist review.
|
Thanks @gemini-code-assist for the helpful review! 🙏 Implemented the suggested optimization to combine multiple find commands into a single command in The change has been committed and pushed to the branch. |
|
That's excellent news, @renecannao! I'm glad to hear that the suggested optimization for combining the |
|
|
retest this please |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (4)
Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure Comment |
Summary
This PR significantly improves development workflow performance by optimizing the
make cleanbehavior to preserve expensive-to-rebuild test dependencies, while maintaining the same build guarantees.Changes
1. Preserve test/deps during
make cleanmake cleanremoved 2.8GB of MySQL connector test dependenciesmake cleanpreserves test dependencies, onlycleanallremoves them2. Preserve cpp-dotenv during
make cleancleanallsupport added3. Fix cpp-dotenv preservation logic
findcommands were still removing cpp-dotenv build artifacts despite source preservationcleanalltarget for cpp-dotenv directoriesPerformance Improvements
Storage preserved during
make clean:Time savings:
make clean: ~35-40 minutes fasterBehavior Changes
Before
After
Technical Details
find -pruneto exclude cpp-dotenv directories from deletioncleanallremoves expensive dependenciesTesting
The changes maintain backward compatibility:
make build_tap_test_debugworks identicallyCommits
2ecda902- Improve build performance and clean behaviorca224cfb- Preserve cpp-dotenv source during 'make clean' to improve build performance4be31efd- Fix cpp-dotenv preservation in clean target and add proper cleanall supportTarget Branch
Target:
v3.0Type: Performance improvement and workflow optimization