Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Optionally install uStreamer from Debian package #110

Merged
merged 3 commits into from
May 24, 2023

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented May 18, 2023

Related #100

This PR only builds uStreamer from source if a uStreamer Debian package is not provided. This will allow TinyPilot to specify a uStreamer Debian package and avoid building uStreamer from source on every install/update.

I've tested these changes in CI via 179fe04.

I've also tested these changes on device via a scratch TinyPilot Pro PR.

Review on CodeApprove

@jdeanwallace jdeanwallace changed the title Optionally install uStreamer Debian package Optionally install uStreamer from Debian package May 22, 2023
This reverts commit 179fe04.
@jdeanwallace jdeanwallace marked this pull request as ready for review May 22, 2023 12:48
Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jotaen4tinypilot please review this Pull Request

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot 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 ➜

In: Discussion
I didn’t get around to reviewing this yet, but I installed the bundle from the scratch PR while testing tiny-pilot/ustreamer-debian#6. I noticed that it still installs Janus via Ansible during the bundle installation:

TASK [ansible-role-ustreamer : enable Janus apt suite] ******************************************
changed: [localhost]

TASK [ansible-role-ustreamer : install Janus package and C header files] ************************
changed: [localhost]

TASK [ansible-role-ustreamer : patch Janus plugin.h file to successfully include refcount.h file] ***
changed: [localhost]

Wouldn’t that be obsolete when we install the uStreamer Debian package? (I might be missing something here, though?)


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

Copy link
Contributor Author

@jdeanwallace jdeanwallace 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 ➜

In: Discussion
Oh no, the uStreamer Debian package just includes the uStreamer Janus plugin and not Janus itself (although Janus is indicated as a dependency). We still need to install Janus alongside uStreamer for WebRTC to work.


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

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot 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

👍

I’ve tested on device via the scratch PR.


In: Discussion
Ah right, thanks – I forgot that Janus and the Janus plugin are two separate things!


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

@jdeanwallace jdeanwallace merged commit 02f37b8 into master May 24, 2023
@jdeanwallace jdeanwallace deleted the ustreamer-debian branch May 24, 2023 14:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants