Bug 879568

Summary: Review Request: <xs-release> - <XS repository configuration>
Product: [Fedora] Fedora Reporter: kparmar4
Component: Package ReviewAssignee: Michael Scherer <misc>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: aadavis1, misc, notting, package-review
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-11-24 21:27:12 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description kparmar4 2012-11-23 05:53:51 EST
Spec URL: <https://www.dropbox.com/s/7pv0yzuiks43t7y/xs-release%20%282%29.spec>
SRPM URL: <https://www.dropbox.com/s/dd8kmpu7pf5qlay/xs-release-6-1.0.2.fc17.src.rpm>
Description: <Hi, This package contains the XS repository configuration. I am working on it as my program's project in which I have to make this package available for x86_64/i686 & ARM architectures. I would appreciate if you would review it for me and give me feedback.>
Fedora Account System Username: kparmar4

Output of RPMLINT:

0 packages and 1 specfiles checked; 0 errors, 0 warnings.
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
xs-release.noarch: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.
xs-release.noarch: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 1 Michael Scherer 2012-11-23 06:38:31 EST
So, a few notes.

First, as said on irc and others bugs ( I just say it for people who will later read this bug ), the review cannot be completed because that's a item we cannot ship. 

See http://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_of_Package_Managers

However, for helping the review, i will continue the review just to show how it go.
Comment 2 Michael Scherer 2012-11-23 07:02:12 EST
So, a few notes :

%clean
rm -rf %{buildroot}

this part is no longer needed, rpm does it by default. So this is cleaner to not add this. See http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean


rm -rf %{buildroot}

same as above, already done by rpm. See http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

Unless if you plan to have a package for RHEL 5 or similar, this can be removed, as weel as the BuildRoot tag in the header.


%defattr(-,root,root,-)

same, that's the default since a few version of rpm. This can be safely removed :
http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


Since there is no tarball, there is no need to have %setup
%setup -q  -c -T

so you can remove %prep section all together ( even if rpmlint will complain )


Any package should have the license, and there is none. See http://fedoraproject.org/wiki/Packaging:LicensingGuidelines

There is no reason why the license would GPLv2 ( since that's written nowhere ), and if this is GPL, you need to distribute the text of the license with it. While for this precise case of configuration file, this is a little bit weird ( and just done to show the process ), the same apply to a regular software. 

IE, the reviewer need to be able to why this is under the license written in the tag ( usually, there is a file with the source code, or there is note in the source code ), and the license should be present when installing the package. 



# Epoch incremented to 1 so that it is seen as an upgrade over xs-release-9 (XS-0.6)
Epoch:		1

Usually, we try to avoid Epoch ( as this produce subtle interactions issues ), but for this one, why is xs-release using a different version number ( ie, if this was 9 before, why is it 6 now ) ? And I am not sure we should care about non fedora package.


URL:		http://wiki.laptop.org
Source0:	olpcxs.repo
Source1:	olpcxs-testing.repo

we usually need to know where the file come from. While for this case, we can say they were written by the packager, usually, the url where to find them, or a comment explaining how to update the file should be added. ( again, this package is not really the best example, so I does not really make sense for this specific packages )

Finally, some of us ( me included ) use a tool called fedora-review ( https://fedorahosted.org/FedoraReview/ ) for reviewing packages ( see https://bugzilla.redhat.com/show_bug.cgi?id=869861 for a recent example review ). And this tool wrk best when the url to the package are directly downloadable.

using dropbox force us to download the file by the browser to then run the tool. So if you could either use something else than dropbox, or put the direct link in the next review request ( if this exist, of course ), this would be better. 

Usually, after that, the reviewer wait for a new version of the spec and srpm with the correction of the issue noted. ( ie, just add a comment saying "I fixed this and this, and the new version of the spec is here, the new srpm is here" ) Then the spec is inspected again and checked against a checklist ( the part done by fedora-review tools ) and if there is more issues, explain them and wait again.

until the package is seen as non suitable for Fedora ( due to unfixable issues such as license problem, etc ), or until the packager change his mind, or until the package is ready to be packaged, where the process continue.

In your case, as you need a sponsor, there is another step, ie finding a sponsor.

Hope this help
Comment 3 kparmar4 2012-11-23 08:02:23 EST
Hi,
Thanks for the review and feedback.
I'll keep in mind all that you mentioned above.
As you would have seen in the spec file, I am not the main packager of this package. I have repackaged it so that it meets the guidelines.
But once again, thank you for the review.
Comment 4 Michael Scherer 2012-11-24 08:43:06 EST
*** Bug 879749 has been marked as a duplicate of this bug. ***
Comment 5 Michael Scherer 2012-11-24 08:44:53 EST
*** Bug 879750 has been marked as a duplicate of this bug. ***