Fix install blocks #1858
Fix install blocks #1858
Conversation
Automated comment from CodeApprove ➜⏳ @cghague please review this Pull Request |
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
We designed the existing install blocks as a deny list to prevent installation in situations where we knew it wouldn't work. We opted for this over an allow list as, while we haven't actively encouraged it, we have previously supported installation on other compatible devices. This change would block that. Is that the intention here?
👀 @db39 it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
We designed the existing install blocks as a deny list to prevent installation in situations where we knew it wouldn't work.
The existing blocks allow users to attempt an installation in situations where we know it doesn't work (64-bit Raspberry Pi OS), so the current blocks aren't fully working as intended. We've had multiple (frustrated) community users reach out to let us know they can't install on Bookworm 64-bit. And since that's the current 'default' OS for Raspberry Pi at the moment, I think we should try to handle that condition.
Installation on other devices / operating systems already requires adaptations to get things working, so I don't think changing the installation blocks would cause an issue, particularly when so few people try it. The exception here would be Ubuntu 20.04 32-bit, which has been reported to work with the standard installer, but would be blocked with this change.
One option is to check for non-Raspberry Pi 32-bit operating systems ("$(lsb_release --id --short)" != 'Raspbian'), show a warning message, then get user input to confirm and continue installation. I'd be concerned some users wouldn't read the warning message and continue to attempt to install regardless, but it allows us to restrict installation to 32-bit Bullseye while letting people on other systems attempt to install if they want. What do you think?
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
There are gaps in the current implementation, but we should carefully consider how to handle them. I see two approaches:
-
Maintain the current deny list and add the 64-bit version of Bullseye as a new item. This approach makes it easier to offer informative error messages and allows installation on other hardware. It's somewhat of a maintenance burden, and some incompatible systems can slip through.
-
Switch to an allow list comprised of only the Raspberry Pi 4B running Bullseye (32-bit) and the Raspberry Pi Zero 2 running Bullseye (32-bit). If we did this, we could use an environment variable (e.g.,
export FORCE_COMPATIBLE=1) to allow users to bypass the restriction on other hardware. An environment variable is probably enough of a filter to ensure only users who need it will use it.
My inclination is to go for option 2, but I'm open to either.
👀 @db39 it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Yeah, option 2 (switch to an allow list with a force-install flag) sounds like a good way of handling this.
I'd be inclined to go a step further and restrict installation on Pi Zero 2 devices. i.e., installation can only proceed on our officially-supported configurations (currently only Pi 4B with Bullseye 32-bit), and users with other setups can bypass that restriction manually.
If that sounds reasonable, I'll update the install script to reflect that.
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
We have a page for the Raspberry Pi Zero, so we should allow it by default or update the documentation. I'd suggest we only allow installation if the following conditions are all met unless the user has set the environment variable to force it:
- Raspberry Pi 4B or Raspberry Pi Zero
- Bullseye
- 32-bit
👀 @db39 it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
so we should allow it by default or update the documentation
I think updating the documentation would be the best solution here. I'm happy to help support other configurations, but I think a restrictive allow list is a sensible default that fits in with the "Simple install" option - particularly when we have some explicit requirements.
What do you think to:
- Updating our Installation Options page documenting the force install option, explaining that the simple install only allows installs on officially-supported systems, but that you can force an install using the flag.
- Adding a note and a link on the Pi Zero page to the installation options force install option.
- Possibly adding another sentence at the bottom of the Simple Install section mentioning installation on other devices/setups.
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
That sounds like an excellent plan! The key is to clearly distinguish between supported, working, and broken.
👀 @db39 it's your turn please take a look
|
@cghague - Cool, thanks! Since those pages are Wikis rather than part of a repo, I'll share the proposed changes to each page here. Once we're happy with these proposed changes and we've merged the install script changes, I'll update the Wiki pages. Here are my proposed changes to Installation Options (the addition of a "Forcing installation" section): ## Simple installation
For a simple installation/upgrade of TinyPilot, see the [instructions in the main README](https://github.com/tiny-pilot/tinypilot/blob/master/README.md#simple-installation).
## Advanced installation
### Forcing installation
The Simple installation restricts installation to only officially supported configurations of hardware and software.
If you'd like to try and install and run TinyPilot on different hardware or a different operating system, you can force installation by setting the `FORCE_INSTALL` environment variable before running the Simple installation commands:
```bash
export FORCE_INSTALL=1
```
...
And here's my proposed change to the Pi Zero page (Adding a sentence about forcing installation with a link to the installation options page): The Pi Zero (W) is not officially supported by TinyPilot, but it offers a low-performance TinyPilot experience if your resources are limited.
To install TinyPilot on a Pi Zero (W), follow the [Advanced installation steps](https://github.com/tiny-pilot/tinypilot/wiki/Installation-Options#forcing-installation) for forcing installation on unsupported system configurations.
...
|
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
@db39 - Great, thanks! Let me know when you're happy with the script, and I'll review it one last time. My feedback for the wiki pages is below:
"Installation options" wiki page
Overall, I like this. I'd suggest dropping the leading The from the paragraph directly after the Forcing Installation header, dropping and run from the paragraph after, and making running the Simple installation into a link. We also need a header for the following content - perhaps ### Configuration options?
"Pi Zero Optimizations" wiki page
Your changes look good!
👀 @db39 it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Thanks! I like all of those changes you suggested.
I think I updated the script last week, so it should be ready to review. My one concern is the force install check (if [[ -z "${FORCE_INSTALL+x}" ]]; then) - I don't think we've used an environment variable check where the environment variable can be unset. One option is to not use set -u until after the force install check.
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Thanks @db39! I thought it was ready but was unsure as CodeApprove is in a strange state! I'll mark this thread as resolved.
In: bundler/bundle/install:
> Line 23
if [[ -z "${FORCE_INSTALL+x}" ]]; then
Suggest flipping the logic as follows to maintain context:
if [[ -n "${FORCE_INSTALL+x}" ]] ; then
# force install
else
# regular checks
fi
In: bundler/bundle/install:
> Line 28
echo 'You are trying to install on incompatible hardware.' >&2
Suggest unsupported hardware.
In: bundler/bundle/install:
> Line 39
echo "TinyPilot currently only supports the 32-bit version of Raspberry Pi OS 11 \"Bullseye\" " >&2
Can we add a full stop to the end of the line?
In: bundler/bundle/install:
> Line 40
echo "To install TinyPilot, you'll need to install Raspberry Pi OS 11 \"Bullseye\" (32-bit)." >&2
Suggest dropping to install.
In: bundler/bundle/install:
> Line 58
elif (( "${FORCE_INSTALL}" == 1 )); then
The only two scenarios are either a supported or forced installation, so should this be an else (see L23 comment for further details)?
👀 @db39 it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: bundler/bundle/install:
Resolved
In: bundler/bundle/install:
Resolved
In: bundler/bundle/install:
Resolved
In: bundler/bundle/install:
Resolved
In: bundler/bundle/install:
Resolved
👀 @cghague it's your turn please take a look
|
Back reference: the disk image build via Packer is currently failing. |
Resolves #1857
This change refactors our installation blocks to use single conditions, fixing a bug when checking for Raspbian-based systems that allowed installations to proceed on 64-bit systems regardless of the OS version.
This also adds a check for a
FORCE_INSTALLflag - setting theFORCE_INSTALLenvironment variable allows you to bypass the system checks to attempt an install on unsupported systems.