Comment 1 Robert Clausecker 2023-02-24 23:39:06 UTC

Review:

 - I see that you bundle libSkiaSharp.so as a binary file (o.O)  Could you check
   if this file can be unbundled or built from source?
 - LICENSE_COMB=dual means that either of the licenses can be used.  Is this 
   correct?  If multiple licenses apply at the same time, please use
   LICENSE_COMB=multi
 - consider replacing amd64 with ${ARCH} in distfiles to ensure that the port
   break in an obvious place when compilation on another architecture is tried
 - you seem to replace jellyfin with ${PORTNAME} in lots of places.  I recommend
   only using ${PORTNAME} in places where changing the PORTNAME would not break
   the build.  Just because it's the string jellyfin doesn't mean that it is
   ${PORTNAME}.
 - why is the one file in PLIST_FILES instead of in pkg-plist?

Created attachment 240452 [details]
0001-multimedia-jellyfin-Add-port.patch

fixed issues noted in review

Comment 3 Robert Clausecker 2023-02-27 16:48:05 UTC

You forgot the amd64 in EXTRACT_ONLY.  Could you enlighted me on the libSkiaSharp.so part of the build?  Could that one (or any part of the thing really) be replaced with source code that is built in the ports tree instead of downloading binaries?

Created attachment 240464 [details]
0001-multimedia-jellyfin-Add-port.patch

Replaced last `amd64` with `${ARCH}` as pointed out in review

Comment 5 Robert Clausecker 2023-02-27 20:41:00 UTC

It would be nice if you could answer the questions I have asked.
These are currently stopping me from taking the patch further.

Ports should usually be built from source when you type "make."  Downloading an already built binary is very unusual and requires justification.

I totally understand what you are saying, and yes, source building would be the nicest option.
But:
the problem with skia is that it is using the google tools, and it only builds if we disable almost all of it
so it's a pretty hacky thing, and it needs a lot of plumbing
The file was not packaged into the jellyfin .tar.gz, because we thought it would be cleaner to ship it seperately because skia has another licence than jellyfin
if it was packaged in the dotnet compliled binary that is shipped in the .tar.gz, this whole discussion was probably nonexistent as it would be just another binary file in the binary package.

We already have some other .NET projects in the ports tree that have NO_BUILD and just unpack a binary release. See net-p2p/radarr, net-p2p/prowlarr and multimedia/emby for examples.

One day, when .NET is available on freebsd, we should look into replacing all of those and source build them.

Comment 7 Robert Clausecker 2023-02-28 09:24:28 UTC

Thank you for documenting this reason.

Comment 8 commit-hook 2023-03-03 21:31:58 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=92747e09c4c666afa7c5554ccde9299f7c14b3fe

commit 92747e09c4c666afa7c5554ccde9299f7c14b3fe
Author:     Michiel van Baak Jansen <michiel@vanbaak.eu>
AuthorDate: 2023-02-21 21:17:02 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2023-03-03 21:31:07 +0000

    multimedia/jellyfin: Add port

    Jellyfin is the volunteer-built media solution that
    puts you in control of your media.
    Stream to any device from your own server, with no strings attached.

    Your media, your server, your way.

    WWW: https://jellyfin.org/

    Maintainer becomes submitter.  Already maintainer of other ports.

    Port installs binaries downloaded from a website without any
    local compilation.  The submitter has done the same for numerous
    previous ports due to difficulties in locally building .NET based
    ports.  See PR for some discussion.

    PR:             269754
    Approved by:    flo (mentor)
    Differential Revision: https://reviews.freebsd.org/D38861

 GIDs                                              |    2 +-
 UIDs                                              |    2 +-
 multimedia/Makefile                               |    1 +
 multimedia/jellyfin/Makefile (new)                |   56 +
 multimedia/jellyfin/distinfo (new)                |    5 +
 multimedia/jellyfin/files/jellyfin.in (new +x)    |   72 +
 multimedia/jellyfin/files/pkg-message.in (new +x) |   53 +
 multimedia/jellyfin/pkg-descr (new +x)            |    5 +
 multimedia/jellyfin/pkg-plist (new +x)            | 1998 +++++++++++++++++++++
 9 files changed, 2192 insertions(+), 2 deletions(-)

Comment 9 Robert Clausecker 2023-03-03 21:33:08 UTC

Thank you for your contribution.

(In reply to Robert Clausecker from comment #1)

> I see that you bundle libSkiaSharp.so as a binary file (o.O)

> Could you check if this file can be unbundled or built from source?

This is dangerous. This should probably be documented in both the package description and the post-install message.