-
-
Notifications
You must be signed in to change notification settings - Fork 253
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.
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
Reinstallation using provided scripts no longer possible #1662
Comments
Can you update the description with a bit more context? Can we include repro steps + expected result vs actual result? Do we know the PR where the regression began? |
@mtlynch - I've added a bit more context to the description. Regarding reproduction, it's tricky as the issue has no direct implications and only affects other processes. I've instead added a few examples of problems that could result from it and how to reproduce those. This regression will have been introduced when we switched to using Debian packaging, which I believe was during this change. |
That sounds like a pretty broad fix for what seems to be a niche issue. Doesn't that also drive up our disk writes for each install? I think we can probably find a more targeted fix for the problems it's creating.
This is a problem, but can we fix it at the instructions layer rather than adjusting our install logic for everyone?
This isn't really a bug. If the user edits TinyPilot files, the results are undefined. I think users would generally prefer this, as they'd probably prefer not to keep making local edits. If users want to undo local changes, we can provide instructions for uninstalling / reinstalling. |
The question comes down to what a user would expect to happen if they run the installation scripts on a system that already has TinyPilot installed. The previous behavior was to perform a reinstallation, which seems sensible to me.
A fresh installation would always install all three packages, so using the
We can create targeted fixes for the two example issues described above. However, we'd also need to consider any other scenario where we've recommended reinstallation using this method. There might also be implications when going from Community to Pro and vice-versa.
The manual solution is to remove the
I intended this example to artificially simulate issues such as file corruption rather than an intentional edit by a user. This example is relevant because we recommend that users re-run the installation scripts as our "Fallback method" of reinstallation, something we usually suggest when TinyPilot shows corruption but the rest of the system appears stable. Unfortunately, due to the behavior change described by this bug, this reinstallation method wouldn't actually reinstall anything and thus won't fix any issues. We'd need to either fix the regression, update the TinyPilot Pro download page to include a reinstallation section, or stop suggesting this reinstallation method altogether. |
Right, it's sensible to reinstall TinyPilot if the user tries to run installation scripts on a system that already has TinyPilot. The part I'm saying is a broad fix would be applying If we have a way of applying
Wouldn't this trigger reinstalls of our third-party dependencies on every install even if they haven't changed since the last TinyPilot release?
For me, the decision comes down to how many users the change affects and how many unknown-unknowns might affect them. The percentage of users who install first for HDMI to USB and then want to switch to HDMI to CSI is probably 0.2%, and probably almost nearly zero TinyPilot Pro users. The percentage of users who have a corrupt filesystem and whose systems get fixed by re-running install is probably about 0.2% as well. If we change to Then the question is: what are the chances of introducing an unexpected side-effect by installing with
I think it makes more sense here to update our support guidance for rare scenarios than to change the core install logic if we can achieve the same thing in either case. I'd be more in favor of adjusting core logic to reduce the number of users who require explicit support instructions, but in the scenarios we're talking about, it's all cases where the user has to run extra commands that we provide outside of the normal install, so we might as well target our adjustments to those cases where we have to provide targeted fixes. |
It's my understanding that the only impact of changing the command to
I've tested this and verified that dependencies aren't affected. Only the packages I specified as part of the
I'm happy to implement the changes to our documentation if that's the preferred approach. I've had a quick check around, and I believe we'd need to make changes in the following places:
|
Still, I think we're introducing too much uncertainty by switching to the non-standard path on such a delicate part of our install. Let's stick with
Is it possible for us to just provide commands that will work either way rather than push additional work onto the user?
Isn't the README the same? If we assume they're using an HDMI to USB dongle, which is the common case, the simple install works. If they have HDMI to CSI, then the README points them to the advanced install. The advanced install should cover the case when they installed for HDMI to USB and are switching to HDMI to CSI, so the README should stay the same.
This should stay the same. The commands assume that the user has a working version of TinyPilot Community and is upgrading to TinyPilot Pro. |
Detecting if a package is installed is surprisingly complicated, so it's not something we'd want to do in a snippet. We could use the following commands if we're happy with always removing the package before reinstalling it: (sudo apt-get remove --yes tinypilot || true) && \
curl [...]
We currently have this text after the simple installation instructions:
However, the positioning and wording don't indicate that this is a different type of installation and imply that the user can make the changes after installation. I'd suggest changing it to:
This approach means our playbook's reinstallation "fallback method" instructions aren't valid. Should we update the playbook to directly contain the relevant commands (as opposed to our current process of asking users to go through the license check page), or should we drop it as a troubleshooting step? |
Yep, that looks good to me. In the wiki specifically, though, not in our standard install, as it's only necessary for an advanced install.
If we change the wiki to add the If that's true, then the guidance on the standard
I can't find the documentation you mean. Can you link it here? Linking to Notion even in public is fine because Notion does its own auth. |
Thanks Michael!
That's true, although it introduces extra disk writes. If you're confident this section is clear enough, then I'm happy to leave it as is.
Good point! The relevant section is here. |
Yep, this is fine. It's not a big deal to add disk writes in a case where a very small number of users will go through the workflow (like switching from USB to CSI) or if the filesystem is already corrupt and we're trying to repair it.
Ah, right. Yeah, we should update that section to unconditionally remove tinypilot before reinstalling. So the work items for closing this bug are:
|
Thanks @mtlynch. Can you please confirm you're happy for me to make the following change to the relevant part of the wiki?
I've also put a similar change through on Notion. |
Wouldn't it need to be (sudo apt remove --yes tinypilot || true) && \
curl ... Otherwise, the command would fail on systems where the But outside of that, yep, that's what I had in mind. Same thing for Notion - we just need to make it work in both the case where |
Thanks @mtlynch, I've made the required changes! |
Great, thanks! |
We recently switched to using Debian packaging, and as a result, simply rerunning the standard installation commands doesn't result in a complete reinstallation as it did previously. This issue occurs because
apt
won't reinstall the Debian packages if they are already at the latest version.Implications
This change in behavior means some configuration changes are only picked up on a fresh installation or an upgrade. For example, switching between a USB HDMI capture device and a TC358743 HDMI capture device is no longer possible by simply modifying
/boot/config.txt
and reinstalling TinyPilot.Example 1: Switching capture devices fails
Steps to reproduce
Actual outcome
TinyPilot remains configured for a USB HDMI adapter, which can be seen as the symlink target of
/opt/ustreamer-launcher/configs.d/000-defaults.yml
remains as/opt/ustreamer-launcher/config-library/hdmi-to-usb.yml
.Expected outcome
TinyPilot becomes configured for the TC358743 chip, which can be seen as the symlink target of
/opt/ustreamer-launcher/configs.d/000-defaults.yml
changes to/opt/ustreamer-launcher/config-library/tc358743.yml
.Example 2: Mangled file not restored
Steps to reproduce
/opt/tinypilot/app/version.py
and add a comment to the first line.Actual outcome
The file isn't modified.
Expected outcome
The file is replaced with the version shipped in the latest release.
Suggested fix
There are several approaches we could take to fix this issue. I've done some brief experimentation, and my suggested strategy would be to go with a basic change of modifying the installer script to use
apt-get reinstall
instead ofapt-get install
:tinypilot/bundler/bundle/install
Line 57 in 951b9bf
The text was updated successfully, but these errors were encountered: