Skip to content

Fix: complete uninstall target to remove presets and assets directories #203

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.

Sign up for GitHub

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

Merged
sudip-mondal-2002 merged 13 commits into from
May 26, 2026
Merged

Fix: complete uninstall target to remove presets and assets directories #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fae9319
Fix: uninstall cleanup removes installed assets
Soujanya-Mctrl May 22, 2026
554476d
Merge branch 'main' into fix/makefile-uninstall-cleanup
Soujanya-Mctrl May 22, 2026
5a8a106
Test: Add install and uninstall CI smoke test
Soujanya-Mctrl May 24, 2026
421c918
Fix: uninstall cleanup removes installed assets
Soujanya-Mctrl May 22, 2026
6a1d783
Test: Add install and uninstall CI smoke test
Soujanya-Mctrl May 24, 2026
db01bb2
chore: merge origin/fix/makefile-uninstall-cleanup
Soujanya-Mctrl May 24, 2026
12d0fb5
Test: Add macOS and Windows install/uninstall CI smoke tests
Soujanya-Mctrl May 24, 2026
47d5643
Test: Use cmake --install for Windows smoke test (make not present in…
Soujanya-Mctrl May 24, 2026
5312d1e
Merge remote-tracking branch 'upstream/main' into fix/makefile-uninst…
Soujanya-Mctrl May 24, 2026
1deea00
Fix: move smoke tests to macOS CI
Soujanya-Mctrl May 24, 2026
ddbac3a
Merge branch 'main' into fix/makefile-uninstall-cleanup
Soujanya-Mctrl May 24, 2026
60ffb7f
Merge branch 'main' into fix/makefile-uninstall-cleanup
sudip-mondal-2002 May 25, 2026
113dab2
Merge branch 'main' into fix/makefile-uninstall-cleanup
Soujanya-Mctrl May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,23 @@ jobs:
- name: Run Tests
run: cd build && ./amplitron-tests

- 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: Package Build Data for Coverage
run: tar -czf build-coverage.tar.gz build/

Expand Down Expand Up @@ -222,6 +239,23 @@ jobs:
- name: Run Tests
run: cd build && ./amplitron-tests

- 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: Bundle Dylibs
if: github.ref == 'refs/heads/main' && github.event_name == 'push'
run: |
Expand Down Expand Up @@ -470,6 +504,27 @@ jobs:
shell: msys2 {0}
run: cd build && ./amplitron-tests.exe

- name: Install Smoke Test
shell: msys2 {0}
env:
INSTALL_PREFIX: ${{ runner.temp }}\amplitron-install
run: |
cmake --install build --prefix "$INSTALL_PREFIX"
test -x "$INSTALL_PREFIX/bin/Amplitron.exe"
"$INSTALL_PREFIX/bin/Amplitron.exe" --version | grep -q "Amplitron v1.0"

- 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"
Comment on lines +516 to +526

@coderabbitai coderabbitai Bot May 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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' Makefile

Repository: 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.yml lines 516-526) manually deletes "$INSTALL_PREFIX/bin/Amplitron.exe" (and related dirs) instead of running make PREFIX="$INSTALL_PREFIX" uninstall.
  • The Makefile uninstall: target currently removes $(PREFIX)/bin/Amplitron (no .exe), so make uninstall on Windows would leave the installed Amplitron.exe behind.

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.


- name: Collect Binaries
if: github.ref == 'refs/heads/main' && github.event_name == 'push'
shell: msys2 {0}
Expand Down
14 changes: 9 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
BUILD_DIR := build
BUILD_TYPE ?= Release
CMAKE_FLAGS ?=
PREFIX ?= /usr/local
NPROC := $(shell nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 4)

.PHONY: all build clean rebuild install uninstall setup run help
Expand All @@ -23,7 +24,7 @@ build: $(BUILD_DIR)/Makefile
@echo "=== Building Guitar Amp Simulator ($(BUILD_TYPE)) ==="
@cmake --build $(BUILD_DIR) --config $(BUILD_TYPE) -j$(NPROC)
@echo "=== Build complete ==="
@echo "Binary: $(BUILD_DIR)/amplitron"
@echo "Binary: $(BUILD_DIR)/Amplitron"

clean:
@echo "=== Cleaning build directory ==="
Expand All @@ -33,15 +34,17 @@ rebuild: clean build

install: build
@echo "=== Installing ==="
@cmake --install $(BUILD_DIR) --prefix /usr/local
@cmake --install $(BUILD_DIR) --prefix $(PREFIX)

uninstall:
@echo "=== Uninstalling ==="
@rm -f /usr/local/bin/amplitron
@rm -f $(PREFIX)/bin/Amplitron
@rm -rf $(PREFIX)/share/amplitron
@rm -rf $(PREFIX)/bin/assets

run: build
@echo "=== Running Guitar Amp Simulator ==="
@./$(BUILD_DIR)/amplitron
@./$(BUILD_DIR)/Amplitron

debug:
@$(MAKE) BUILD_TYPE=Debug build
Expand All @@ -54,10 +57,11 @@ help:
@echo " make debug - Build the project (Debug)"
@echo " make clean - Remove build directory"
@echo " make rebuild - Clean + build"
@echo " make install - Install to /usr/local/bin"
@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)"
Comment on lines +60 to 67

@coderabbitai coderabbitai Bot May 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Loading