Bug 2277423 - Review Request: nextcloud-spreed-signaling - Standalone signaling server which can be used for Nextcloud Talk
Summary: Review Request: nextcloud-spreed-signaling - Standalone signaling server whic...
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 2277416 2277418 2277421 2277422
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-04-26 20:41 UTC by Renich Bon Ciric
Modified: 2024-05-21 07:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Renich Bon Ciric 2024-04-26 20:41:39 UTC
Spec URL: https://fedorapeople.org/~renich/signaling/nextcloud-spreed-signaling.spec
SRPM URL: https://fedorapeople.org/~renich/signaling/nextcloud-spreed-signaling-1.2.4-1.fc39.src.rpm
Description: Standalone signaling server which can be used for Nextcloud Talk
Fedora Account System Username: renich

Comment 1 Mikel Olasagasti Uranga 2024-04-27 13:44:35 UTC
- This one can be removed: docker/README.md

> %global godocs          docs CHANGELOG.md README.md docker/README.md


- Although correct, I think this time these can be conveted to "BuildRequires: /usr/bin/easyjson" type of BR.

> BuildRequires:  golang-github-mailru-easyjson
> BuildRequires:  golang-google-grpc
> BuildRequires:  golang-google-protobuf


- If you plan to use `make` you need a BR on it.


- Doesn't setting GO* env vars before make work?

> make GOPROXY=off GO111MODULE=off GOPATH=%{gobuilddir}:%{gopath} common


- Is client required? Makefile's build section only builds server and proxy.

> %gobuild -o %{gobuilddir}/bin/client %{goipath}/client

Documentation refers to it as "A simple client exists to benchmark the server."


- Change from this:

> %{_bindir}/*

to: 

%{_bindir}/signaling
%{_bindir}/proxy
%{_bindir}/client


- You can remove these to create a binary only package:

> %gopkginstall
> (...)
> %gopkgfiles


- IIUC this package is meant to be used as a daemon by default? If so systemd definition and user creation is missing.

https://github.com/strukturag/nextcloud-spreed-signaling/blob/master/README.md#running-as-daemon


- Does it make sense to split the package into signaling and proxy subpackages? Or add the proxy subpackage?

Comment 2 Renich Bon Ciric 2024-04-29 16:54:41 UTC
(In reply to Mikel Olasagasti Uranga from comment #1)
> - This one can be removed: docker/README.md
> 
> > %global godocs          docs CHANGELOG.md README.md docker/README.md

Done.

> - Although correct, I think this time these can be conveted to
> "BuildRequires: /usr/bin/easyjson" type of BR.
> 
> > BuildRequires:  golang-github-mailru-easyjson
> > BuildRequires:  golang-google-grpc
> > BuildRequires:  golang-google-protobuf

Done.

> - If you plan to use `make` you need a BR on it.

Done.
 
> - Doesn't setting GO* env vars before make work?
> 
> > make GOPROXY=off GO111MODULE=off GOPATH=%{gobuilddir}:%{gopath} common

No; still getting build errors:

Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.mz5mbd
+ umask 022
+ cd /builddir/build/BUILD
+ cd nextcloud-spreed-signaling-1.2.4
+ GOPROXY=off
+ GO111MODULE=off
+ GOPATH=/builddir/build/BUILD/nextcloud-spreed-signaling-1.2.4/_build:/usr/share/gocode
+ make common
protoc \
        --go_out=. --go_opt=paths=source_relative \
        grpc_backend.proto
protoc \
        --go_out=. --go_opt=paths=source_relative \
        grpc_internal.proto
protoc \
        --go_out=. --go_opt=paths=source_relative \
        grpc_mcu.proto
protoc \
        --go_out=. --go_opt=paths=source_relative \
        grpc_sessions.proto
protoc \
        --go-grpc_out=. --go-grpc_opt=paths=source_relative \
        grpc_backend.proto
protoc \
        --go-grpc_out=. --go-grpc_opt=paths=source_relative \
        grpc_internal.proto
protoc \
        --go-grpc_out=. --go-grpc_opt=paths=source_relative \
        grpc_mcu.proto
protoc \
        --go-grpc_out=. --go-grpc_opt=paths=source_relative \
        grpc_sessions.proto
rm -f easyjson-bootstrap*.go
easyjson -all api_async.go
Error parsing api_async.go: file '/builddir/build/BUILD/nextcloud-spreed-signaling-1.2.4/api_async.go' is not in GOPATH '/builddir/build/BUILD/nextcloud-spreed-signaling-1.2.4/_build:/usr/share/gocode'
make: *** [Makefile:102: api_async_easyjson.go] Error 1

The full log is at: https://paste.centos.org/view/08c1fffe

> - Is client required? Makefile's build section only builds server and proxy.
> 
> > %gobuild -o %{gobuilddir}/bin/client %{goipath}/client
> 
> Documentation refers to it as "A simple client exists to benchmark the
> server."

It is needed for the tests. I don't think it is needed in the actual package. I don't think proxy is required at all either. 
 
> 
> - Change from this:
> 
> > %{_bindir}/*
> 
> to: 
> 
> %{_bindir}/signaling
> %{_bindir}/proxy
> %{_bindir}/client

Done.

> - You can remove these to create a binary only package:
> 
> > %gopkginstall
> > (...)
> > %gopkgfiles

Done.

> - IIUC this package is meant to be used as a daemon by default? If so
> systemd definition and user creation is missing.
> 
> https://github.com/strukturag/nextcloud-spreed-signaling/blob/master/README.
> md#running-as-daemon

You are right. I haven't included the systemd service unit. I will do this as soon as I can make the package build.

> - Does it make sense to split the package into signaling and proxy
> subpackages? Or add the proxy subpackage?

I think it is worth splitting. You don't always need a proxy. I will do this as soon as I can make the package build.

Comment 3 Renich Bon Ciric 2024-04-30 02:14:29 UTC
OK, package built thanks to Mikel's help. Thanks a lot. I have implemented all your indications.

Comment 4 Renich Bon Ciric 2024-05-14 05:34:53 UTC
I have added a sysusers configuration file.


Note You need to log in before you can comment on or make changes to this bug.