Updated install.sh to use atomic filesystem compatible locations#47
Updated install.sh to use atomic filesystem compatible locations#47cschram wants to merge 1 commit into
Conversation
AndyCappDev
left a comment
There was a problem hiding this comment.
Thanks — atomic-FS support is a real gap, and the bulk of this is solid. The migration-detection block at the bottom is a nice touch.
One bug to fix before merging: the &&/|| chain on the desktop-file removal block (lines 297–300) doesn't do what it looks like. Because of the || true after update-desktop-database, the chain always succeeds at the update-desktop-database line, so the echo "Removed..." always fires and the echo "Could not remove..." branch is dead code.
Compare to the launcher block immediately above (lines 290–292), which is structured correctly because nothing in the chain has its own fallback.
Easiest fix is an if/else — suggestion inline.
One non-blocking note: the new launcher path $HOME/.local/bin/tuxbox-gui isn't in PATH by default on every shell (the issue #46 reporter was on fish, for example, where it isn't). Consider either adding a PATH check + warning, or noting it in the final post-install instructions. Up to you whether to address this here or in a follow-up.
| sudo rm -f "/usr/share/applications/tourbox-gui.desktop" 2>/dev/null && \ | ||
| sudo update-desktop-database /usr/share/applications/ 2>/dev/null || true | ||
| echo -e "${GREEN}✓${NC} Removed old desktop entry: /usr/share/applications/tourbox-gui.desktop" | ||
| echo -e "${GREEN}✓${NC} Removed old desktop entry: /usr/share/applications/tourbox-gui.desktop" || \ | ||
| echo -e "${YELLOW}!${NC} Could not remove old desktop entry (read-only filesystem)" |
There was a problem hiding this comment.
| sudo rm -f "/usr/share/applications/tourbox-gui.desktop" 2>/dev/null && \ | |
| sudo update-desktop-database /usr/share/applications/ 2>/dev/null || true | |
| echo -e "${GREEN}✓${NC} Removed old desktop entry: /usr/share/applications/tourbox-gui.desktop" | |
| echo -e "${GREEN}✓${NC} Removed old desktop entry: /usr/share/applications/tourbox-gui.desktop" || \ | |
| echo -e "${YELLOW}!${NC} Could not remove old desktop entry (read-only filesystem)" | |
| if sudo rm -f "/usr/share/applications/tourbox-gui.desktop" 2>/dev/null; then | |
| sudo update-desktop-database /usr/share/applications/ 2>/dev/null || true | |
| echo -e "${GREEN}✓${NC} Removed old desktop entry: /usr/share/applications/tourbox-gui.desktop" | |
| else | |
| echo -e "${YELLOW}!${NC} Could not remove old desktop entry (read-only filesystem)" | |
| fi |
There was a problem hiding this comment.
Thanks for the review @AndyCappDev, and good catch, I missed that. I've pushed an update that uses your fix, and adds a warning if ~/.local/bin isn't in PATH.
ba1badd to
803401f
Compare
803401f to
8427c77
Compare
AndyCappDev
left a comment
There was a problem hiding this comment.
Thanks for tackling the broader atomic-filesystem refactor — moving the install to ~/.local/, the PATH check with shell-specific instructions, and the leftover-files detection are all nice additions on top of the original fix. The desktop-block change is exactly right too.
One small consistency request before merging: could the launcher and icon cleanup blocks use the same if/then/else shape you applied to the desktop block? They currently still use the cmd && echo X || echo Y chain, which leaves the same MIGRATED_SOMETHING=true-on-failure quirk we fixed for the desktop block. Mirroring its structure keeps the three cleanup paths consistent and easier to read side-by-side.
Suggestions inline.
| if [ -f "/usr/local/bin/tourbox-gui" ]; then | ||
| sudo rm -f "/usr/local/bin/tourbox-gui" | ||
| echo -e "${GREEN}✓${NC} Removed old launcher: /usr/local/bin/tourbox-gui" | ||
| sudo rm -f "/usr/local/bin/tourbox-gui" 2>/dev/null && \ | ||
| echo -e "${GREEN}✓${NC} Removed old launcher: /usr/local/bin/tourbox-gui" || \ | ||
| echo -e "${YELLOW}!${NC} Could not remove old launcher (read-only filesystem)" | ||
| MIGRATED_SOMETHING=true | ||
| fi |
There was a problem hiding this comment.
Mirror the desktop block's structure here so all three cleanup paths read consistently and MIGRATED_SOMETHING only flips on actual success.
| if [ -f "/usr/local/bin/tourbox-gui" ]; then | |
| sudo rm -f "/usr/local/bin/tourbox-gui" | |
| echo -e "${GREEN}✓${NC} Removed old launcher: /usr/local/bin/tourbox-gui" | |
| sudo rm -f "/usr/local/bin/tourbox-gui" 2>/dev/null && \ | |
| echo -e "${GREEN}✓${NC} Removed old launcher: /usr/local/bin/tourbox-gui" || \ | |
| echo -e "${YELLOW}!${NC} Could not remove old launcher (read-only filesystem)" | |
| MIGRATED_SOMETHING=true | |
| fi | |
| if [ -f "/usr/local/bin/tourbox-gui" ]; then | |
| if sudo rm -f "/usr/local/bin/tourbox-gui" 2>/dev/null; then | |
| echo -e "${GREEN}✓${NC} Removed old launcher: /usr/local/bin/tourbox-gui" | |
| MIGRATED_SOMETHING=true | |
| else | |
| echo -e "${YELLOW}!${NC} Could not remove old launcher (read-only filesystem)" | |
| fi | |
| fi |
| if [ -f "/usr/share/pixmaps/tourbox-icon.png" ]; then | ||
| sudo rm -f "/usr/share/pixmaps/tourbox-icon.png" | ||
| echo -e "${GREEN}✓${NC} Removed old icon: /usr/share/pixmaps/tourbox-icon.png" | ||
| sudo rm -f "/usr/share/pixmaps/tourbox-icon.png" 2>/dev/null && \ | ||
| echo -e "${GREEN}✓${NC} Removed old icon: /usr/share/pixmaps/tourbox-icon.png" || \ | ||
| echo -e "${YELLOW}!${NC} Could not remove old icon (read-only filesystem)" | ||
| MIGRATED_SOMETHING=true | ||
| fi |
There was a problem hiding this comment.
Same change as the launcher block above — matches the desktop-block pattern.
| if [ -f "/usr/share/pixmaps/tourbox-icon.png" ]; then | |
| sudo rm -f "/usr/share/pixmaps/tourbox-icon.png" | |
| echo -e "${GREEN}✓${NC} Removed old icon: /usr/share/pixmaps/tourbox-icon.png" | |
| sudo rm -f "/usr/share/pixmaps/tourbox-icon.png" 2>/dev/null && \ | |
| echo -e "${GREEN}✓${NC} Removed old icon: /usr/share/pixmaps/tourbox-icon.png" || \ | |
| echo -e "${YELLOW}!${NC} Could not remove old icon (read-only filesystem)" | |
| MIGRATED_SOMETHING=true | |
| fi | |
| if [ -f "/usr/share/pixmaps/tourbox-icon.png" ]; then | |
| if sudo rm -f "/usr/share/pixmaps/tourbox-icon.png" 2>/dev/null; then | |
| echo -e "${GREEN}✓${NC} Removed old icon: /usr/share/pixmaps/tourbox-icon.png" | |
| MIGRATED_SOMETHING=true | |
| else | |
| echo -e "${YELLOW}!${NC} Could not remove old icon (read-only filesystem)" | |
| fi | |
| fi |
This updates the install script to use locations that are writable on an atomic/ostree filesystem. For example, instead of installing the launcher to /usr/local, it installs to $HOME/.local. It should also clean up files in the old locations when someone who previously installed on a non-atomic filesystem runs the install script again.