Bug 509533 - Review Request: sap - A small CLI audio player
Review Request: sap - A small CLI audio player
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-03 06:55 EDT by Julian Aloofi
Modified: 2009-08-15 04:31 EDT (History)
3 users (show)

See Also:
Fixed In Version: 0.4.4-7.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-15 04:31:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Julian Aloofi 2009-07-03 06:55:58 EDT
Spec URL: http://julian.fedorapeople.org/sap.spec
SRPM URL: http://julian.fedorapeople.org/sap-0.4.4-1.fc11.src.rpm
Description: SAP is a small CLI audio player featuring a volume control and
supports many file formats through the gstreamer framework.

As I don't have any packages in Fedora yet, I need a sponsor for this.
Comment 1 Michael Schwendt 2009-07-04 13:02:49 EDT
> %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+".
Comment 2 Julian Aloofi 2009-07-04 13:56:54 EDT
Thanks for the correction. I updated the spec file and rebuilt the SRPM.
Comment 3 Susi Lehtola 2009-07-06 09:11:25 EDT
- 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.
Comment 4 Julian Aloofi 2009-07-06 09:31:07 EDT
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.
Comment 5 Susi Lehtola 2009-07-10 09:09:06 EDT
(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.
Comment 6 Julian Aloofi 2009-07-11 06:58:54 EDT
(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
Comment 7 Susi Lehtola 2009-07-11 07:06:49 EDT
The release should have gone to 2 not 4 :)
Comment 8 Julian Aloofi 2009-07-11 08:56:49 EDT
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?
Comment 9 Susi Lehtola 2009-07-11 09:04:28 EDT
(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.
Comment 10 Julian Aloofi 2009-07-11 10:03:20 EDT
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
Comment 11 Julian Aloofi 2009-07-12 18:05:29 EDT
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 :)
Comment 12 Susi Lehtola 2009-07-12 18:31:32 EDT
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.
Comment 13 Julian Aloofi 2009-07-12 18:47:10 EDT
(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.
Comment 14 Susi Lehtola 2009-07-13 06:06:03 EDT
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
Comment 15 Julian Aloofi 2009-07-13 07:26:08 EDT
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
Comment 16 Julian Aloofi 2009-07-13 09:52:21 EDT
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.
Comment 17 Julian Aloofi 2009-07-17 19:51:33 EDT
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
Comment 18 Susi Lehtola 2009-07-18 05:49:21 EDT
(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.
Comment 19 Julian Aloofi 2009-07-24 10:22:46 EDT
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?
Comment 20 Susi Lehtola 2009-07-24 10:31:01 EDT
(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.
Comment 21 Julian Aloofi 2009-07-24 20:56:59 EDT
Here's another submission I made:
https://bugzilla.redhat.com/show_bug.cgi?id=513733
Comment 22 Susi Lehtola 2009-07-25 16:32:19 EDT
This package has been

APPROVED


I couldn't find you on the sponsorship queue. Request for membership in the FAS Packager group ASAP.
Comment 23 Julian Aloofi 2009-07-25 17:20:29 EDT
New Package CVS Request
=======================
Package Name: sap
Short Description: A small CLI audio player
Owners: julian
Branches: F-11
InitialCC:
Comment 24 Julian Aloofi 2009-07-25 17:22:20 EDT
That's that. Thanks for your patience and all the help!
Comment 25 Susi Lehtola 2009-07-25 18:33:00 EDT
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 :)
Comment 26 Kevin Fenzi 2009-07-26 15:42:33 EDT
cvs done.
Comment 27 Julian Aloofi 2009-07-26 16:25:47 EDT
(In reply to comment #25)
> No F-10 branch?
The latest vala in F10 is too old for this package, so unfortunately not.
Comment 28 Susi Lehtola 2009-07-26 16:58:56 EDT
(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.
Comment 29 Fedora Update System 2009-07-26 18:52:27 EDT
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
Comment 30 Fedora Update System 2009-07-27 17:36:58 EDT
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
Comment 31 Fedora Update System 2009-08-15 04:31:03 EDT
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.

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