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.

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

Build uStreamer Debian package with Janus plugin #6

Merged
merged 40 commits into from
May 24, 2023

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented May 12, 2023

Resolves #3

This PR builds a uStreamer Debian package (with Janus plugin) in CI. The reason why we couldn't just use the official(?) uStreamer Debian package is because it doesn't compile the Janus plugin (which we need for WebRTC support).

Now we can avoid building uStreamer from source every time TinyPilot is installed or updated.

You can test the uStreamer Debian package on a device via this scratch TinyPilot Pro build bundle.

Notes:

Helpful resources on creating Debian packages:

Review on CodeApprove

@mtlynch
Copy link
Contributor

mtlynch commented May 12, 2023

We have another PR in progress that does ARMv7 + AMD64 builds in Docker+QEMU if it's helpful for reference:

https://github.com/tiny-pilot/tinypilot/pull/1352/files

@jdeanwallace jdeanwallace changed the base branch from master to port-scripts May 16, 2023 11:32
@jdeanwallace jdeanwallace changed the title Build debian pkg Build uStreamer Debian package with Janus plugin May 19, 2023
@jdeanwallace jdeanwallace marked this pull request as ready for review May 19, 2023 14:26
Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jotaen4tinypilot please review this Pull Request

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 ➜

Cool! Excited to see this coming through!


In: debian/ustreamer.copyright:

> Line 2
Copyright (c) 2023 TinyPilot, LLC <legal@tinypilotkvm.com>

In addition to the copyright notice, we also need to include the GPLv3 license in the package:

https://github.com/tiny-pilot/ustreamer/blob/28c85991672b43b5a0e1c17329c000308293f5ba/LICENSE


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

Copy link

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

⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved

Pretty cool! Thanks for providing all the links and the detailed instructions, that was really helpful for understanding the structure and how it all ties together.

I’ve tested the package installation on device both with the bundle and standalone.


In: Discussion
Just a philosophical thought, on an unrelated note: I wonder if at some point we should write up the ultimate guide to creating Debian packages that we had wished for when we started. It’s cool to see how much knowledge we’ve built up over the last months, and what we are able to achieve now. But at the same time, when considering all the parts that are involved, and all the little details (from lintian over Docker build kit to the Debian helper tools), it’s also impressive how much complexity it takes to do something seemingly simple like bundling up a few files.


In: Dockerfile:

> Line 39
ENV PKG_NAME='ustreamer'

Does that have to be ENV, or could it be ARG?


In: Dockerfile:

> Line 70
RUN mkdir -p /build/placeholder-pkg-id

(optional) I think the WORKDIR directive creates the desired directory automatically, so technically, we might not have to mkdir -p beforehand. It might be clearer with it, though.


In: Dockerfile:

> Line 108
Homepage: https://pikvm.org/

Shall we link the PiKVM website or the uStreamer Github repo here? My thinking would be that on the PiKVM website, you wouldn’t find anything about uStreamer directly.


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

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 ➜

In: Dockerfile:

> Line 108
Homepage: https://pikvm.org/

That's true. Let's link to https://github.com/tiny-pilot/ustreamer


👀 @jdeanwallace, @jotaen4tinypilot 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
Yeah that would be one hell of a blog post/guide. This has been my 2nd attempt at building a Debian package (this was my 1st) and things were way easier this time, mostly thanks to Michael's breakthrough on how to use debhelper and this blog post. So even though this time things went smoothly, I'm sure there's a lot more that I'm missing. That being a said, I would have wished for a quick start guide on debhelper, but I think that's mostly covered here.


In: debian/ustreamer.copyright:

> Line 2
Copyright (c) 2023 TinyPilot, LLC <legal@tinypilotkvm.com>

Done.


In: Dockerfile:
It could be ARG too. I just went with ENV because I didn't think we'd need to change it. Should I be using ARG instead here?


In: Dockerfile:
Done.


In: Dockerfile:
Oh cool, I didn't know that, thanks. Removed.


In: Dockerfile:
Oh nevermind, I just used ARG just incase.


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

Copy link

@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: I have approved this change on CodeApprove and all of my comments have been resolved.

@jdeanwallace
Copy link
Contributor Author

jdeanwallace commented May 24, 2023

@mtlynch - Just a heads up, I already got an approval on this PR, but the PR is still blocked on your change requests 🤔

I have tried re-syncing the PR on codeapprove, but it didn't help:
Screen Shot 2023-05-24 at 14 17 50

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


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

@mtlynch
Copy link
Contributor

mtlynch commented May 24, 2023

@jdeanwallace - Fixed, sorry! Raised it up with CodeApprove as well: codeapprove/feedback#80 (comment)

@jdeanwallace jdeanwallace merged commit 258e1e4 into master May 24, 2023
@jdeanwallace jdeanwallace deleted the build-debian-pkg branch May 24, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a Dockerfile that builds a uStreamer Debian package from source
3 participants