Bug 910793

This is my first package, and I am seeking a sponsor.

I'm not a sponsor yet, but I'd be happy to do an informal review as a packager to make it easier on your sponsor.  (You may also want to do a couple of informal reviews of other packages needing reviews, to show your potential sponsor that you understand the packaging guidelines.)

In short, there are a couple of minor issues with the licensing that should be addressed, but other than that, the package looks good.

[ O K ] MUST: rpmlint must be run on every package. The output should be posted
in the review.

$ rpmlint rpmlint spice-html5.spec spice-html5-0.1.0-1.fc18.src.rpm spice-html5-0.1.0-1.fc18.noarch.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

[ O K ] MUST: The package must be named according to the Package Naming
Guidelines.

[ O K ] MUST: The spec file name must match the base package %{name}, in the
format %{name}.spec unless your package has an exemption.

[ O K ] MUST: The package must meet the Packaging Guidelines.

[ O K ] MUST: The package must be licensed with a Fedora approved license and
meet the Licensing Guidelines.

[ BAD ] MUST: The License field in the package spec file must match the actual
license. 
 - The spec file says that the package is GPLv3, but at least some of the code
   appears to be LGPLv3

[ BAD ] MUST: 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 must be included in %doc.
 - The COPYING.LESSER file is not included in %doc

[ O K ] MUST: The spec file must be written in American English. 

[ O K ] MUST: The spec file for the package MUST be legible. 

[ O K ] MUST: The sources used to build the package must match the upstream
source, as provided in the spec URL. Reviewers should use md5sum for this task.
If no upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.

$ md5sum spice-html5-0.1.0.tar.gz ; curl -s -o - http://www.spice-space.org/download/spice-html5/spice-html5-0.1.0.tar.gz | md5sum -
ef7daf5aa50bc1748af002522718ce81  spice-html5-0.1.0.tar.gz
ef7daf5aa50bc1748af002522718ce81  -

[ N/A ] MUST: The package MUST successfully compile and build into binary rpms
on at least one primary architecture. 

[ N/A ] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line. 

[ O K ] MUST: All build dependencies must be listed in BuildRequires, except
for any that are listed in the exceptions section of the Packaging Guidelines ;
inclusion of those as BuildRequires is optional. Apply common sense.

[ N/A ] MUST: The spec file MUST handle locales properly. This is done by using
the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.

[ N/A ] MUST: Every binary RPM package (or subpackage) which stores shared
library files (not just symlinks) in any of the dynamic linker's default paths,
must call ldconfig in %post and %postun. 

[ O K ] MUST: Packages must NOT bundle copies of system libraries.

[ N/A ] MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker. 

[ O K ] MUST: A package must own all directories that it creates. If it does
not create a directory that it uses, then it should require a package which
does create that directory. 

[ O K ] MUST: A Fedora package must not list a file more than once in the spec
file's %files listings. (Notable exception: license texts in specific
situations)

[ O K ] MUST: Permissions on files must be set properly. Executables should be
set with executable permissions, for example. Every %files section must include
a %defattr(...) line. 

[ O K ] MUST: Each package must consistently use macros. 

[ O K ] MUST: The package must contain code, or permissible content. 

[ N/A ] MUST: Large documentation files must go in a -doc subpackage. (The
definition of large is left up to the packager's best judgement, but is not
restricted to size. Large can refer to either size or quantity). 

[ O K ] MUST: If a package includes something as %doc, it must not affect the
runtime of the application. To summarize: If it is in %doc, the program must
run properly if it is not present. 

[ N/A ] MUST: Header files must be in a -devel package. 

[ N/A ] MUST: Static libraries must be in a -static package. 

[ N/A ] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in
a -devel package. 

[ N/A ] MUST: In the vast majority of cases, devel packages must require the
base package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} 

[ N/A ] MUST: Packages must NOT contain any .la libtool archives, these must be
removed in the spec if they are built.

[ N/A ] MUST: Packages containing GUI applications must include a
%{name}.desktop file, and that file must be properly installed with
desktop-file-install in the %install section. If you feel that your packaged
GUI application does not need a .desktop file, you must put a comment in the
spec file with your explanation. 

[ O K ] MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed
should own the files or directories that other packages may rely upon. This
means, for example, that no package in Fedora should ever share ownership with
any of the files or directories owned by the filesystem or man package. If you
feel that you have a good reason to own a file or directory that another
package owns, then please present that at package review time. 

[ O K ] MUST: All filenames in rpm packages must be valid UTF-8.

[ O K ] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. 

[ O K ] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
[ O K ] SHOULD: The reviewer should test that the package builds in mock.
[ O K ] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[ O K ] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[ N/A ] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
[ N/A ] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[ N/A ] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
[ N/A ] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[ N/A ] SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

Hi Jeremy, I recently read your self introduction on the list, and I also want to give a warmly welcome to fedora, and give my comments ;)

Recently by the arrival of nodejs to Fedora, has revived the need for an official guideline for the treatment of  javascript libraries. See: [1] and [2]

At this time JavaScript intended to be served to a web browser is exempted from the exposed in [3], but this will likely change in the future.(Near future, IMO)

You have a directory called thirdparthy, and this files come from these projects,[4],[5] as well you indicate on the top of these files.

From my perspective I see benefits to fedora,packaging these dependencies also, I would like to know your opinion on this.


[1] http://lists.fedoraproject.org/pipermail/packaging/2013-January/008860.html
[2] http://fedoraproject.org/wiki/Talk:JavaScript_libraries_packaging_guideline_draft
[3] https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
[4] http://www-cs-students.stanford.edu/~tjw/jsbn/
[5] http://pajhome.org.uk/crypt/md5

Best Regards

I also recommend you take a look at this link, in order to get a willing sponsor 

http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Convincing_someone_to_sponsor_you

> You have a directory called thirdparthy, and this files come from these
> projects,[4],[5] as well you indicate on the top of these files.
> 
> From my perspective I see benefits to fedora,packaging these dependencies
> also, I would like to know your opinion on this.

Thank you for the review and the comments.  I had mentally grouped these projects under the 'Copylib' exception for bundling libraries, but I
do see your point.

The jsbn scripts do not provide a formalized release, instead offering single files for download.  There also does not appear to be a source code repository, so it would appear to be difficult to build a clean spec file from the upstream.  The jshash utilities do appear to have a released zip file which could be packaged.

By way of learning, and hopefully proving my value as a packager, I will make an effort to create packages for these.  I would like to reserve the right to return and argue that the upstreams do not work well, and that it's fair to consider it a Copylib.

But I will make a stab at creating a jshash package, and I will email Tom Wu (the jsbn author) to see if he would be willing to make files available that would lend themselves to packaging.

There are some mechanical challenges with linking Javascript libraries, largely around path resolution and Apache configuration.  Is there an example of a package that does this already?

> [4] http://www-cs-students.stanford.edu/~tjw/jsbn/
> [5] http://pajhome.org.uk/crypt/md5
> 
> Best Regards

I will also look at the 'convincing' link and try to gain some karma that way as well.

Cheers,

Jeremy

(In reply to comment #5)
> There are some mechanical challenges with linking Javascript libraries,
> largely around path resolution and Apache configuration.  Is there an
> example of a package that does this already?

I think this could be helpful:
http://pkgs.fedoraproject.org/cgit/moodle.git/tree/moodle.conf
http://pkgs.fedoraproject.org/cgit/moodle.git/tree/moodle.spec

in this package solves the problem with symlinks

(In reply to comment #6)
> (In reply to comment #5)
> > There are some mechanical challenges with linking Javascript libraries,
> > largely around path resolution and Apache configuration.  Is there an
> > example of a package that does this already?
> 
> I think this could be helpful:
> http://pkgs.fedoraproject.org/cgit/moodle.git/tree/moodle.conf
> http://pkgs.fedoraproject.org/cgit/moodle.git/tree/moodle.spec
> 
> in this package solves the problem with symlinks

Hmm.  That is helpful, but then it begs another question:

Should Javascript package XXX be installed into /usr/share/js/XXX/, ala
php, pear et all?  Or /usr/share/javascript?  Or just /usr/share/XXX?

(In reply to comment #7)
> Should Javascript package XXX be installed into /usr/share/js/XXX/, ala
> php, pear et all?  Or /usr/share/javascript?  Or just /usr/share/XXX?

Well, In this draft guideline can be read wich is the best location of these files [1]

Let us refer to this line:

"The JavaScript library itself must reside in a subdirectory of %{_datadir}"

i.e. /usr/share/subdirectory

where subdirectory, is usually the name (% {name}) of the package


http://fedoraproject.org/wiki/JavaScript_libraries_packaging_guideline_draft#Filesystem_Locations

This is a web application.  A bit of a special one in that all the server does is serve static files, but it is the same in every other respect.  Therefore, you should follow the Pacakging Guidelines for Web Applications when packaging this:
https://fedoraproject.org/wiki/Packaging:Guidelines#Web_Applications

Fedora does not require client-side JavaScript to be unbundled:
https://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries

Many web applications in the distribution do not perform such unbundling at this time.  Therefore, you should continue to use the bundled JavaScript files.  Without clear client-side JavaScript packaging guidelines, unbundling currently causes more problems than it solves.

(In reply to comment #9)
> Fedora does not require client-side JavaScript to be unbundled:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Duplication_of_system_libraries
Hi T.C.

I explained this in:  
https://bugzilla.redhat.com/show_bug.cgi?id=910793#c3

But I am among those who believe that to start changing something, one should do so promptly.

However in this case i will not go against the current, and I think Jeremy should have the final decision

Believe me, I very much want to kill the client-side bundled JS exception.  I hope to make it a Feature by F21 or so.

Unfortunately, without clear JavaScript guidelines, packages trying to split them off now will inevitably go about it different ways, and that'll create more work for the people trying to unbundle everything properly later with clear guidelines in place.  Instead of just dealing with one legacy of bundling everything, we'll have to deal with several other legacy unbundling methods.

(In reply to comment #11)
> Believe me, I very much want to kill the client-side bundled JS exception. 
> I hope to make it a Feature by F21 or so.
> 
> Unfortunately, without clear JavaScript guidelines, packages trying to split
> them off now will inevitably go about it different ways, and that'll create
> more work for the people trying to unbundle everything properly later with
> clear guidelines in place.  Instead of just dealing with one legacy of
> bundling everything, we'll have to deal with several other legacy unbundling
> methods.

T.C. Well, don't dwell on this for now. ;)

Let us then over the package

- Jeremy, the package include a configuration file for Apache, however, i don't see a proper Requires for it, 

Why?

I guess that the package would work well without a web server, but installs files in /etc/httpd/conf.d

Let us see the ownership of the directory:
$ rpm -qf /etc/httpd/conf.d/
httpd-2.4.3-12.fc18.x86_64
spice-html5 places files into /etc/httpd/conf.d/
spice-html5 depends on httpd to function normally, and would Require: httpd 

- cleaning of buildroot in %install is not needed
- %defattr is not needed
at least that you have intentions to ship this package to el5, 
see http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5.

But if you ask me, I am among those who think that we should move to el6 directly. This is obviously your decision :)

- Please include COPYING.LESSER on %doc
- Please correct the tag license
- In %Source0, use the appropriate macros
Source0:        http://www.spice-space.org/download/spice-html5/spice-html5-0.1.0.tar.gz
should be
Source0:        http://www.spice-space.org/download/spice-html5/%{name}-%{version}.tar.gz
This is beneficial for the packager because it saves work to upgrade the package

T.C. Sorry,  I meant, I don't dwell on this for now. ;) (my english yet is not very polished)

(In reply to comment #12)
> - Jeremy, the package include a configuration file for Apache, however, i
> don't see a proper Requires for it, 
> 
> Why?
> 
> I guess that the package would work well without a web server, but installs
> files in /etc/httpd/conf.d

always a good idea to move httpd files into a sub package %name-httpd for people don't wanna use httpd

Sorry for the slow reply; I don't have permission to write to the downloads folder on the main spice project server, so I'm dependent on someone else to push files for me.

I'm pushing a package with the correct license, that removes %defattr and cleaning in install, and that uses %name more broadly.  I've also moved the apache.conf file to a the doc/ directory.  To some extent, I prefer that, as I think a system administrator should deliberately open a particular page up for active use, rather than have it occur implicitly.  That also eliminates the conundrum over whether or not to Require httpd.  I'll post those files once they are live.

I've switched to hosting the binaries on fdo, and have a new build here:

Spec URL: http://people.freedesktop.org/~jwhite/spice-html5/spice-html5.spec
SRPM URL: http://people.freedesktop.org/~jwhite/spice-html5/spice-html5-0.1.2-1.fc18.src.rpm

the package looks good, I'll sponsor you, but I'll request something else before, you can a informal review to another packager?

btw, please escape the character % in changelog, there should be no macros in comments or in changelogs.
Example:
Revise the .spec file to use %%{name} (correct)
Revise the .spec file to use %{name}    (incorrect)

Thanks. I'll change the % in the change log and respin.

I did start reviewing another package that interested me - icaro.  But then I decided that would be too much cronyism :-/.  So I'm off looking for another reviewer's package to review.

(In reply to comment #19)
> Thanks. I'll change the % in the change log and respin.
> 
> I did start reviewing another package that interested me - icaro.  But then
> I decided that would be too much cronyism :-/.  So I'm off looking for
> another reviewer's package to review.

If you ask me I do not see any problem ;), anyway let me know when you have finished the informal review

Ping, Jeremy?

My apologies; I performed a pair of naive reviews of other packages, and was hoping to reply once I had done something useful :-/.  I am hoping to find time next week to do that.

I have spun up an 0.1.2-2 of the spice-html5 client, which will test whether or not I can make an utterly trivial change correctly :-/.

Spec URL: http://people.freedesktop.org/~jwhite/spice-html5/spice-html5.spec
SRPM URL: http://people.freedesktop.org/~jwhite/spice-html5/spice-html5-0.1.2-2.fc18.src.rpm

(although fdo apache is hurting atm, so it may be a bit before those are available).

I've checked their informal reviews and IMO I seem sufficient to sponsor, Jeremy, Welcome to the package maintainers group ;) 

I proceed to remove the flag FE-NEEDSPONSOR and do the formal review of the package

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

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


===== 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
[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.
[x]: 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 61440 bytes in 5 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.
[-]: Fully versioned dependency in subpackages, if present.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[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 %doc.
[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: No rpmlint messages.

===== 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).
[?]: 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]: 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: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: spice-html5-0.1.2-2.fc17.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint spice-html5
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
spice-html5 (rpmlib, GLIBC filtered):



Provides
--------
spice-html5:
    spice-html5



MD5-sum check
-------------
http://people.freedesktop.org/~jwhite/spice-html5/spice-html5-0.1.2.tar.gz :
  CHECKSUM(SHA256) this package     : 6ce1e0ebb0834a880c5ed3c7602e9fbdb6f3c6f018530c73a2930416c8f44268
  CHECKSUM(SHA256) upstream package : 6ce1e0ebb0834a880c5ed3c7602e9fbdb6f3c6f018530c73a2930416c8f44268

----------------

PACKAGE APPROVED

----------------

Jeremy, you can now proceed to request a repository:
https://fedoraproject.org/wiki/Package_SCM_admin_requests

New Package SCM Request
=======================
Package Name: spice-html5
Short Description: Pure Javascript SPICE client
Owners: jwhite echevemaster
Branches: f18 el6
InitialCC:

Git done (by process-git-requests).

spice-html5-0.1.2-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/spice-html5-0.1.2-2.fc18

spice-html5-0.1.2-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/spice-html5-0.1.2-2.el6

spice-html5-0.1.2-2.el6 has been pushed to the Fedora EPEL 6 testing repository.

spice-html5-0.1.2-2.fc18 has been pushed to the Fedora 18 stable repository.

spice-html5-0.1.4-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/spice-html5-0.1.4-1.el6

Package spice-html5-0.1.4-1.el6:
* should fix your issue,
* was pushed to the Fedora EPEL 6 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=epel-testing spice-html5-0.1.4-1.el6'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-EPEL-2013-11945/spice-html5-0.1.4-1.el6
then log in and leave karma (feedback).

spice-html5-0.1.4-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

New Package SCM Request
=======================
Package Name: spice-html5
Short Description: Pure Javascript SPICE client
Owners: jwhite echevemaster
Branches: el7
InitialCC:

Git done (by process-git-requests).

Jon - Looks like ownership wasn't given to jwhite when this request was processed.  Can that be fixed by you?

Just to note - EPEL7 branch name is "epel7", not "el7".

spice-html5-0.1.5-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/spice-html5-0.1.5-1.el7

Branch and package appear to exist.

spice-html5-0.1.5-1.el7 has been pushed to the Fedora EPEL 7 testing repository.

spice-html5-0.1.5-1.el7 has been pushed to the Fedora EPEL 7 stable repository.