Bug 1939875

Initial comments:

1. To ensure correct ordering in case of updating from snapshot to newer snapshot, version string should include date. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ section Snapshots
2. Patches should link to patch/issue/pull request/something where the patch is upstreamed, or explain why the patch is Fedora specific. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatus/

I will continue later.

gumbo-parser is already packaged; you should probably unbundle it, but at the very least not clobber the existing files.

A couple of findings more:

3. When I try to build from srpm in Koji, I get the following error:
   
     /bin/sh: line 1: xxd: command not found

  Build command was this:

     $ koji build --scratch f35 litehtml-0.5-1.gitdb7f59d.fc35.src.rpm

  and logs are at https://koji.fedoraproject.org/koji/taskinfo?taskID=63994712

  Is BuildRequires: vim-common missing?

     $ dnf repoquery --whatprovides /usr/bin/xxd
     vim-common-2:8.2.1770-1.fc33.x86_64
     vim-common-2:8.2.2607-1.fc33.x86_64

  (Strange by the way, I do not understand how vim is involved with this.)

4. I agree with Elliott about (un)bundling gumbo. Upstream should be contacted about this, too. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

5. I see some tests available upstream, but there is no %check in the spec file. The tests should be run if possible. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites, also discussion of %ctest macro in https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/ may be relevant

6. Gumbo Parser is licensed with Apache Software License 2.0. If unbundling it (item 4) cannot be done, the License must be marked as 'BSD and ASL2' and full license text included. Note that upstream is currently violating Gumbo's license by not including the full license text as required by its terms — they should be informed about this situation, too. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text

Thanks for your comments!

Spec URL: https://smani.fedorapeople.org/review/litehtml.spec
SRPM URL: https://smani.fedorapeople.org/review/litehtml-0.5-1.20210317gitb6442d9.fc35.src.rpm

%changelog
* Fri Mar 19 2021 Sandro Mani <manisandro> - 0.5.1.gitb6442d9
- Update to git b6442d9
- Drop upstreamed patches
- Unbundle downstream
- Enable tests

Good progress!

After the latest updates, fedora-review ran to completion, so review can progress. 

7. The source tree comes bundled with tools/xxd.exe. This Windows binary is of course useless, and brings more licensing issues, since the Vim license [1] also requires including a copy. Could also that be removed from in %prep?

I have to admit being a little lost regarding the license violations: Bad items can be removed in %prep, but they still end up in the srpm. Is that a problem? I sent a question to the packaging mailing list about this [2])

8. Changelog formatting is slightly off: Character '-' should be used between version and release parts. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs (also, I do not think it is necessary to keep a changelog for changes made during review — but not wrong either, if you prefer to have those)

9. fedora-review spotted discrepancy between given spec and srpm:

    Generic:
    [!]: Spec file according to URL is the same as in SRPM.
         Note: Spec file as given by url is not the same as in SRPM (see
         attached diff).
         See: (this test has no URL)

    Diff spec file in url and in SRPM
    ---------------------------------
    --- /home/otto/src/Jakelut/Fedora/Katselmointi/litehtml/1939875-litehtml/srpm/litehtml.spec 2021-03-21 08:58:47.234730891 +0200
    +++ /home/otto/src/Jakelut/Fedora/Katselmointi/litehtml/1939875-litehtml/srpm-unpacked/litehtml.spec 2021-03-19 12:43:35.000000000 +0200
    @@ -76,5 +76,5 @@
 
     %changelog
    -* Fri Mar 19 2021 Sandro Mani <manisandro> - 0.5.1.20210317gitb6442d9
    +* Fri Mar 19 2021 Sandro Mani <manisandro> - 0.5.1.gitb6442d9
     - Update to git b6442d9
     - Drop upstreamed patches

[1]: https://github.com/vim/vim/blob/master/LICENSE
[2]: https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/HPVIZZW2U6LJUGW6UKQOKPDLHBVYOQJY/

I have completed the review. Review report below. It includes all remaining issues, also those that have been discussed in earlier comments.

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

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

===== List of MUST issues =====

[!]: Changelog in prescribed format.
     Note: Should use '-' between version and release
[!]: Sources contain only permissible code or content.
     Note: gumbo license text missing, xxd.exe license is unclear. See packaging
     mailing list discussion for solution options:
     https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/HPVIZZW2U6LJUGW6UKQOKPDLHBVYOQJY/


===== List of SHOULD issues =====

[!]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
     Note: gumbo and xxd.exe licenses not included
[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
     Note: litehtml_unbundle-gumbo.patch has not been sent upstream. It would be good to
     have an option in upstream buildsystem to prevent building bundled
     dependencies and rely on that. The link should point to that pull request/
     issue/discussion.


===== List of EXTRA issues =====

[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see
     attached diff).


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[!]: Changelog in prescribed format.
     Note: Should use '-' between version and release
[!]: Sources contain only permissible code or content.
     Note: gumbo license text missing, xxd.exe license is unclear
[-]: Package contains desktop file if it is a GUI application.
[x]: 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: There are rpmlint messages (see attachment).
[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 does not own files or directories owned by other packages.
[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 must not depend on deprecated() packages.
[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.
     Note: gumbo and xxd.exe licenses not included
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
     Note: litehtml_unbunde-gumbo.patch is not upstream. It would be good to
     have an option in upstream buildsystem to prevent building bundled
     dependencies and rely on that. The link should point to that pull request/
     issue/discussion.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[?]: 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]: Fully versioned dependency in subpackages if applicable.
[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:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see
     attached diff).
     See: (this test has no URL)
[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.


Rpmlint
-------
Checking: litehtml-0.5-1.20210317gitb6442d9.fc35.x86_64.rpm
          litehtml-devel-0.5-1.20210317gitb6442d9.fc35.x86_64.rpm
          litehtml-debuginfo-0.5-1.20210317gitb6442d9.fc35.x86_64.rpm
          litehtml-debugsource-0.5-1.20210317gitb6442d9.fc35.x86_64.rpm
          litehtml-0.5-1.20210317gitb6442d9.fc35.src.rpm
litehtml.x86_64: W: incoherent-version-in-changelog 0.5.1.gitb6442d9 ['0.5-1.20210317gitb6442d9.fc35', '0.5-1.20210317gitb6442d9']
litehtml-devel.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (debuginfo)
-------------------
Checking: litehtml-debuginfo-0.5-1.20210317gitb6442d9.fc35.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
litehtml-devel.x86_64: W: no-documentation
litehtml.x86_64: W: incoherent-version-in-changelog 0.5.1.gitb6442d9 ['0.5-1.20210317gitb6442d9.fc35', '0.5-1.20210317gitb6442d9']
4 packages and 0 specfiles checked; 0 errors, 2 warnings.



Diff spec file in url and in SRPM
---------------------------------
--- /home/otto/src/Jakelut/Fedora/Katselmointi/litehtml/1939875-litehtml/srpm/litehtml.spec 2021-03-21 08:58:47.234730891 +0200
+++ /home/otto/src/Jakelut/Fedora/Katselmointi/litehtml/1939875-litehtml/srpm-unpacked/litehtml.spec    2021-03-19 12:43:35.000000000 +0200
@@ -76,5 +76,5 @@
 
 %changelog
-* Fri Mar 19 2021 Sandro Mani <manisandro> - 0.5.1.20210317gitb6442d9
+* Fri Mar 19 2021 Sandro Mani <manisandro> - 0.5.1.gitb6442d9
 - Update to git b6442d9
 - Drop upstreamed patches


Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review --bug 1939875
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: SugarActivity, Haskell, Perl, Java, PHP, fonts, R, Ocaml, Python
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Spec URL: https://smani.fedorapeople.org/review/litehtml.spec
SRPM URL: https://smani.fedorapeople.org/review/litehtml-0.5-1.20210317gitb6442d9.fc35.src.rpm

%changelog
* Tue Mar 23 2021 Sandro Mani <manisandro> - 0.5-2.20210317gitb6442d9
- Delete bundled xxd.exe in prep
- Fix changelog formatting


I've submitted a PR for the unbundling patch at https://github.com/litehtml/litehtml/pull/149

Great progress!

The only remaining thing is source rpm licensing. I think it would be the best if upstream could be convinced to do the following:

1. Add copy of the Gumbo license in the repository. README.md already mentions Gumbo's licensing and all source files start by declaring the Apache Software License 2.0. But, the license conditions require attaching a copy of the license, not just linking to it.
2. Clarify the situation of xxd.exe licensing (most probably it is the Vim license, but it is the best let upstream say what it is, because they should know where they got it from) and add its license text.

If that is not possible, then the source rpm must be modified. See the mailing list discussion linked above for option. (Hopefully upstream agrees to clarify its licensing and we do not have to do this.)

One more tiny thing. I suppose this comment is not relevant any more, since upstream accepted the so versioning patch? At least wording could be updated, since the named patch does not exist any more.

> # WARNING: Update soversion in litehtml_soversion.patch if necessary!

Spec URL: https://smani.fedorapeople.org/review/litehtml.spec
SRPM URL: https://smani.fedorapeople.org/review/litehtml-0.5-3.20210323gitb4c815c.fc35.src.rpm

%changelog
* Tue Mar 23 2021 Sandro Mani <manisandro> - 0.5-3.20210323gitb4c815c
- Update to git b4c815c
- Drop upstreamed patches

Upstream has accepted my PR to add license texts for gumbo and xxd, and also to drop the precompiled xxd binary, bundling it as source code instead.

Nice! Great co-operation with upstream. Package approved! Let me know if you need any help with anything. Otherwise, thank you and best wishes for the rest of the process!

Thanks a lot for the review!

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/litehtml