Bug 1645765

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- The file protocol/wlr-layer-shell-unstable-v1.xml is licensed with "NTP (legal
  disclaimer)". It's not on the list of good licenses. You should check with
  Fedora Legal whether this is a good license.
- Upstream files are not properly licensed, most files are missing
  license headers. This should at least be reported to upstream.


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[?]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "NTP (legal disclaimer)", "Unknown or
     generated". 12 files have unknown license. Detailed output of
     licensecheck in /home/thofmann/fedora/reviews/review-
     slurp/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: slurp-0.0.1-1.20181024git0dbd039.fc30.x86_64.rpm
          slurp-debuginfo-0.0.1-1.20181024git0dbd039.fc30.x86_64.rpm
          slurp-debugsource-0.0.1-1.20181024git0dbd039.fc30.x86_64.rpm
          slurp-0.0.1-1.20181024git0dbd039.fc30.src.rpm
4 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (debuginfo)
-------------------
Checking: slurp-debuginfo-0.0.1-1.20181024git0dbd039.fc30.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
slurp.x86_64: W: invalid-url URL: https://github.com/emersion/slurp <urlopen error [Errno -2] Name or service not known>
slurp-debugsource.x86_64: W: invalid-url URL: https://github.com/emersion/slurp <urlopen error [Errno -2] Name or service not known>
slurp-debuginfo.x86_64: W: invalid-url URL: https://github.com/emersion/slurp <urlopen error [Errno -2] Name or service not known>
3 packages and 0 specfiles checked; 0 errors, 3 warnings.



Requires
--------
slurp (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libwayland-client.so.0()(64bit)
    libwayland-cursor.so.0()(64bit)
    rtld(GNU_HASH)

slurp-debugsource (rpmlib, GLIBC filtered):

slurp-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
slurp:
    slurp
    slurp(x86-64)

slurp-debugsource:
    slurp-debugsource
    slurp-debugsource(x86-64)

slurp-debuginfo:
    debuginfo(build-id)
    slurp-debuginfo
    slurp-debuginfo(x86-64)



Source checksums
----------------
https://github.com/emersion/slurp/archive/0dbd03991462397eb92bb40af712c837c898ebf1.tar.gz#/slurp-0.0.1-20181024git0dbd039.tar.gz :
  CHECKSUM(SHA256) this package     : cf2cdc909ec9a479f6b8a895ce9cbcc587f2795f93be9b9fb8be8785fbecc47e
  CHECKSUM(SHA256) upstream package : cf2cdc909ec9a479f6b8a895ce9cbcc587f2795f93be9b9fb8be8785fbecc47e


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -u https://bugzilla.redhat.com/show_bug.cgi?id=1645765 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Spec URL: https://fedorapeople.org/~jpokorny/pkgs/slurp/slurp.spec
SRPM URL: https://fedorapeople.org/~jpokorny/pkgs/slurp/slurp-0.0.1-2.20181024git0dbd039.fc30.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=30774201

- Clarify non-MIT license notice
- Add -grim subpackage carrying smooth slurp-grim integration

Sorry for my blindness to "issues" section, was concentrating merely
on the checkboxes since my brain was learnt that there's usually a lot
of ballast coming from fedora-review (meaning it likely needs some
love to catch up the evolution):

> - The file protocol/wlr-layer-shell-unstable-v1.xml is licensed with
>   "NTP (legal disclaimer)". It's not on the list of good licenses. You
>   should check with Fedora Legal whether this is a good license.

Indeed it is a good license, and that's just unfortunate than
licensecheck provides a misleading verdict vs. how Fedora classifies
the license -- this is the matching text:

https://fedoraproject.org/wiki/Licensing:MIT#Old_Style_with_legal_disclaimer

but it contains "and sell" addition to the very first sentences, but
this matches with, e.g.,

https://fedoraproject.org/wiki/Licensing:MIT#Old_Style

(see also [bug 1475962 comment 12]).

Do you see any conflict here?  I shall fix the wording in the spec to
this very interpretation since otherwise I am carrying this cargo cult
confusion (originating in wlroots since [bug 1529352 comment 5]),
though.

And you should likely do the same with sway, since the protocol file
in question was borrowed there :-)

> - Upstream files are not properly licensed, most files are missing
>   license headers. This should at least be reported to upstream.

Cannot find anyting to that effect in
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines
Can you point out the particular part to me that asks for this, please?

(Again, sway is in the same boat here.)

My TODO is to to get the slurp-grim.desktop more into shape
(especially Categories).

(In reply to Jan Pokorný from comment #3)
> Sorry for my blindness to "issues" section, was concentrating merely
> on the checkboxes since my brain was learnt that there's usually a lot
> of ballast coming from fedora-review (meaning it likely needs some
> love to catch up the evolution):

Funny, I always do it the other way around: focus on issues, post the rest for completeness' sake.

> 
> > - The file protocol/wlr-layer-shell-unstable-v1.xml is licensed with
> >   "NTP (legal disclaimer)". It's not on the list of good licenses. You
> >   should check with Fedora Legal whether this is a good license.
> 
> Indeed it is a good license, and that's just unfortunate than
> licensecheck provides a misleading verdict vs. how Fedora classifies
> the license -- this is the matching text:
> 
> https://fedoraproject.org/wiki/Licensing:MIT#Old_Style_with_legal_disclaimer
> 
> but it contains "and sell" addition to the very first sentences, but
> this matches with, e.g.,
> 
> https://fedoraproject.org/wiki/Licensing:MIT#Old_Style
> 
> (see also [bug 1475962 comment 12]).
> 
> Do you see any conflict here?  I shall fix the wording in the spec to
> this very interpretation since otherwise I am carrying this cargo cult
> confusion (originating in wlroots since [bug 1529352 comment 5]),
> though.

OK, that makes sense. No conflict here, it just wasn't clear to me.
> 
> And you should likely do the same with sway, since the protocol file
> in question was borrowed there :-)

Yes!

> 
> > - Upstream files are not properly licensed, most files are missing
> >   license headers. This should at least be reported to upstream.
> 
> Cannot find anyting to that effect in
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
> LicensingGuidelines
> Can you point out the particular part to me that asks for this, please?

It's not explicitly stated in the guidelines, but if the code is not properly licensed, then it cannot be shipped in Fedora. I think it's clear enough what upstream intends, but legally it's not 100% clear. Afaik, just putting a LICENSE file in your repository doesn't license all the files with that license. Since the upstream intention is clear, this is not a blocker, but it should definitely be pointed out to them so they can fix it, which should be in their interest.
> 
> (Again, sway is in the same boat here.)

Good point, I never checked that when I started maintaining sway (I did not create the initial package).

> 
> My TODO is to to get the slurp-grim.desktop more into shape
> (especially Categories).

Can we get the package wrapped up?

I reconsidered my comment about the licensing, apparently this is handled differently for MIT-licensed projects, MIT doesn't have the requirement that the license is mentioned in every source file. So this is not a blocker.

If you can update the package to the latest release, I can continue (or rather re-do) the review.

Jan, are you still interested in this package?

*** This bug has been marked as a duplicate of bug 1789980 ***