-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Set up TinyPilot virtual environment from Debian package #1352
Conversation
I think we jinxed it 🙃 @mtlynch - This PR is ready to be reviewed. Notes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
LGTM, but it seems like I can't actually approve since I started the PR.
In: Discussion
Can we update the PR description to include (3) and (4) from this note?
In: Discussion
This is a funny PR to review since I started it and you finished it. It's like async pair programming.😃
I'm afraid I'm going to complain about something and then find out I wrote the line.
In: .circleci/continue_config.yml:
> Line 105
command: find . -name '*.deb' -exec mv {} ./debian-pkg/releases/ \;
Can we add a comment explaining this one? Basically just your point (5) from this note.
In: bundler/create-bundle:
> Line 75
# Clear Ansible roles.
Can we clarify that this is specifically clearing Ansible role files that exist from previous bundle builds?
(out of scope, optional) I think it might make more sense now or in the future for us to do something like this at the beginning of the script so that the bundle is always a clean slate:
BUNDLE_DIR="$(mktemp --directory)"
readonly BUNDLE_DIR
cp -r bundle/* "${BUNDLE_DIR}"
In: debian-pkg/Dockerfile:
> Line 97
RUN set -x && \
Can we do set -xu
or set -exu
since we're using env vars and we want to fail if any are blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
LGTM, but it seems like I can't actually approve since I started the PR.
Allow me 😌
In: Discussion
Done.
In: Discussion
Don't worry, I didn't say a word 🤐
In: .circleci/continue_config.yml:
> Line 113
command: find . -name '*.deb' -exec mv {} ./debian-pkg/releases/ \;
Done.
In: bundler/create-bundle:
Can we clarify that this is specifically clearing Ansible role files that exist from previous bundle builds?
Done.
(out of scope, optional) I think it might make more sense now or in the future for us to do something like this at the beginning of the script so that the bundle is always a clean slate...
I think this might be a little tricky because copying the contents of bundle/
would also copy the "dirty" files/directories, so I didn't do it in this PR.
In: debian-pkg/Dockerfile:
Done.
👀 @mtlynch it's your turn please take a look
I'm not breaking protocol and merging our PR. |
This change integrates the dh-virtualenv extension to debhelper to install TinyPilot's Python virtual environment as part of the Debian package install.
This seemed like it would be easy, but it turned out to be fairly complicated. Putting the virtualenv into the Debian package makes the Debian package architecture-specific, so we have to spin up QEMU and manage architecture-specific bits throughout the build process.
What this PR does
Reduces disk writes on customer devices
The advantage of this change is that on customer devices, this reduces disk writes. Instead of installing/compiling Python packages on the device, most of that work is done ahead of time, so we're putting less wear on the customer's microSD.
Speeds up installs/updates
Similarly to the reduction in disk writes, it should speed up installs. We don't have to go through the install process for the virtualenv on the end-user's device because the packages are pre-provisioned in the Debian package.
Reduces Ansible code
This change continues our "war on Ansible," reimplementing Ansible logic as Debian package logic.
Makes TinyPilot's Debian package architecture-dependent
Previously, the TinyPilot Debian package could declare an architecture of
all
because it didn't contain any native binaries. Usingdh-virtualenv
means that TinyPilot's third-party libraries are now packaged inside the Debian package, and some of the dependencies are architecture-dependent, which means the package as a whole is architecture dependent.Only builds AMD64 in PRs
Because we have to emulate ARMv7 from CI, this change slows down Debian package building significantly. It's about a 20x slowdown, increasing build time for the Debian package from ~30s to ~6m.
I considered building in ARM on CircleCI, but we need 32-bit ARM, and CircleCI only supports 64-bit.
https://circleci.com/docs/using-arm/#limitations
With that in mind, we now only build AMD64 in PRs and ARMv7+AMD64 in master/releases.
Notes
Lintian Override Syntax
I find the documentation for Lintian overrides really confusing. I got the overrides by copying errors from Lintian and pasting them into the overrides. What I put in doesn't seem to match Lintian's documented syntax, but when I try to adjust the overrides to match my understanding of the documentation, the overrides fail.
Debian package hardening
In an effort to resolve a
hardening-no-relro
lintian issue, I enabled Debian package "hardening":tinypilot/debian-pkg/debian/rules
Lines 3 to 6 in 79fe3e6
But to be honest, I don't know what it really does for our package. It didn't resolve the lintian issue, but it seems to be something to do with package security, so I kept it in.
install-from-source
scriptTo get the
install-from-source
script working again, I mostly just needed to clean out previous ansible roles before creating the bundle.