Skip to content
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

Jump to bottom

Ensure lsb-release is installed. #1215

Merged
merged 1 commit into from
Dec 5, 2022
Merged

Ensure lsb-release is installed. #1215

merged 1 commit into from
Dec 5, 2022

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Nov 30, 2022 •
edited by codeapprove bot
Loading

Part of tiny-pilot/ansible-role-ustreamer#77

As mention in tiny-pilot/ansible-role-ustreamer#78 (comment):

To accurately distinguish between Raspberry Pi OS and Debian, we need to use the ansible_lsb.id fact because ansible_distribution=='Debian' for both OSs. This fact is only defined if the lsb-release apt packages has been installed prior to ansible gathering facts.
Review on CodeApprove

@jdeanwallace jdeanwallace mentioned this pull request Nov 30, 2022
Merged
@jdeanwallace jdeanwallace marked this pull request as ready for review November 30, 2022 17:28
@jdeanwallace CodeApprove
Copy link
Contributor Author

Automated comment from CodeApprove ➜

⏳ @mtlynch please review this Pull Request

@mtlynch
Copy link
Contributor

mtlynch commented Nov 30, 2022 •
edited
Loading

How was Raspbian detection working without this change?

Adding the dependency here makes it a little hard to notice the connection between the Raspbian detection and this package. Could we add the package as an Ansible play here?

https://github.com/tiny-pilot/ansible-role-ustreamer/blob/58d9801ac32202ef85c53ff1972f3ec499c8f6fb/tasks/main.yml#L66

That way, it's obvious that we're installing it

@jdeanwallace
Copy link
Contributor Author

@mtlynch

How was Raspbian detection working without this change?

It worked because lsb-release just happens to be installed on Raspberry Pi OS. As for our CI builds, we assume the OS to not be Raspbian, so the missing package has no effect.

So I thought it would be a good idea to explicitly install lsb-release or do you think it's safe to assume it will always be installed?

From Ansible docs:

Not all facts exist for all hosts. For example, the ‘lsb_major_release’ fact used in an example below only exists when the lsb_release package is installed on the target host.

Could we add the package as an Ansible play here?
https://github.com/tiny-pilot/ansible-role-ustreamer/blob/58d9801ac32202ef85c53ff1972f3ec499c8f6fb/tasks/main.yml#L66

Unfortunately not, because the package must be installed before Ansible gathers facts (which is the first thing the playbook does). We could add this molecule hack to our install.yml playbook? But it's not my first choice.

jdeanwallace added a commit to tiny-pilot/ansible-role-ustreamer that referenced this pull request Dec 2, 2022
@jdeanwallace
Install Janus from debian-backports. (#78)
c4c6c95 
Part of #77

I was able to successfully test the following:
- Install TinyPilot Community on Hobbyist device running Raspberry Pi OS
Buster results in [Janus
`0.9.2`](https://packages.debian.org/buster-backports/janus)
- Install TinyPilot Community on Hobbyist device running Raspberry Pi OS
Bullseye results in [Janus
`1.0.1`](https://packages.debian.org/bullseye-backports/janus)
- Update TinyPilot Community (via terminal) on Hobbyist device running
Raspberry Pi OS Buster and our custom Janus package results in [Janus
`0.9.2`](https://packages.debian.org/buster-backports/janus)
- Update TinyPilot Community (via terminal) on Hobbyist device running
Raspberry Pi OS Bullseye and our custom Janus package results in [Janus
`1.0.1`](https://packages.debian.org/bullseye-backports/janus)

<s>I'm not sure / I haven't tested how this change will affect users who
already installed our custom Janus Debian package 🤔 </s>

### Notes:

1. To build uStreamer `WITH_JANUS`, we need to expose the Janus C header
files. We do this by installing
[`janus-dev`](https://packages.debian.org/buster-backports/armhf/janus-dev/filelist).
The Janus `plugins.h` file still suffers from [this
bug](tiny-pilot/ansible-role-tinypilot#192),
so we needed to [patch it
here](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/tasks/install_janus.yml#L40-L46).
2. Janus installs all plugins, transports, and loggers by default. So we
needed to [disable them
here](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/templates/janus.jcfg.j2).
3. <s>Notice the [default folder for
plugins](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/f42d012e11ffa3bcaaf3c349c644ebeb7a5657e5/vars/main.yml#L7),
I'm sure this would change depending on the device architecture.</s>
4. Janus spits out this warning when running on Buster:
    ```
[WARN] libnice version outdated: 0.1.14 installed, at least 0.1.15
recommended
    ```
5. With regards to testing with molecule, this repo specifies the
correct ustreamer version (v4 or v5) based on the debian image. This
almost guarantees ustreamer to build successfully `WITH_JANUS=1`.
However, when it comes to running molecule from
`ansible-role-tinypilot`, it [dynamically determines the ustreamer
version](https://github.com/tiny-pilot/ansible-role-tinypilot/blob/a6889cbbb4aa3b9bcee1cce8cccc1d7f78a36c63/tasks/ustreamer.yml#L7-L15).
This revealed some flaws in our ansible conditions. Namely around when
to install Janus audio packages, which [you also pointed out
here](tiny-pilot/ansible-role-tinypilot#240 (comment)).
To accurately distinguish between Raspberry Pi OS and Debian, we need to
use the `ansible_lsb.id` fact because `ansible_distribution=='Debian'`
for both OSs. This fact is [only defined if the `lsb-release` apt
packages has been
installed](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_conditionals.html#conditionals-based-on-ansible-facts)
prior to ansible gathering facts. Our docker images that molecule uses
doesn't have it installed by default, so I had to [force the
`lsb-release` installation before ansible gathers
facts](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/molecule/default/converge.yml#L4-L13).
I have also [created a
PR](tiny-pilot/tinypilot#1215) to install
`lsb-release` when we install TinyPilot.
6. The follow snippets come from the `janus-debian` repo:
- The [Janus C header
hack](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/tasks/install_janus.yml#L25-L31)
is based on [this
snippet](https://github.com/tiny-pilot/janus-debian/blob/master/Dockerfile#L97-L101).
- The [Janus websockets transport config]() is based on [this
snippet](https://github.com/tiny-pilot/janus-debian/blob/master/Dockerfile#L114-L121).
- The [main Janus
config](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/templates/janus.jcfg.j2)
is a super lean version of the [default sample
config](https://github.com/meetecho/janus-gateway/blob/v1.0.1/conf/janus.jcfg.sample.in).
    
We can archive the `janus-debian` repo after merging this PR (probably
the [`libnice-debian`
repo](https://github.com/tiny-pilot/libnice-debian) too).
7. I no longer see the value of the `janus` ansible tag. We don't use it
anywhere and there is now a few extra helper tasks (like `set_fact`,
`include_vars`) that also would need to the `janus` tag if we wanted to
run the playbook for only janus tasks. So I just [removed all janus
tags](686b253)
instead.
8. As part of the Janus installation I also [conditionally remove our
now legacy Janus package (that we
maintained)](https://github.com/tiny-pilot/ansible-role-ustreamer/blob/apt-get-janus/tasks/install_janus.yml#L2-L15).


<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/ansible-role-ustreamer/78"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
@mtlynch
Copy link
Contributor

mtlynch commented Dec 2, 2022

Ah, okay. So it'll be a no-op on Raspberry Pi OS.

I didn't understand that it had to be installed before the gather facts stage, so now I get why we want to do it here rather than in the Ansible role.

I wanted to keep the pre-Ansible apt installs limited to just the minimal set to bootstrap an Ansible environment, but I now understand that we're still observing that because lsb_release is part of the minimal Ansible environment we need.

mtlynch
mtlynch approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@mtlynch mtlynch left a 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 ➜

Approved on CodeApprove
✔️ Approved

LGTM, thanks!

👀 @jdeanwallace it's your turn please take a look

@jdeanwallace jdeanwallace merged commit e1c3647 into master Dec 5, 2022
@jdeanwallace jdeanwallace deleted the lsb-release branch December 5, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtlynch mtlynch mtlynch approved these changes

Assignees
No one assigned
Labels
codeapprove
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdeanwallace @mtlynch