New URLS:
Spec URL: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM URL: http://tomspur.fedorapeople.org/review/shedskin-0.3.1-1.fc12.src.rpm

Changes:
- New version from upstream


######################

rpmlint is not clean (same as before):
many devel-file-in-non-devel-package warnings.
These files are needed at runtime, so splitting does not make sense at all.

######################


Builds in koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1918450

I'm not an approved packager yet, so I'll give you an informal review in hopes that it will help.

See below - rpmlint output

ok - Package meets naming guidelines
ok - Spec file matches base package name
NO - Meets Packaging Guidelines
NO - License (GPLv3+ and MIT and Paul Hsieh Derivative)
NO - License field in spec matches
ok - License file included in package
ok - Spec in American English
ok - Spec is legible
ok - Sources match upstream md5sum:
232885f019fda79a534c251ddd7e4c42  shedskin-0.3-1.tgz
232885f019fda79a534c251ddd7e4c42  shedskin-0.3-1.tgz.upstream

NO - BuildRequires correct
na - Spec handles locales/find_lang
na - Package has .so files in %{_libdir} and runs ldconfig in %post and %postun
ok - Package does not bundle system libs
na - Package relocatability is justified
ok - No duplicate files in %files
ok - Spec has %defattr in each %files section
ok - File permissions are sane
ok - Spec has a correct %clean section
ok - Spec has rm -rf %{buildroot} at top of %install
ok - Spec has consistant macro usage
ok - Package is code or permissible content
ok - Spec has correct buildroot
ok - File names valid UTF-8

ok - %doc files don't affect runtime
no - Headers go in -devel package  (Headers necessary for proper functioning)
na - Static libs go in -static package
ok - Package contains no .la files
na - Package is a GUI app and has a .desktop file installed w/ desktop-file-install
ok - Package owns all directories it creates
ok - Package's files and directories don't conflict with others'
ok - Package obeys FHS, except libexecdir and /usr/target

ok - Compiles and builds on at least one arch
ok - Compiles and builds on all archs or has ExcludeArch + bugs filed

SHOULD Items:

na - Query upstream for license inclusion
no - Translations of description and summary
ok - Builds in mock
na - Builds on all supported archs (noarch)
ok - Scriptlets are sane
na - Non-devel subpackages require base w/ fully-versioned dependency
na - pkgconfig (.pc) files go in -devel package
ok - Latest version
ok - Has dist tag
ok - No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin

########################################

* rpmlint output

shedskin.noarch: W: devel-file-in-non-devel-package /usr/lib/python2.6/site-packages/shedskin/lib/socket.cpp
(previous message repeated for 47 other source files)
shedskin.noarch: W: incoherent-version-in-changelog 0.3-1 ['0.3.1-1.fc12', '0.3.1-1']

devel-file-in-non-devel-package is fine because this is a development tool that requires the headers and sources in question to function correctly.

* Meets packaging guidelines

The version you give in the changelog (0.3) doesn't match that of the package (0.3.1).  In fact, I'm confused overall as to what the program's actual version number is.  The tarball, readme, and Debian package indicate that it is 0.3, while the version given in the spec file is 0.3.1.  If this is a svn checkout of a post-0.3 tree, the Release field should indicate so.

* License

Portions of a couple sources seem to have licenses other than GPLv3+.
- shedskin/lib/time.cpp:  LGPLv2.1+
- shedskin/lib/builtin.cpp:  Paul Hsieh derivative license

The Paul Hsieh derivative license is not allowed in Fedora.  However, the LICENSING file gives MIT licenses for most everything in shedskin/lib; were those portions of the code relicensed?

http://fedoraproject.org/wiki/Licensing#Bad_Licenses

* License field in spec matches

Most files in shedskin/lib are MIT-licensed according to the LICENSING file, but the License field does not reflect this.

* BuildRequires correct

Since you're providing a setuptools egg you need BuildRequires: python-setuptools-devel

http://fedoraproject.org/wiki/Packaging/Python/Eggs#Providing_Eggs_using_Setuptools

(In reply to comment #2)
> I'm not an approved packager yet, so I'll give you an informal review in hopes
> that it will help.

Thanks anyway. Great review!

> * Meets packaging guidelines
> 
> The version you give in the changelog (0.3) doesn't match that of the package
> (0.3.1).  In fact, I'm confused overall as to what the program's actual version
> number is.  The tarball, readme, and Debian package indicate that it is 0.3,
> while the version given in the spec file is 0.3.1.  If this is a svn checkout
> of a post-0.3 tree, the Release field should indicate so.

Upstream released 0.3 and after fixing, 0.3-1 was released. -> I used 0.3.1 as version number, because there is no other way to reflect this (Debian should be wrong here). They also can't explain why using '-' and not '.' there. Don't know, if they will change the version numbering in the future.

-> It's not a svn checkout (you also did a md5check above, I downloaded it from there, too).

> 
> * License
> 
> Portions of a couple sources seem to have licenses other than GPLv3+.
> - shedskin/lib/time.cpp:  LGPLv2.1+
> - shedskin/lib/builtin.cpp:  Paul Hsieh derivative license
> 
> The Paul Hsieh derivative license is not allowed in Fedora.  However, the
> LICENSING file gives MIT licenses for most everything in shedskin/lib; were
> those portions of the code relicensed?
> 
> http://fedoraproject.org/wiki/Licensing#Bad_Licenses

Thanks for that. Don't know, why I missed that.

Will ask upstream for that.

> * License field in spec matches
> 
> Most files in shedskin/lib are MIT-licensed according to the LICENSING file,
> but the License field does not reflect this.
> 
> * BuildRequires correct
> 
> Since you're providing a setuptools egg you need BuildRequires:
> python-setuptools-devel
> 
> http://fedoraproject.org/wiki/Packaging/Python/Eggs#Providing_Eggs_using_Setuptools    

Quoting from there:
'When upstream uses setuptools to provide eggs [...]'

Upstream uses distutils and not setuptools, so there should be no need to add this as BR.

New URLS till resolving the license issue :(
Spec URL: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM URL: http://tomspur.fedorapeople.org/review/shedskin-0.3.1-2.fc12.src.rpm

Make
 %{python_sitelib}/*
more explicit, with the wildcard you may e.g. miss if the eggs are not built.

(In reply to comment #4)
> Make
>  %{python_sitelib}/*
> more explicit, with the wildcard you may e.g. miss if the eggs are not built.    

Thanks, will do so (maybe) sometime ;)

__________________________________________________________________________

Upstream will resolve the license issues in 'a few weeks'.

When this is done, I can reopen this review request, or maybe someone else wants to take this then.

Currently I experiment with mpi4py and shedskin currently does not support that, so it depends on my mood in a few weeks. ;)

As far as I can tell, the reference to the Paul Hsieh license was removed in this commit:
  http://code.google.com/p/shedskin/source/detail?r=1187

(In reply to comment #6)
> As far as I can tell, the reference to the Paul Hsieh license was removed in
> this commit:
>   http://code.google.com/p/shedskin/source/detail?r=1187

Yes, it seems Paul Hsieh license is now BSD-like:
http://www.azillionmonkeys.com/qed/weblicense.html

-> Blocking FE-LEGAL to be sure (and maybe let the licensing packge get an update)

SPEC: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM: http://tomspur.fedorapeople.org/review/shedskin-0.5-1.fc13.src.rpm

Which Paul Hsieh license is in play here? Only his BSD license (which is just BSD) is Free, his other two licenses (notably, his derivative license) are non-free.

(In reply to comment #8)
> Which Paul Hsieh license is in play here? Only his BSD license (which is just
> BSD) is Free, his other two licenses (notably, his derivative license) are
> non-free.

It once was his derivative license, but Paul wanted to let others also use a free license instead. From the webpage above:
"For the specific coverage of raw source code (only) obtained from this website, you have the option of using the old-style BSD license to use the code instead of other the licenses."

So it looks like, the source in shedskin/lib/builtin.cpp, which is copyrighted by Paul is allowed to use BSD, isn't it?

If this is still not a BSD license now, I'll try to delete that upstream and replace it with a free hash function (there should be plenty of them out there).

The Paul Hsieh license is out now.

Shedskin now uses: http://sites.google.com/site/murmurhash/

which is Public Domain or for bussines purposes MIT, so I choosed MIT now, to comply with the rest of shedskin/lib/*.

-> unblock FE-LEGAL

SPEC: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM: http://tomspur.fedorapeople.org/review/shedskin-0.6-1.fc13.src.rpm

"All code is released to the public domain. For business purposes, Murmurhash is under the MIT license."

I'm having trouble understanding what that actually means.  Is it public domain or not?  What are "business purposes"?  Why do software authors think they can just make random legal-sounding statements like that?

Being a separate upstream project (own version, release schedule, license, etc.) the murmurhash code would seem to run afoul of the bundled library policy.  It seems like a pretty clear exception (it's only 50 lines of code anyway) but the current policy has no automatic exemption based on size.

Can you file an exception request with FPC?

(In reply to comment #11)
> "All code is released to the public domain. For business purposes, Murmurhash
> is under the MIT license."
> 
> I'm having trouble understanding what that actually means.  Is it public domain
> or not?  What are "business purposes"?  Why do software authors think they can
> just make random legal-sounding statements like that?

I don't know... I think using MIT directly and we are save, isn't it?
Furthermore the rest of shedskin/lib/* is MIT, so it would be best to do so too...

> Being a separate upstream project (own version, release schedule, license,
> etc.) the murmurhash code would seem to run afoul of the bundled library
> policy.  It seems like a pretty clear exception (it's only 50 lines of code
> anyway) but the current policy has no automatic exemption based on size.
> 
> Can you file an exception request with FPC?

Done at:
https://fedorahosted.org/fpc/ticket/39

I think a bundled lib is a header file or some other sorf of copylib (with or without modifications). Shedskin is only copying one function, which is deeply implemented into shedskin and not called in a header file, so this doesn't comply with my 'definition' of a bundled lib...

So thanks for giving me the advice of asking FPC (wouldn't have done that otherwise, to be honest...).

"Exception granted."

-> provide bundled(murmurhash)

SPEC: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM: http://tomspur.fedorapeople.org/review/shedskin-0.6-2.fc13.src.rpm

BTW, I also asked about the murmurhash license and the result is that we'll just use it under the MIT license.  Is there some other part of the software that is public domain?  Also, keep in mind that the license tag reflects the binary package, not the source package, so it is not useful to indicate which parts of the source are under which license.

(In reply to comment #14)
> BTW, I also asked about the murmurhash license and the result is that we'll
> just use it under the MIT license.

ok, thanks.

> Is there some other part of the software
> that is public domain?

Just some examples or scripts, which are not installed or used later on.
So I guess Public Domain can be deleted now...

> Also, keep in mind that the license tag reflects the
> binary package, not the source package, so it is not useful to indicate which
> parts of the source are under which license.

Yes, the python part of shedskin is GPLv3 and the c++ library is MIT (used for linking later on).
So "GPLv3 and MIT" should be correct, isn't it?

Depends; I haven't built the package to see if the MIT-license code appears by itself in any binary files.  I also didn't audit for GPLv3 vs. GPLv3+.  That's all something you need to do.  I was just commenting on the fact that your comments before the license tag refer to the source code, when what's important is the licenses on the files in the binary package.

(In reply to comment #16)
> Depends; I haven't built the package to see if the MIT-license code appears by
> itself in any binary files.  I also didn't audit for GPLv3 vs. GPLv3+.  That's
> all something you need to do.  I was just commenting on the fact that your
> comments before the license tag refer to the source code, when what's important
> is the licenses on the files in the binary package.

"Binary" is only GPLv3, the python source code, but because there is the C++ lib installed later to be used, when actually building the translated C++ programm, you use MIT code.

So I'd use "GPLv3 and MIT" in this case, but if license should only apply to 'binary', it's GPLv3 (that would make me wonder...).

Well, read these:

http://fedoraproject.org/wiki/Licensing:FAQ#How_should_I_handle_multiple_licensing_situations.3F
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License:_field

Do those not answer your question?

(In reply to comment #18)
> Well, read these:
> 
> http://fedoraproject.org/wiki/Licensing:FAQ#How_should_I_handle_multiple_licensing_situations.3F
> http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License:_field
> 
> Do those not answer your question?

Partly.

Your "binary" still confuses me...

But your second link talks about "files", and the source files are MIT, python binaries are GPLv3, same what I wrote above.

-> "GPLv3 and MIT"

At this point I can't figure out why you're not getting it.

"
The source code contains some .c files which are GPLv2+ and some other .c files which are BSD. They're compiled together to form an executable. Since some of the files are licensed as GPL, the resulting executable is also GPL. The License tag should read: License: GPLv2+ 
"

But you've read that and it doesn't answer the question I think you're asking, so perhaps you're simply asking a different question than the one I'm answering.

Let me try a different way.  Please tell me which file present in the built, non-debuginfo rpm, is under the MIT license.  Just find one that is pure MIT with no GPL code compiled together with it.  You have to do this anyway, because when you include a complicated license tag like that you have to say which files are under which license.

(In reply to comment #20)
> Let me try a different way.  Please tell me which file present in the built,
> non-debuginfo rpm, is under the MIT license.  Just find one that is pure MIT
> with no GPL code compiled together with it.

Pure MIT is the C++ header library, what I told above.

What's confusing is, that you link later on when actually using shedskin with the MIT library, but you are using a GPLed python binary.
So using and modifying the python parts needs to happen under the GPL, but using the C++ library allowes to use MIT.

shedskin/lib/* is MIT, but nothing is build/linked at compile time. Only at 'using time' later on, so there is no binary (what was confusing me).

Hope I did it right like this...

New version:

SPEC: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM: http://tomspur.fedorapeople.org/review/shedskin-0.7-1.fc13.src.rpm

Okay, I've grepped through the source code.  It looks like there's no need to list Public Domain now.  But we do need to list Python since code under that license is used in builtin.cpp.  I think that this would be a proper License tag and comment:

# The dict implementation in shedskin/lib/builtin.cpp is under the Python
# license. The Murmurhash implementation in builtin.cpp is bundled (noted
# below) and licensed MIT.  Other files in shedskin/lib/ are MIT, rest GPLv3
License:        GPLv3 and (MIT and Python)

Toshio is right; just fix the license field and you should be good to go.  A full review is attached.

Created attachment 469201 [details]
Review for new package shedskin-0.7-1.fc15

Looks like attaching reviews causes more problems than it solves, so here's a copy of the whole thing:

This is a review of proposed package shedskin-0.7-1.fc15.

Spec file:   http://tomspur.fedorapeople.org/review/shedskin.spec
Source RPM:  http://tomspur.fedorapeople.org/review/shedskin-0.7-1.fc13.src.rpm

Mandatory review guidelines:
ok - rpmlint output
     devel-file-in-non-devel-package justified by development tool exception
ok - Package meets naming guidelines
ok - Spec file name matches base package name
ok - License is acceptable (GPLv3 and (MIT and Python))
NO - License field in spec is correct
     The shedskin binary is GPLv3, while the libraries are MIT and Python.
ok - License files included in package %docs or not included in upstream source
ok - License files installed when any subpackage combination is installed
ok - Spec written in American English
ok - Spec is legible
ok - Sources match upstream unless altered to fix permissibility issues
     Upstream MD5:  0cd084152d8d2ddd719bf79572804e22  shedskin-0.7.tgz
     Your MD5:      0cd084152d8d2ddd719bf79572804e22  shedskin-0.7.tgz
     No idea why rpmlint has trouble fetching the tarball; it works for me.
ok - Build succeeds on at least one supported platform
ok - Build succeeds on all supported platforms or has ExcludeArch + bugs filed
ok - BuildRequires correct
na - Package handles locales with %find_lang
na - %post, %postun call ldconfig if package contains shared .so files
ok - No bundled system libs
     bundled(murmurhash) justified by FPC
na - Relocatability is justified
ok - Package owns all directories it creates
ok - Package requires other packages for directories it uses but does not own
ok - No duplicate files in %files unless necessary for license files
ok - File permissions are sane
ok - Each %files section contains %defattr
ok - Consistent use of macros
ok - Sources contain only permissible code or content
na - Large documentation files go in -doc package
ok - Missing %doc files do not affect runtime
ok - Headers go in -devel package
     Development tool exception
na - Static libs go in -static package
na - Unversioned .so files go in -devel package
na - Devel packages require base with fully-versioned dependency
ok - Package contains no .la files
na - GUI app installs .desktop file w/ desktop-file-install or has justification
ok - Package's files and directories don't conflict with others' or justified
ok - File names are valid UTF-8

Optional review guidelines:
na - Query upstream about including license files
no - Translations of description, Summary
ok - Builds in mock
ok - Builds on all supported platforms
na - Scriptlets are sane
ok - Non-devel subpackage Requires are sane
na - .pc files go in -devel unless main package is a development tool
ok - No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
no - Man pages included for all executables

Packaging guidelines:
ok - Has dist tag
ok - Useful without external bits
ok - Package obeys FHS, except libexecdir and /usr/target
ok - Changelog in prescribed format
ok - Spec file lacks Packager, Vendor, PreReq tags
ok - Correct BuildRoot tag on < F10/EL6
ok - Correct %clean section on < F13/EL6
ok - Requires correct, justified where necessary
ok - Summary, description do not use trademarks incorrectly
ok - All relevant documentation is packaged, tagged appropriately
ok - %build honors applicable compiler flags or justifies otherwise
na - Package with .pc files Requires pkgconfig on < EL6
na - Useful -debuginfo package or disabled and justified
ok - No static executables
ok - Rpath absent or only used for internal libs
ok - Config files marked with %config
ok - %config files marked noreplace or justified
ok - No %config files under /usr
na - SysV-style init script
ok - Spec uses macros instead of hard-coded directory names
ok - %makeinstall used only when ``make install DESTDIR=...'' doesn't work
ok - Macros in Summary, %description expandable at SRPM build time
ok - Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR or %{sourcedir}
ok - %global instead of %define where appropriate
na - Package containing translations BuildRequires gettext
ok - File timestamps preserved by file ops
na - Parallel make
ok - Spec does not use Requires(pre,post) notation
na - User, group creation handled correctly (See Packaging:UsersAndGroups)
na - Web app files go in /usr/share/%{name}, not /var/www
na - Conflicts are justified
ok - No external kernel modules
ok - No files in /srv
ok - One project per package
ok - Patches link to upstream bugs/comments/lists or are otherwise justified

Python Guidelines:
ok - Runtime Requires correct
ok - Python macros declared on < F13/EL6
     This package will not build on EL5 for this reason.
ok - All .py files packaged with .pyc, .pyo counterparts
ok - Includes .egg-info files/directories when generated
ok - Provides/Requires properly filtered
na - Code that invokes gtk.gdk.get_pixels_array() Requires numpy

(In reply to comment #23)
> Okay, I've grepped through the source code.  It looks like there's no need to
> list Public Domain now.  But we do need to list Python since code under that
> license is used in builtin.cpp.  I think that this would be a proper License
> tag and comment:
> 
> # The dict implementation in shedskin/lib/builtin.cpp is under the Python
> # license. The Murmurhash implementation in builtin.cpp is bundled (noted
> # below) and licensed MIT.  Other files in shedskin/lib/ are MIT, rest GPLv3
> License:        GPLv3 and (MIT and Python)

Thanks, Toshio, for looking.

(In reply to comment #24)
> Toshio is right; just fix the license field and you should be good to go.  A
> full review is attached.

Thanks for the review.

Fixed License in:
SPEC: http://tomspur.fedorapeople.org/review/shedskin.spec
SRPM: http://tomspur.fedorapeople.org/review/shedskin-0.7-2.fc13.src.rpm

Looks good.  Enjoy!

Great :)

New Package SCM Request
=======================
Package Name: shedskin
Short Description: Python to C++ compiler
Owners: tomspur
Branches: f13 f14
InitialCC:

Git done (by process-git-requests).

shedskin-0.7-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/shedskin-0.7-2.fc14

shedskin-0.7-2.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/shedskin-0.7-2.fc13

shedskin-0.7-2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update shedskin'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/shedskin-0.7-2.fc14

shedskin-0.7-3.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

shedskin-0.7-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: shedskin
New Branches: el5 el6
Owners: tomspur

Git done (by process-git-requests).