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

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


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

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]: 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 requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[-]: 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 %doc.
[x]: 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)", "Unknown or generated". 3 files have unknown
     license. Detailed output of licensecheck in /home/zbyszek/fedora/1014607
     -python-jsmin/licensecheck.txt
[x]: Package consistently uses macro is (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.
[!]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[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
[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).

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

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

Generic:
[x]: 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]: 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.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

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

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


Rpmlint
-------
Checking: python-jsmin-2.0.4-1.fc19.noarch.rpm
python-jsmin.noarch: W: spelling-error Summary(en_US) minifier -> magnifier
python-jsmin.noarch: W: spelling-error %description -l en_US minifier -> magnifier
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

bogus, as usual.

So, there are three issues:
1. /usr/lib/python2.7/site-packages/jsmin/ is unowned (just remove '/*').

2. %check is missing. The package has tests, I think they should be relatively easy to incorporate in the build:

% python /usr/lib/python2.7/site-packages/jsmin/test.py
.........................
----------------------------------------------------------------------
Ran 25 tests in 0.005s

OK

3. The package supports python 3, so I don't see a reason why not add a python 3 subpackage.

This is a derivative of Douglas Crockford's jsmin and likely not free software.  Blocking FE-LEGAL and advised on the mailing list that the package submitter ask for guidance from the Fedora Legal mailing list.

Note: python3 subpackages should only be added if upstream supports the package on python3.  Otherwise a separate package should be made.  See the warning here: https://fedoraproject.org/wiki/Packaging:Python#Subpackages

Support can be taken somewhat loosely: Does the upstream accept and merge patches that fix issues with running the code on python3.  However, the less work that the upstream does, the more work that the packager may need to perform so bear that in mind.

(In reply to Toshio Ernie Kuratomi from comment #3)
> Note: python3 subpackages should only be added if upstream supports the
> package on python3.
Yes, the source seems to be 2/3 compatible. Since this is a really a one file package, making a separate source package seems like overkill.

> This is a derivative of Douglas Crockford's jsmin and likely not free 
> software.  Blocking FE-LEGAL and advised on the mailing list that the package 
> submitter ask for guidance from the Fedora Legal mailing list.
Thank you for catching this. I wasn't diligent enough in the review.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #4)
> (In reply to Toshio Ernie Kuratomi from comment #3)
> > Note: python3 subpackages should only be added if upstream supports the
> > package on python3.
> Yes, the source seems to be 2/3 compatible. Since this is a really a one
> file package, making a separate source package seems like overkill.
> 
The question is really about upstream support.  It could be that the package will stop functioning on python-3.4,5,6,7, etc.  If the jsmin upstream doesn't (at least) take patches to fix python3 problems then you'll be stuck generating fixes for the python3 problems yourself without upstream help.  You'll have to carry patches in the Fedora package to fix the issues.  At that point, you've really forked the software and are shipping something different from wat upstream provides.

It's better to have this out in the open -- find out what upstream intends and if they aren't taking python3 patches, make it clear that there's two separate streams now rather than later.

(In reply to Toshio Ernie Kuratomi from comment #5)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #4)
> > (In reply to Toshio Ernie Kuratomi from comment #3)
> > > Note: python3 subpackages should only be added if upstream supports the
> > > package on python3.
> > Yes, the source seems to be 2/3 compatible. Since this is a really a one
> > file package, making a separate source package seems like overkill.
> > 
> The question is really about upstream support.  It could be that the package
> will stop functioning on python-3.4,5,6,7, etc.
Experience seems to suggest otherwise: many packages are slow to get python 3 support, but once it happens, they usually don't drop it. Looking at the sources, it seems more likely that python 2 support will dropped, since it is provided as compatibility imports.

> If the jsmin upstream
> doesn't (at least) take patches to fix python3 problems then you'll be stuck
> generating fixes for the python3 problems yourself without upstream help. 
> You'll have to carry patches in the Fedora package to fix the issues.  At
> that point, you've really forked the software and are shipping something
> different from wat upstream provides.
I'm pretty sure that the work required to go through a second review for a separate package trumps all work to support python 3 compatibility.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> Experience seems to suggest otherwise: many packages are slow to get python
> 3 support, but once it happens, they usually don't drop it. Looking at the
> sources, it seems more likely that python 2 support will dropped, since it
> is provided as compatibility imports.
> 
I'm not sure I understand you but if I do then you are making incorrect assumptions.  There are many packages which have added python3 support but when upstream makes updates the python2 versions are tested while the python3 versions are buggy and have to be patched by package maintainers and those packages sent upstream.

> > If the jsmin upstream
> > doesn't (at least) take patches to fix python3 problems then you'll be stuck
> > generating fixes for the python3 problems yourself without upstream help. 
> > You'll have to carry patches in the Fedora package to fix the issues.  At
> > that point, you've really forked the software and are shipping something
> > different from wat upstream provides.
> I'm pretty sure that the work required to go through a second review for a
> separate package trumps all work to support python 3 compatibility.

then you would be wrong.  Just take a look at some of our essential and problematic packages like python-docutils and python-nosetests where the python3 compatibility has held back updates for a week or more at a time both when they update and when Fedora updates the version of the python3 interpreter.  The one-time pain of a second package and review is trivial compared to the ongoing maintenance of a package.

Anyhow, the Packaging Guidelines specify that unless upstream supports building the python3 package not to build it as a subpackage of the python2 package.  So someone should just find out if upstream supports this.  If they do, then feel free to build it as a subpackage.  If not, then do not.

(In reply to Toshio Ernie Kuratomi from comment #7)
[snip]
> Anyhow, the Packaging Guidelines specify that unless upstream supports
> building the python3 package not to build it as a subpackage of the python2
> package.  So someone should just find out if upstream supports this.  If
> they do, then feel free to build it as a subpackage.  If not, then do not.
Right. So a) the package is tagged as python :: 3 in pypi, b) sources have explicit python 2/3 compatibility. I think it is enough to understand that python 3 is supported.

(In reply to Martin Krizek from comment #0)

> Description:
> JavaScript minifier. It can be used for compressing JavaScript files in e.g.
> Flask framework.

Hi, I think, you have a short description [1] should add "It can be used for compressing JavaScript files in e.g. Flask framework."


[1]
%description
JavaScript minifier.

Sadly, the fact that this is based on Douglas Crockford's non-free jsmin makes this one non-free as well. Feel free to reach out to Mr. Crockford if you are feeling masochistic, perhaps his stance on his silly joke license clause has changed with time.

It's quite unfortunate, but the licensing situation is unlikely to change.

I'll close this in a few days if nobody objects.