Bug 487098 - Review Request: python-djblets - A collection of useful classes and functions for Django
Review Request: python-djblets - A collection of useful classes and functions...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Dave Malcolm
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 487097
  Show dependency treegraph
 
Reported: 2009-02-24 02:20 EST by ramez hanna
Modified: 2010-01-04 15:26 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-21 13:09:49 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dmalcolm: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
python-djblets 0.5.6 SRPM (221.10 KB, application/x-rpm)
2009-12-21 11:28 EST, Stephen Gallagher
no flags Details
specfile for python-djblets 0.5.6 (2.58 KB, application/octet-stream)
2009-12-21 11:30 EST, Stephen Gallagher
sgallagh: review? (dmalcolm)
Details

  None (edit)
Description ramez hanna 2009-02-24 02:20:42 EST
Spec URL: http://informatiq.org/files/djblets.spec
SRPM URL: http://informatiq.org/files/Djblets-0.5alpha3-1.fc10.src.rpm

Description: 
I earlier submitted ReviewBoard for review, Djblet is a dependency of it.
Comment 1 Fabian Affolter 2009-03-08 06:10:08 EDT
Please take a look at the Naming Guidelines of alpha releases.
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages
Comment 2 Fabian Affolter 2009-03-08 06:13:53 EDT
An other issue after a quick look at your spec file is that the changelog entry is not in a proper format.

https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Comment 3 Fabian Affolter 2009-04-18 08:23:42 EDT
ping?
Comment 4 Fabian Affolter 2009-04-23 09:37:47 EDT
NEWS should go into %doc.  0.5beta1 was released.
Comment 5 Dave Malcolm 2009-05-07 16:49:58 EDT
Should the package be named "python-djblets", rather than djblets/Djblets?
See https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29

I've taken the liberty of making an updated version of the package that renames it, fixes a few other issues, plus bumps to the latest upstream release (0.5rc1).  Hope that's ok.

Specfile is here:
http://people.redhat.com/dmalcolm/python/python-djblets.spec

SRPM here:
http://people.redhat.com/dmalcolm/python/python-djblets-0.5-0.1.rc1.src.rpm


Both RPM and SRPM are clean when run through rpmlint (rpmlint-0.82-3.el5); 
(Caveat: I did this on a RHEL5 box, rather than Fedora)


Re comment #2: I didn't reformat Ramez' changelog entry, thinking it better to preserve history.  rpmlint doesn't seem to complain about it.
Comment 6 Dan Young 2009-05-13 10:55:41 EDT
Not sure about status on this package. Ramez, it looks like you don't want ReviewBoard, I assume the same here. I'll take this if nobody else wants it.

Dave, your last spec looks good. Looks like there should be an explicit BuildRequires: python; I know there's already one on python-devel but: http://fedoraproject.org/wiki/Packaging/Python#BuildRequires

Also, should the BuildRequires: python-setuptools-devel should just be python-setuptools? Looks like -devel just includes easy_install, which isn't necessary to build Djblets.

I tested that in a mock chroot; it built OK w/:

BuildRequires: python
BuildRequires: python-devel
BuildRequires: python-setuptools

Spec:
http://files.mesd.k12.or.us/~dyoung/reviewboard/python-djblets.spec

SRPM:
http://files.mesd.k12.or.us/~dyoung/reviewboard/python-djblets-0.5-0.2.rc1.fc11.src.rpm
Comment 7 Dave Malcolm 2009-05-13 14:46:14 EDT
Based on https://bugzilla.redhat.com/show_bug.cgi?id=487097#c8 I'm guessing Ramez would be happy if one or both of us took this over.

Dan: the various things in comment #6 look good to me.  

I guess one of us needs to become the "submitter", and the other the "reviewer" to complete the process of getting this into Fedora (and then on to ReviewBoard...).

I'll look into reviewing Dan's packages listed in comment #6.
Comment 8 Dave Malcolm 2009-05-13 15:47:43 EDT
Scratch build performed in koji:
$ koji build --scratch dist-f12 python-djblets-0.5-0.2.rc1.fc11.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=1353465

I worked through http://fedoraproject.org/wiki/Packaging/ReviewGuidelines:

Summary is that we may need to clarify the licensing of the package; otherwise everything looks good to me.

Details follow:
    * MUST: rpmlint must be run on every package. The output should be posted in the review:
    Clean, with rpmlint-0.85-2.fc10.noarch
    
    * MUST: The package must be named according to the Package Naming Guidelines .
    Yes, python module for import as "djblets" hence "python-djblets"
    
    * MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption:
    Yes, they match
    
    * MUST: The package must meet the Packaging Guidelines:
    Appears correct; the Changelog format error is minor, and preserving history seems more important
    
    * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
    * MUST: The License field in the package spec file must match the actual license. [3]
    * 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.[4]
    http://code.google.com/p/reviewboard/wiki/Djblets states "Djblets is under the MIT license.", however, there is no license file within the upstream tarball.  Not all of the files in the upstream tarball carry a license header, though all that do that contain a "copyright" do contain an MIT-style license grant.
    Does upstream need to explicitly include a license file in the tarball? 
    
    In addition, the package contains an embedded copy of jquery, which is MIT/GPL dual license (installed below /usr/lib/python2.6/site-packages/djblets/media/js/ )
    This isn't a path conflict, merely a duplication of content.   My understanding is that Fedora' guidelines for JS packaging aren't yet decided on this issue.  My own feeling is that it's acceptable: the upstream have done their compatibility testing against this version of the library, and have written some URL logic to handle cache-correctness using it.
    
     
    * MUST: The spec file must be written in American English:
    Yes
    
    * MUST: The spec file for the package MUST be legible:
    Yes
    
    * 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.
MD5 sum from http://downloads.review-board.org/releases/Djblets-0.5rc1.tar.gz:
10b611756e1cbe03bfe35ab13b19638b  Djblets-0.5rc1.tar.gz

MD5 sum from SRPM:
10b611756e1cbe03bfe35ab13b19638b  Djblets-0.5rc1.tar.gz
    
    * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture:
    Yes, see scratch build.
    
    * 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.
    N/A
    
    * 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.
    Yes
    
    * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
    No translations
    
    * 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. [10]
    Not applicable
    
    * 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. [11]
    Not applicable
    
    * 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. [12]
    Looks good
    
    * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [13]
    OK
    
    * 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. [14]
    defattr present
    
    * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [15]
    clean present and correct.
    
    * MUST: Each package must consistently use macros. [16]
    Appears sane.  Uses %define, but I don't think that's ruled a blocker yet.
    
    * MUST: The package must contain code, or permissable content. [17]
    Code
    
    * 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). [18]
    Not applicable.
    
    * 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. [18]
    OK
    
    * MUST: Header files must be in a -devel package. [19]
    Not applicable.

    * MUST: Static libraries must be in a -static package. [20]
    Not applicable.
    
    * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [21]
    Not applicable.
    
    * 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. [19]
    Not applicable.

    * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [22]
    Not applicable.

    * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20]
    Not applicable.

    * 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. [23]
    Not applicable.

    * 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. [24]
    See notes on javascript above.
        
    * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [25]
    It does.
    
    * MUST: All filenames in rpm packages must be valid UTF-8. [26]
    Appears correct.
Comment 9 Dave Malcolm 2009-05-13 16:02:41 EDT
(and to complicate matters further, the setup.py has a GPLv2 license, but does state "MIT" in the metadata)
Comment 10 Dave Malcolm 2009-05-13 16:06:44 EDT
I've emailed fedora-legal-list requesting clarification; email is in moderation queue.
Comment 11 Dan Young 2009-05-13 16:16:48 EDT
I reported this as a bug to upstream here:
http://code.google.com/p/reviewboard/issues/detail?id=1120
Comment 12 Dave Malcolm 2009-05-13 17:20:11 EDT
Thanks.

I got a response from fedora-legal on the partial license grant on this question:
---
The project's website does state that the overall code license is MIT
[1], my concern was that only a subset of the files within the tarball
carry an explicit MIT grant in their headers, the others are devoid of
copyright/license text.

Based on that, it sounds like this is OK for inclusion, and that I was
being overly paranoid.  Is this correct?

Dave
[1] http://code.google.com/p/reviewboard/wiki/Djblets states "Djblets is
under the MIT license."
---

----
Response at:
https://www.redhat.com/archives/fedora-legal-list/2009-May/msg00025.html
> Based on that, it sounds like this is OK for inclusion, and that I was
> being overly paranoid.  Is this correct?

Yes, although, you should still ask upstream to fix the license
attribution on the files which do not contain it.
----

I think this package is ready to approve, with these caveats:
 - Please add a comment above the License tag indicating the upstream license text, and the link to fedora-legal-list above.
 - Please change License to "MIT and (MIT or GPL)", with a further comment about the bundled version of jquery.
Comment 13 Dan Young 2009-05-19 16:30:03 EDT
(In reply to comment #11)
> I reported this as a bug to upstream here:
> http://code.google.com/p/reviewboard/issues/detail?id=1120  

GPL boilerplate in setup.py has been fixed upstream:
http://code.google.com/p/reviewboard/issues/detail?id=1120#c1
Comment 14 Dave Malcolm 2009-08-21 18:41:03 EDT
Looks like I managed to let this stall; I think I was expecting dyoung to post a revised specfile, but on rereading I think dyoung was expecting me to approve.  Sorry about the miscommunication.

Based on comment #12, I'm flagging this review as approved, though please do address the caveats I mentioned there when you import the specfile:
>  - Please add a comment above the License tag indicating the upstream license
> text, and the link to fedora-legal-list above.
> - Please change License to "MIT and (MIT or GPL)", with a further comment
> about the bundled version of jquery.  

Thanks!

(Looks like upstream have released a new version in the meantime, though I think it's best to get this into Fedora as is with the old version, and then track the update in pkg cvs)
Comment 15 Jeffrey C. Ollie 2009-10-15 11:50:46 EDT
Ping?  Any recent action here?
Comment 16 Dave Malcolm 2009-10-15 18:10:32 EDT
(Jeffrey, thanks for the ping)

My understanding of the state of this bug is that I'm the reviewer, and that I've approved the review, and that whoever is to be the owner is now responsible for actually requesting the package import.

However, looking back over the history, it's not clear to me who the maintainer is to be: Ramez or Dan, and thus who's responsible for doing the next steps on  this review.  I'm happy to be maintainer or comaintainer of this package, but I don't know if that's acceptable within the package review rules.

Ramez?  Dan?  Do you still want to maintain this package for Fedora?

Sorry about my lack of clarity.
Comment 17 Dan Young 2009-10-15 18:42:32 EDT
(In reply to comment #16)
> My understanding of the state of this bug is that I'm the reviewer, and that
> I've approved the review, and that whoever is to be the owner is now
> responsible for actually requesting the package import.
> 
> However, looking back over the history, it's not clear to me who the maintainer
> is to be: Ramez or Dan, and thus who's responsible for doing the next steps on 
> this review.  I'm happy to be maintainer or comaintainer of this package, but I
> don't know if that's acceptable within the package review rules.
> 
> Ramez?  Dan?  Do you still want to maintain this package for Fedora?

Aggh, sorry I've been flaky on this. So found on more niggling license issue while adding the comment #12 additions, specifically that "GPL" is not a valid license tag (at least rpmlint complains). JQuery appears to be MIT or GPLv2 (no "or later" that I can find). Would a License tag of "MIT and (MIT or GPLv2)" work? I'll push the new package out ASAP if Dave signs off. Thanks,
Comment 18 Dave Malcolm 2009-10-21 17:55:31 EDT
My understanding is that values for the License tag in specfiles should use the "Short Name" column in the table of "Good Licenses" on https://fedoraproject.org/wiki/Licensing (and that not all of these are currently understood by rpmlint, so will generate a known error); further rules can be seen here:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines

As I understand it, it's more important to comply with those guidelines than to achieve silence from rpmlint.

GPLv2 without an "or later" appears in that table with short name "GPLv2"

So I believe the value of the license tag should be:
  License: MIT and (MIT or GPLv2)
and that you should add a comment to the specfile spelling out the situation as discussed above in comment #12.
Comment 19 Dan Young 2009-10-23 19:06:21 EDT
Updated spec with Comment #18 fixes:
http://files.mesd.k12.or.us/~dyoung/reviewboard/python-djblets.spec
Comment 20 Dan Young 2009-10-23 19:07:10 EDT
New Package CVS Request
=======================
Package Name: python-djblets
Short Description: A collection of useful classes and functions for Django
Owners: dyoung dmalcolm
Branches: F-11 F-12
InitialCC:
Comment 21 Kevin Fenzi 2009-10-26 16:18:21 EDT
cvs done.
Comment 22 Dan Young 2009-10-26 17:33:17 EDT
I've done the initial import into the devel branch.

I can tag and build 0.5rc1 in all the branches, but I'd rather go ahead with the update to upstream's latest, 0.5.5. I've tested a mock build; looks good here. Any thoughts?
Comment 23 Stephen Gallagher 2009-12-21 11:28:24 EST
Created attachment 379647 [details]
python-djblets 0.5.6 SRPM
Comment 24 Stephen Gallagher 2009-12-21 11:30:38 EST
Created attachment 379648 [details]
specfile for python-djblets 0.5.6

I took Dave's specfile from http://files.mesd.k12.or.us/~dyoung/reviewboard/python-djblets.spec and updated it to support the latest stable release of Djblets (0.5.6)

It would be helpful if this could be reviewed and committed to Fedora so as to facilitate the review process for ReviewBoard - https://bugzilla.redhat.com/show_bug.cgi?id=487097
Comment 25 Dave Malcolm 2009-12-21 13:09:49 EST
(In reply to comment #22)
> I've done the initial import into the devel branch.
> 
> I can tag and build 0.5rc1 in all the branches, but I'd rather go ahead with
> the update to upstream's latest, 0.5.5. I've tested a mock build; looks good
> here. Any thoughts?  
Sorry for not responding.

Looking in the package db, this is indeed in Fedora as "python-djblets"
https://admin.fedoraproject.org/pkgdb/packages/name/python-djblets

Looks like this review should have been closed out at that point.

Renaming this bug to "python-djblets" and closing this review out as "CLOSED CURRENTRELEASE".  

Re comment #23 and #24, looking at the above link I see that Dan took Stephen's changes and committed them and built them about an hour ago, so we have the latest version of python-djblets built into rawhide (thanks Dan!).

I think if there's a call to issue updates for earlier releases, file them as separate bugs to avoid confusing this review further.  I've checked and there is a "python-djblets" is a component in bz.
Comment 26 Stephen Gallagher 2010-01-04 14:49:54 EST
Package Change Request
======================
Package Name: python-djblets
New Branches: EL-5
Owners: sgallagh

python-djblets is a required dependency for ReviewBoard, which is being considered for inclusion in the Fedora Hosted infrastructure. We need to branch this package to EL-5 for this support.
Comment 27 Kevin Fenzi 2010-01-04 15:26:44 EST
cvs done.

Note You need to log in before you can comment on or make changes to this bug.