Bug 1936772 - Review Request: sixel - Encoder and decoder for DEC Sixel
Summary: Review Request: sixel - Encoder and decoder for DEC Sixel
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Aleksei Bavshin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 2227397
TreeView+ depends on / blocked
 
Reported: 2021-03-09 06:14 UTC by Nick Black
Modified: 2023-09-18 11:03 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2022-03-19 08:48:03 UTC
Type: ---
Embargoed:
alebastr89: fedora-review?


Attachments (Terms of Use)

Description Nick Black 2021-03-09 06:14:44 UTC
Spec URL: http://dank.qemfd.net/sixel.spec
SRPM URL: http://dank.qemfd.net/sixel-1.8.6-1.fc35.src.rpm
Description: Encoder and decoder for DEC Sixel terminal pixel graphics
Fedora Account System Username: nickblack

Comment 1 Aleksei Bavshin 2021-03-13 04:05:14 UTC
> Name: sixel

Upstream name is libsixel and all other distributions are unanimously using it[1].
Let's avoid renaming the package without a good reason.

> Source0: https://github.com/saitoha/lib%{name}/releases/download/v%{version}/libsixel-%{version}.tar.gz

You can shorten Source to %{url}/releases/download/v%{version}/%{name}-%{version}.tar.gz.
But the release archive lacks license files[2] so you'd really want to use %{url}/archive/v%{version}/%{name}-%{version}.tar.gz

> BuildRequires: git

Unnecessary. Noting in the build process requires git.

> BuildRequires: gcc
> BuildRequires: pkgconfig(libjpeg)
> BuildRequires: pkgconfig(libpng)

Missing `BuildRequires: make` [3].

There's an optional dependency on libcurl, but given that there are at least 2 known CVEs[4][5] in the file loaders it's better to keep network support disabled.

> %package devel
> Summary: Development files for %{name}
> Provides: %{name}%{?_isa} = %{?epoch:%{epoch}:}%{version}-%{release}

Uh... what? You are trying to tell that the main package with libraries is not necessary if -devel is installed.
I believe you meant `Requires: %{name}%{?_isa} = %{version}-%{release}`. Rpmlint agrees with me: sixel-devel.x86_64: W: no-dependency-on sixel/sixel-libs/libsixel

> %package utils
> Summary:       Binaries from the libsixel project

How about `SIXEL decoder and encoder utilities`? 

> License:       MIT

You don't need to repeat the license for subpackage if it doesn't differ from the main one.

> %description utils
> Binaries from libsixel.

sixel-utils.x86_64: W: description-shorter-than-summary
Let's make that at least `%{summary}.`

> make %{?_smp_mflags}

%make_build

> make install DESTDIR=$RPM_BUILD_ROOT

%make_install

Source archive contains unit tests. Consider running them at build time with
%check
%make_build test

> %files
> %doc ChangeLog NEWS

Please, add license files. For example:
%license LICENSE LICENSE.{pnmcolormap,sdump,sixel,stb}

Some of those are MIT and it's important to distribute these along with the binaries.

> %{_mandir}/man5/*.5*

The man file could be more suitable for devel or utils subpackage. It's just a generic description of a SIXEL format.

> %files utils
> # we don't want libsixel-config

We really want it in -devel package. Some applications may call libsixel-config instead of using pkg-config at build time.

> %{_datadir}/bash-completion/*
> %{_datadir}/zsh/*

Please, be more explicit. Also, you need to own the zsh completion directories (bash-completion is already owned by filesystem):

%{_datadir}/bash-completion/completions/*
%dir %{_datadir}/zsh
%dir %{_datadir}/zsh/site-functions
%{_datadir}/zsh/site-functions/_*

---
[1] https://repology.org/project/libsixel/versions
[2] https://github.com/saitoha/libsixel/pull/129
[3] https://fedoraproject.org/wiki/Changes/Remove_make_from_BuildRoot
[4] https://github.com/saitoha/libsixel/issues/134 (CVE-2020-11721)
[5] https://github.com/saitoha/libsixel/issues/136 (CVE-2020-19668)

Comment 2 Aleksei Bavshin 2021-03-13 04:26:26 UTC
Adding FE-Legal blocker.

https://github.com/saitoha/libsixel/blob/master/LICENSE.sixel looks benign and the source files in question mention that the code was relicensed under MIT[1][2]. I can't read the original license text though and not qualified to make decisions on the custom licenses, however permissive these are.

Several sources are also originally distributed under public domain. In my (limited) understanding that means it's fine to distribute the result under MIT and keep the License tag as simple `MIT`.

Ben, can you please look into that and confirm that it's safe to consider the combined work licensed as MIT? I hope we have someone who can read Japanese flavor of legalese :)

[1] https://github.com/saitoha/libsixel/blob/master/src/fromsixel.c
[2] https://github.com/saitoha/libsixel/blob/master/src/tosixel.c

Comment 3 Ben Cotton 2021-03-15 13:23:03 UTC
Ack. I'll look into it.

Comment 4 Ben Cotton 2021-03-18 18:45:35 UTC
The original license meets Fedora's standards and does not prohibit re-licensing. You may consider the combined work as MIT-licensed.

Removing the FE-LEGAL block.

Comment 5 Nick Black 2021-03-18 19:19:23 UTC
Thanks Ben, and thanks for the review @alebastr89. I'll get on your feedback before the end of the week.

Comment 6 Package Review 2022-03-19 00:45:22 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Comment 7 Nick Black 2022-03-19 08:48:03 UTC
I don't see these problems as easily surmounted, and I'm no longer involved with libsixel. Closing!


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