[Bug 1519785] Review Request: notepadqq - An advanced text editor for developers

https://bugzilla.redhat.com/show_bug.cgi?id=1519785

Ben Rosser <rosser.bjr(a)gmail.com&gt; changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?



--- Comment #11 from Ben Rosser <rosser.bjr(a)gmail.com&gt; ---
I finally got around to doing a full review. Now that the bundling issues have
been sorted out, I think the package mostly looks good!

Issues
------

- 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.
  Note: License file COPYING is not marked as %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

$ rpmls results/notepadqq-1.3.6-1.fc29.x86_64.rpm  | grep COPYING
-rw-r--r--  /usr/share/doc/notepadqq/COPYING

You should not install COPYING into /usr/share/doc; avoid copying it here in
the first place. Instead, as it says on the linked wiki page, you should just
do this:

%license COPYING

This will automatically install the license file into /usr/share/licenses.

- If the package is under multiple licenses, the licensing breakdown must be
documented in the spec.

When writing the License tag it's good to add a comment explicitly detailing
the license breakdown-- sorry, I should have mentioned this before. (e.g. which
part is under GPL and which part is under MIT). It doesn't need to be super
detailed, just something like "This bundled library is under this license, this
one is under that license", etc.


- Package must own all directories that it creates.
  Note: Directories without known owners: /usr/libexec/notepadqq

This is because of the way you list this in the file list:

%{_libexecdir}/%{name}/%{name}-bin

The directory %{_libexecdir}/%{name} will not be marked as owned by the
package. Instead, you can just do this, and contents of %{_libexecdir}/%{name}
will still be included:

%{_libexecdir}/%{name}

- Latest version is packaged.

There's been a new release within the past month, but this is my fault for not
getting to the review sooner. :(

I think these are the only showstoppers. Unfortunately, the package currently
does not install on Rawhide...

Error: 
 Problem: package notepadqq-1.3.6-1.fc29.x86_64 requires nodejs-archiver, but
none of the providers can be installed
  - package nodejs-archiver-1.0.1-4.fc29.noarch requires (npm(archiver-utils)
> = 1.0.0 with npm(archiver-utils) < 2), but none of the providers
> can be 
installed
  - conflicting requests
  - nothing provides (npm(normalize-path) >= 2.0.0 with npm(normalize-path) <
3) needed by nodejs-archiver-utils-1.3.0-5.fc29.noarch

This is nothing to do with notepadqq though, this is a nodejs packaging problem
(ergh). I've filed a ticket against nodejs-archiver on your behalf:
https://bugzilla.redhat.com/show_bug.cgi?id=1581012

I did also test that the package builds on Fedora 28, and it looks fine. So
please just fix the three issues above, and I'll be happy to approve it and
sponsor you.

Again, sorry this took so long, I haven't had as much time for Fedora lately as
I was hoping.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component