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

Add support for building packages for the recording server #8788

Merged
merged 25 commits into from
Oct 4, 2023
Merged

Add support for building packages for the recording server #8788

merged 25 commits into from
Oct 4, 2023

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Feb 20, 2023 •
edited
Loading

Requires #9120

🚧 TODO

  • Add missing fields to .deb packages (like author)
  • Add additional dependencies (like ffmpeg or firefox)
  • Install server.conf.in as /etc/nextcloud-talk-recording/server.conf
  • Add systemd service to start the recording server
  • Add support to build rpm packages - Not for now, it requires even more hacking than the .deb packages

@danxuliu danxuliu added this to the 🚀 First Final (26) milestone Feb 20, 2023
@SystemKeeper
Copy link
Contributor

The recording server only provides an HTTP API. It is expected that TLS termination will be provided by an additional component, like a reverse proxy.

Just for clarification: The recording server only communicates with the Nextcloud instance, right? (In contrast to the HPB itself, which also communicates with the clients). So if NC and recording server can communicate internally, you would not need to expose the recording server externally, is that correct?
(Which does not change the fact, that a reverse proxy for TLS is needed, one way or another)

@nickvergessen
Copy link
Member

The recording server only communicates with the Nextcloud instance, right?

And with the HPB iirc

So if NC and recording server can communicate internally, you would not need to expose the recording server externally, is that correct?

Correct as per my assumptions

@danxuliu
Copy link
Member Author

The recording server only communicates with the Nextcloud instance, right?

And with the HPB iirc

Yes, both Nextcloud server and HPB.

So if NC and recording server can communicate internally, you would not need to expose the recording server externally, is that correct?

Correct as per my assumptions

Correct as well. In fact, given that it requires installing typical desktop software in a server system (like a web browser) I would firewall the recording server machine to allow access only to/from the Nextcloud server and the HPB (and maybe the STUN/TURN server, depending on the setup, although in general it should not be needed).

@SystemKeeper
Copy link
Contributor

to allow access only to/from the Nextcloud server and the HPB

Maybe we should add that as a recommendation or "best practice" or something. Docker-in-docker (dind) would be another alternative I could imagine.

@danxuliu
Copy link
Member Author

to allow access only to/from the Nextcloud server and the HPB

Maybe we should add that as a recommendation or "best practice" or something. Docker-in-docker (dind) would be another alternative I could imagine.

Of course, I intended to mention it in the setup instructions :-)

@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch 2 times, most recently from e4f3e15 to f044bce Compare March 21, 2023 14:38
@danxuliu
Copy link
Member Author

danxuliu commented Mar 22, 2023 •
edited
Loading

Sigh... I rebased the branch but forgot to commit the documentation 🤦 Anyway, fixed now. Feel free to rebase, review and merge once #9120 is merged.

@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch from 9ee878f to 21d4584 Compare March 22, 2023 14:08
SystemKeeper
SystemKeeper reviewed Mar 22, 2023
View reviewed changes
recording/docs/encoders.md Outdated Show resolved Hide resolved
recording/docs/installation.md Outdated Show resolved Hide resolved
recording/docs/installation.md Outdated Show resolved Hide resolved
recording/docs/installation.md Outdated Show resolved Hide resolved
@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch from 21d4584 to 34994b8 Compare March 22, 2023 21:54
@danxuliu danxuliu marked this pull request as ready for review March 22, 2023 21:59
@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch 2 times, most recently from 0da747c to 5656d1b Compare March 23, 2023 00:40
@danxuliu
Make builds reproducible
178d848 
stdeb supports setting SOURCE_DATE_EPOCH to have reproducible builds, so
it is now set to the git timestamp of the recording directory for the
recording server and the date of the root directory in the source Python
package for the dependencies.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add example configuration in the recording server package
cc27cf4 
Installed files can not be renamed with the standard ".install" syntax;
"dh-exec" needs to be used instead.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add executable for the recording server
ff34f67 
Rather than having to call "python3" and specify the module an
executable to be called just as "nextcloud-talk-recording" was added for
convenience.

However, right now it is not possible to just specify a module to be
loaded; as a function is needed to entry module is now wrapped in a
function that is explicitly called for the executable and implicitly
called when the module is loaded, thus keeping the previous behaviour.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add option to just show the version and exit
3ed6f23 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add systemd service for the recording server
dc3b015 
The recording server does not need to be run as root, although it needs
a home directory to store some Firefox and Pulseaudio related files (not
explicitly created by the recording server but by the services
themselves). Therefore a system user, which can not log in, but with a
home directory, is used to run it.

The systemd addon for dh is used when creating the package, so
"dh_systemd_enable" and "dh_systemd_start" will be automatically called
when building the package. Those helpers will take care of enabling and
starting the service as needed when the package is installed.
Nevertheless, as a custom ".postint" script is used "#DEBHELPER#" needs
to be explicitly called to add the code of the helpers.

Unfortunately the service is named "python3-nextcloud-talk-recording",
just like the package, as renaming the service requires a "debian/rules"
file with a content like:

override_dh_installsystemd:
        dh_installsystemd --name=nextcloud-talk-recording

but stdeb provides its own "debian/rules" files and there does not seem
to be a way to extend/customize it.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add helper script to build the packages
22e0394 
The script creates one container for each supported distribution,
installs the needed dependencies and then builds the packages.

The recording server directory is mounted in the containers so the
packages can be built in the "build" subdirectory of the actual
recording server directory. In the containers the building is done as
the owner of the recording server directory, so the built files are also
owned by that user.

For simplicity, and given that the containers are not expected to be
used for anything else, the volume with the recording server is mounted
as a direct child of the root directory (forgive me FSH for I have
sinned).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add dependency and executable for the benchmark tool
79b4e59 
The benchmark tool has a specific dependency (psutil) that is not
required for the rest of the recording server. However, for simplicity,
rather than splitting the benchmark in its own Python project and
package its dependency and helper script is now included in the main
project and package (the code was already there).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Rename package to "nextcloud-talk-recording"
f1528c8 
By default stdeb prefixes the package name with "python3-". However,
even if other Python modules could import the Python code of the
recording server it is not designed as a library, it is an executable
that happens to be written in Python, so to speak.

Moreover, besides the Python code itself the package also includes a
configuration file and a systemd service, which do not belong to
"python3-" packages (there are also helper executable scripts, although
those are allowed in "python3-" packages).

Due to all that the package "python3-nextcloud-talk-recording" is now
renamed to "nextcloud-talk-recording".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch from 5656d1b to 2b3c9d0 Compare September 27, 2023 06:41
SystemKeeper
SystemKeeper reviewed Sep 27, 2023
View reviewed changes
recording/docs/installation.md Outdated Show resolved Hide resolved
recording/docs/installation.md Outdated Show resolved Hide resolved
@SystemKeeper
Copy link
Contributor

Building the packages through build.sh worked for me, but currently no system available to install these deb packages.

@danxuliu
Add support for building packages in Debian 11
2b27419 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch from 793d2fd to 4c3e06c Compare October 2, 2023 10:37
@danxuliu
Copy link
Member Author

danxuliu commented Oct 3, 2023

While referencing the documentation elsewhere I realized that the installation was described without having described first the machine that the recording server should be installed in. Due to that I moved the System setup introduction and the Hardware requirements to the beginning of the document.

At the same time I am not fully convinced of the change, though, but I do not know why 🤷

SystemKeeper
SystemKeeper approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested after the latest changes, worked and generated the following files:

./ubuntu22.04/deb/python3-selenium_4.8.2-1~ubuntu22.04_all.deb
./ubuntu22.04/deb/python3-pulsectl_22.3.2-1~ubuntu22.04_all.deb
./ubuntu22.04/deb/nextcloud-talk-recording_0.1-1~ubuntu22.04_all.deb
./ubuntu22.04/deb/python3-certifi_2022.12.7-1~ubuntu22.04_all.deb
./ubuntu22.04/deb/python3-trio-websocket_0.9.2-1~ubuntu22.04_all.deb

./ubuntu20.04/deb/python3-selenium_4.8.2-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-urllib3_1.26.14-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/nextcloud-talk-recording_0.1-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-requests_2.25.0-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-certifi_2022.12.7-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-pulsectl_22.3.2-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-pyvirtualdisplay_3.0-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-trio-websocket_0.9.2-1~ubuntu20.04_all.deb
./ubuntu20.04/deb/python3-trio_0.21.0-1~ubuntu20.04_all.deb

./debian11/deb/python3-certifi_2022.12.7-1~debian11_all.deb
./debian11/deb/python3-pulsectl_22.3.2-1~debian11_all.deb
./debian11/deb/python3-trio_0.21.0-1~debian11_all.deb
./debian11/deb/python3-selenium_4.8.2-1~debian11_all.deb
./debian11/deb/nextcloud-talk-recording_0.1-1~debian11_all.deb
./debian11/deb/python3-trio-websocket_0.9.2-1~debian11_all.deb
./debian11/deb/python3-pyvirtualdisplay_3.0-1~debian11_all.deb

Also the changes made to the documentation make sense to me.

@nickvergessen
Copy link
Member

Then time to fixup

@danxuliu
Add documentation for installing the recording server
54a747a 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix typo
e6cd521 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the add-support-for-building-packages-for-the-recording-server branch from 78ea1f6 to e6cd521 Compare October 3, 2023 23:13
@danxuliu danxuliu merged commit 77a4405 into master Oct 4, 2023
26 checks passed
@danxuliu danxuliu deleted the add-support-for-building-packages-for-the-recording-server branch October 4, 2023 12:21
@nextcloud nextcloud deleted a comment from backportbot-nextcloud bot Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@szaimen szaimen szaimen left review comments

@SystemKeeper SystemKeeper SystemKeeper approved these changes

Assignees

@danxuliu danxuliu

Labels
3. to review enhancement feature: recordings ⏺️ Including the recording server
Projects
💬 Talk team
Archived in project
Milestone
v18.0.0-beta.1
Development

Successfully merging this pull request may close these issues.
4 participants
@danxuliu @SystemKeeper @nickvergessen @szaimen