Fix: complete uninstall target to remove presets and assets directories#203
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesConfigurable Installation Paths and CI Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hey @sudip-mondal-2002 the PR is ready to review |
|
Please add a new CI that installs checks if installed via version flag |
Code Coverage Report 📊Line Coverage: 43.5% Full Coverage Summary |
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 60-67: The uninstall help text in the Makefile is out of date:
update the `@echo` line that currently reads "make uninstall - Remove installed
binary" to accurately reflect that the uninstall target removes more than just
the binary (it also removes presets and assets as implemented elsewhere in the
Makefile). Edit the help block in the Makefile (the echo lines) to say something
like "make uninstall - Remove installed binary, presets, and assets" so the
description matches the actual behavior of the uninstall target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e7f915-2429-4326-a561-49d990ac9220
📒 Files selected for processing (2)
.github/workflows/ci.ymlMakefile
| @echo " make install - Install to $(PREFIX)/bin" | ||
| @echo " make uninstall - Remove installed binary" | ||
| @echo " make run - Build and run" | ||
| @echo "" | ||
| @echo "Variables:" | ||
| @echo " BUILD_TYPE=Debug|Release (default: Release)" | ||
| @echo " PREFIX=/path/to/install (default: /usr/local)" | ||
| @echo " CMAKE_FLAGS=... (extra CMake flags)" |
There was a problem hiding this comment.
Update uninstall help text to reflect full cleanup.
Line 61 still says uninstall removes only the binary, but Lines 41-43 also remove presets/assets directories. Please align the help description with actual behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 60 - 67, The uninstall help text in the Makefile is
out of date: update the `@echo` line that currently reads "make uninstall - Remove
installed binary" to accurately reflect that the uninstall target removes more
than just the binary (it also removes presets and assets as implemented
elsewhere in the Makefile). Edit the help block in the Makefile (the echo lines)
to say something like "make uninstall - Remove installed binary, presets, and
assets" so the description matches the actual behavior of the uninstall target.
Soujanya-Mctrl
force-pushed
the
fix/makefile-uninstall-cleanup
branch
from
5a8a106 to
6a1d783
Compare
|
@sudip-mondal-2002 I’ve added the CI smoke test to install with a version flag, verify the installed binary via |
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
There was a problem hiding this comment.
LGTM but the smoke tests should be there for macos and windows as well, please do that change @Soujanya-Mctrl
|
@sudip-mondal-2002 I added matching macOS and Windows smoke tests to CI (ci.yml) and fixed the Windows job to use |
|
|
||
| - name: Install Smoke Test | ||
| env: | ||
| INSTALL_PREFIX: ${{ runner.temp }}/amplitron-install | ||
| run: | | ||
| make PREFIX="$INSTALL_PREFIX" install | ||
| test -x "$INSTALL_PREFIX/bin/Amplitron" | ||
| "$INSTALL_PREFIX/bin/Amplitron" --version | grep -q "Amplitron v1.0" | ||
|
|
||
| - name: Uninstall Smoke Test | ||
| env: | ||
| INSTALL_PREFIX: ${{ runner.temp }}/amplitron-install | ||
| run: | | ||
| make PREFIX="$INSTALL_PREFIX" uninstall | ||
| test ! -e "$INSTALL_PREFIX/bin/Amplitron" | ||
| test ! -e "$INSTALL_PREFIX/share/amplitron" | ||
| test ! -e "$INSTALL_PREFIX/bin/assets" | ||
|
|
||
| - name: Install Smoke Test | ||
| env: | ||
| INSTALL_PREFIX: ${{ runner.temp }}/amplitron-install | ||
| run: | | ||
| make PREFIX="$INSTALL_PREFIX" install | ||
| test -x "$INSTALL_PREFIX/bin/Amplitron" | ||
| "$INSTALL_PREFIX/bin/Amplitron" --version | grep -q "Amplitron v1.0" | ||
|
|
||
| - name: Uninstall Smoke Test | ||
| env: | ||
| INSTALL_PREFIX: ${{ runner.temp }}/amplitron-install | ||
| run: | | ||
| make PREFIX="$INSTALL_PREFIX" uninstall | ||
| test ! -e "$INSTALL_PREFIX/bin/Amplitron" | ||
| test ! -e "$INSTALL_PREFIX/share/amplitron" | ||
| test ! -e "$INSTALL_PREFIX/bin/assets" | ||
|
|
There was a problem hiding this comment.
Now you have the tests twice in linux and never in MAC
sudip-mondal-2002
added
type:bug
|
@sudip-mondal-2002 fixed in the latest push: Linux no longer has the duplicate smoke tests, and macOS now has the install/uninstall pair. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 516-526: The CI step "Uninstall Smoke Test" currently deletes
Amplitron.exe manually; update two things: (1) change the Makefile uninstall
target (the uninstall: rule) to also remove the Windows binary (e.g., remove
$(PREFIX)/bin/Amplitron.exe or use a platform-aware removal using an $(EXEEXT)
variable) so make PREFIX="$(PREFIX)" uninstall actually removes the .exe on
Windows, and (2) modify the CI workflow "Uninstall Smoke Test" to call make
PREFIX="$INSTALL_PREFIX" uninstall instead of manually deleting files so the
uninstall path is exercised by CI; if you can't make uninstall remove the .exe,
add a brief comment in the Makefile documenting the Windows limitation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - name: Uninstall Smoke Test | ||
| shell: msys2 {0} | ||
| env: | ||
| INSTALL_PREFIX: ${{ runner.temp }}\amplitron-install | ||
| run: | | ||
| rm -f "$INSTALL_PREFIX/bin/Amplitron.exe" | ||
| rm -rf "$INSTALL_PREFIX/share/amplitron" | ||
| rm -rf "$INSTALL_PREFIX/bin/assets" | ||
| test ! -e "$INSTALL_PREFIX/bin/Amplitron.exe" | ||
| test ! -e "$INSTALL_PREFIX/share/amplitron" | ||
| test ! -e "$INSTALL_PREFIX/bin/assets" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if Makefile uninstall target handles Windows .exe extension
# Test: Search for .exe handling in uninstall target
rg -n -A5 -B2 'uninstall:' Makefile
# Test: Check if there's platform-specific logic for Windows
rg -n 'MINGW|Windows|\.exe' MakefileRepository: sudip-mondal-2002/Amplitron
Length of output: 285
Windows uninstall smoke test should exercise make uninstall (and Makefile must remove Amplitron.exe)
- The Windows uninstall smoke test (
.github/workflows/ci.ymllines 516-526) manually deletes"$INSTALL_PREFIX/bin/Amplitron.exe"(and related dirs) instead of runningmake PREFIX="$INSTALL_PREFIX" uninstall. - The
Makefileuninstall:target currently removes$(PREFIX)/bin/Amplitron(no.exe), somake uninstallon Windows would leave the installedAmplitron.exebehind.
Update the Makefile uninstall logic to remove $(PREFIX)/bin/Amplitron.exe on Windows (platform-specific if needed), then switch the Windows CI step to run make PREFIX="$INSTALL_PREFIX" uninstall for consistency; otherwise document the Windows limitation.
Recommended CI adjustment after fixing Makefile
- name: Uninstall Smoke Test
shell: msys2 {0}
env:
INSTALL_PREFIX: ${{ runner.temp }}\amplitron-install
run: |
- rm -f "$INSTALL_PREFIX/bin/Amplitron.exe"
- rm -rf "$INSTALL_PREFIX/share/amplitron"
- rm -rf "$INSTALL_PREFIX/bin/assets"
+ make PREFIX="$INSTALL_PREFIX" uninstall
test ! -e "$INSTALL_PREFIX/bin/Amplitron.exe"
test ! -e "$INSTALL_PREFIX/share/amplitron"
test ! -e "$INSTALL_PREFIX/bin/assets"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 516 - 526, The CI step "Uninstall
Smoke Test" currently deletes Amplitron.exe manually; update two things: (1)
change the Makefile uninstall target (the uninstall: rule) to also remove the
Windows binary (e.g., remove $(PREFIX)/bin/Amplitron.exe or use a platform-aware
removal using an $(EXEEXT) variable) so make PREFIX="$(PREFIX)" uninstall
actually removes the .exe on Windows, and (2) modify the CI workflow "Uninstall
Smoke Test" to call make PREFIX="$INSTALL_PREFIX" uninstall instead of manually
deleting files so the uninstall path is exercised by CI; if you can't make
uninstall remove the .exe, add a brief comment in the Makefile documenting the
Windows limitation.
2 participants
What does this PR do?
This PR fixes the
Makefileuninstall target so it removes the installed presets and assets directories in addition to the main executable. This keepsmake uninstallaligned with whatmake installputs on the system.Related Issue
Fixes #199
Type of Change
How Was This Tested?
make installand verified files were placed in/usr/local/bin/,/usr/local/share/amplitron/and/usr/local/bin/assets/make uninstalland confirmed all three locations were fully cleaned up./amplitron-testsChecklist
./amplitron-tests)std::mutex::lock()on the hot path)Summary by CodeRabbit
Tests
Build