Bug 509533

> %install
> rm -rf $RPM_BUILD_ROOT
> sh ./build
> mkdir -p %{buildroot}%{_bindir}
>>>
> %clean
> rm -rf $RPM_BUILD_ROOT

Use either $RPM_BUILD_ROOT or %buildroot, not both at once.


> mkdir -p %{buildroot}%{_defaultdocdir}/%{name}-%{version}
> cp -p README gpl.txt %{buildroot}%{_defaultdocdir}/%{name}-%{version}

You could simply include them with %doc in the %files section:
%doc README gpl.txt


> mkdir -p %{buildroot}%{_mandir}/man1
> gzip sap.1
> cp -p sap.1.gz %{buildroot}%{_mandir}/man1

Install it uncompressed, and rpmbuild will compress it automatically.

> %{_mandir}/man1/sap.1.gz

Here prefer %{_mandir}/man1/sap.1.* as automatic compression of manual pages makes it possible to replace gzip with a different compressor any time.


> %doc
> %{_defaultdocdir}/%{name}-%{version}
> %{_defaultdocdir}/%{name}-%{version}/README
> %{_defaultdocdir}/%{name}-%{version}/gpl.txt

These lines can be deleted in favour of the "%doc README gpl.txt"  mentioned above.


> License: GPLv3

Actually the source files say it's "GPLv3+".

Thanks for the correction. I updated the spec file and rebuilt the SRPM.

- Don't run the build any more in %install: drop the line
 sh ./build

- Instead of running
 mkdir -p %{buildroot}%{_bindir}
 cp -p sap %{buildroot}%{_bindir}
 mkdir -p %{buildroot}%{_defaultdocdir}/%{name}-%{version}
 mkdir -p %{buildroot}%{_mandir}/man1
 cp -p sap.1 %{buildroot}%{_mandir}/man1
you could just
 install -D -p -m 755 sap %{buildroot}%{_bindir}/sap
 install -D -p -m 644 sap.1 %{buildroot}%{_mandir}/man1/sap.1

- You could do with a bit of space inbetween spec file sections.

Thank you, I edited the spec file and rebuilt the SRPM again.
I was not really familiar with the install command so I didn't use it, but familiarized myself with it now.
I also left more space in the spec file.

(In reply to comment #4)
> Thank you, I edited the spec file and rebuilt the SRPM again.
> I was not really familiar with the install command so I didn't use it, but
> familiarized myself with it now.
> I also left more space in the spec file.  

You should increment the Release tag every time you make changes to the spec file. Otherwise it is really confusing.

(Now you have even too much space! One empty line or two is enough in between sections of the spec file.)

Once you have been sponsored you will be able to do formal package reviews of your own. I am willing to sponsor you, if you demonstrate your knowledge of the Fedora packaging guidelines by submitting at least one other package for review and perform informal reviews of packages of other people.

(In reply to comment #5)
> You should increment the Release tag every time you make changes to the spec
> file. Otherwise it is really confusing.
> 
> (Now you have even too much space! One empty line or two is enough in between
> sections of the spec file.)
> 
> Once you have been sponsored you will be able to do formal package reviews of
> your own. I am willing to sponsor you, if you demonstrate your knowledge of the
> Fedora packaging guidelines by submitting at least one other package for review
> and perform informal reviews of packages of other people.  

Thank you! I'll start with that as soon as possible and then post the related links here. I adjusted the Release tag and deleted some whitespaces.
The new SRPM is here:
http://julian.fedorapeople.org/sap-0.4.4-4.fc11.src.rpm

The release should have gone to 2 not 4 :)

If I change it now I had to set release to 3. That would be confusing...
Just changed to 4 because it is the 4th release :)
Should I change it?

(In reply to comment #8)
> If I change it now I had to set release to 3. That would be confusing...
> Just changed to 4 because it is the 4th release :)
> Should I change it?  

Oh, that's right. You just need to fill in the changelog for the missing entries.

OK, that includes all fixes and changelogs:
Spec URL: http://julian.fedorapeople.org/sap/sap.spec
SRPM URL: http://julian.fedorapeople.org/sap/sap-0.4.4-5.fc11.src.rpm

I did a review on this package, which was pretty easy:
https://bugzilla.redhat.com/show_bug.cgi?id=508352
I will look for another package that may be more of a thrill and demonstrates my knowledge of the guidelines more impressively :)

Okay.

Please review only packages that aren't tagged with FE-NEEDSPONSOR, since after your informal review I will have to do the formal one to check if you have got everything correctly. Also, be sure to check everything in the review guidelines, http://fedoraproject.org/wiki/Packaging/ReviewGuidelines.

Remember that you have to do another submission as well.

(In reply to comment #12)
> Okay.
> 
> Please review only packages that aren't tagged with FE-NEEDSPONSOR, since after
> your informal review I will have to do the formal one to check if you have got
> everything correctly. Also, be sure to check everything in the review
> guidelines, http://fedoraproject.org/wiki/Packaging/ReviewGuidelines.
> 
> Remember that you have to do another submission as well.  
Okay, already thought that the Review above wouldn't really count :)

> Remember that you have to do another submission as well.  
That could take a bit longer because I'm going to vacation in two weeks and I'm not sure whether I'll find the time to create one before that.

Here's the review in full:


rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- You should have empty lines between changelog entries.
(Also, I prefer to have %doc in the %files section straight after the %defattr line.)

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Instead of
 sh ./build
I suggest using
 valac --thread --pkg curses --pkg gstreamer-0.10 curses_ui.vala audioplayer.vala main_controller.vala -C
to generate C code from the vala source and then
 gcc %{optflags} `pkg-config glib-2.0 --cflags --libs` `pkg-config gstreamer-0.10 --cflags --libs` -lncurses  audioplayer.c curses_ui.c main_controller.c -o sap
to compile the binary from the generated C code. (I can't verify the use of Fedora optimization flags from the plain call of vala).

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. N/A
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so 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. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

OK, edited the package again, SRPM is here:
http://julian.fedorapeople.org/sap/sap-0.4.4-6.fc11.src.rpm
and spec file is here:
http://julian.fedorapeople.org/sap/sap.spec

OK, I did a complete review now (except of assigning the bug to myself) on this package:
https://bugzilla.redhat.com/show_bug.cgi?id=507943
It also doesn't Block FE-NEEDSPONSOR. I hope it's OK.

I'm still here, I'm waiting for upstreams next release of eViacam, and if it doesn't help I'll pick another piece of software. Should I do some Reviews in the meantime? I also updated the sap package again:
SPEC file: http://julian.fedorapeople.org/sap/sap.spec
SRPM: http://julian.fedorapeople.org/sap/sap-0.4.4-7.fc11.src.rpm

(In reply to comment #17)
> I'm still here, I'm waiting for upstreams next release of eViacam, and if it
> doesn't help I'll pick another piece of software.

You can have a look at the Fedora wishlist for ideas.

> Should I do some Reviews in the meantime?

Yeah, you should do another one.

> I also updated the sap package again:
> SPEC file: http://julian.fedorapeople.org/sap/sap.spec
> SRPM: http://julian.fedorapeople.org/sap/sap-0.4.4-7.fc11.src.rpm  

You don't have to be so fussy about BuildRequires, since they're only installed in the temporary build root when you build the package; it's Requires: you really have to care about. But this is fine.

Hello Jussi, I created another package, it's #513619
As far as I can see, all that's missing is another Package Review now, right?

(In reply to comment #19)
> Hello Jussi, I created another package, it's #513619
> As far as I can see, all that's missing is another Package Review now, right?  

Yes. Once you have done it request membership in the Packager group in FAS (if you haven't done so already) and I will sponsor you.

Removing FE-NEEDSPONSOR flag as it is no more necessary.

Here's another submission I made:
https://bugzilla.redhat.com/show_bug.cgi?id=513733

This package has been

APPROVED


I couldn't find you on the sponsorship queue. Request for membership in the FAS Packager group ASAP.

New Package CVS Request
=======================
Package Name: sap
Short Description: A small CLI audio player
Owners: julian
Branches: F-11
InitialCC:

That's that. Thanks for your patience and all the help!

No F-10 branch?

(In reply to comment #24)
> That's that. Thanks for your patience and all the help!  

No problem. Contact me if you have anything more to ask.

We have a big review queue, so *please* start reviewing packages of other people. Now as you are sponsored you are able do official reviews. Reviewing a package is a one-time thing; maintaining it is a more long term activity :)

cvs done.

(In reply to comment #25)
> No F-10 branch?
The latest vala in F10 is too old for this package, so unfortunately not.

(In reply to comment #27)
> (In reply to comment #25)
> > No F-10 branch?
> The latest vala in F10 is too old for this package, so unfortunately not.  

Right, I had forgotten about that.

sap-0.4.4-7.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/sap-0.4.4-7.fc11

sap-0.4.4-7.fc11 has been pushed to the Fedora 11 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 sap'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8040

sap-0.4.4-7.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.