Please note new URLS:

SRPM:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzN2M4ZWNkZmYtZWU5MS00NjUzLWFhN2EtY2QwNjljMWUyOWYz&hl=en&authkey=COWCqZUM
SPEC:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzZjgxOTg2YWQtYjE1OC00ZjFlLThkYjItZTE5NWE4M2U0MmZh&hl=en&authkey=CNSS3vEK

and rpmlint output:

jackbeat.src: W: spelling-error %description -l en_US resized -> resined, resided, re sized
jackbeat.src: W: spelling-error %description -l en_US jackeq -> jacket, jacked, jack eq
jackbeat.src: W: spelling-error %description -l en_US olivier -> Olivier
jackbeat.src: W: spelling-error %description -l en_US xung -> sung, rung, lung
jackbeat.src: W: no-version-in-last-changelog
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

I took a quick look into your SPEC file and found a few issues:

- Is it just me or is the macro %global desktop_vendor fedora unnecessary?

- The licence information is not correct. The distributed COPYING file states that the licence would be GPLv2 but you find further information in the main.c (and others)

 * [...] redistribute it and/or modify it under the terms of the GNU
 * General Public License as published by the Free Software Foundation;
 * either version 2 of the License, or (at your option) any later version.
[...]

So GPLv2+ would be the right licence tag then. Refer to
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
and
http://fedoraproject.org/wiki/Licensing
for further information. In case of any doubt you can ask upstream about the
licence.

- Additionally, you forgot to include the version number in your changelog
entry. (As rpmlint told you)

Thanks so much for your feedback so quickly! I have made changes as directed.

New SRPM:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzMjczNzNiMTAtMDEzMi00ODc4LThiMjktZmM2ODdiNTQ0NzM1&hl=en&authkey=CITh9iY

New SPEC:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzOWNiOGY2MWMtODBkYS00NGI4LWExNmMtMDE3ZTJiZTQ1MDcz&hl=en&authkey=CIqz48QI

rpmlint jackbeat-0.7.6-1.fc14.src.rpm 
jackbeat.src: W: spelling-error %description -l en_US resized -> resined, resided, re sized
jackbeat.src: W: spelling-error %description -l en_US jackeq -> jacket, jacked, jack eq
jackbeat.src: W: spelling-error %description -l en_US olivier -> Olivier
jackbeat.src: W: spelling-error %description -l en_US xung -> sung, rung, lung
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

I had a closer look at your SPEC file and found some more issues:

- Some BuildRequires depend on others, so not all BuildRequires need to be
  listed:

  gtk2-devel depends on automake
  libgnomecanvas-devela depends on gtk2-devel
  libsamplerate-devel depends on pulseaudio-libs-devel
  libxml2-devel depends on zlib-devel

- I do not understand why you need 'export CPPFLAGS="$RPM_OPT_FLAGS"'. Maybe
  I am missing something but aren't the $RPM_OPT_FLAGS used anyway?

- You have to take a closer look at the %files section, the package creates
  directories it does not own. They wouldn't be deleted when
  the package is removed for example. You may find this useful:
  http://fedoraproject.org/wiki/Packaging:UnownedDirectories
  
Finally, I have a question concerning the icon file you provide additionally. Where does it come from? Are there any licence issues or is it safe to include it? I would recommend to make a comment on this in the SPEC file.

Thanks again, I'm finding this really helpful. I will update the dependencies and have removed the reference to $RPM_OPT_FLAGS .

I created the icon based on the upstream image - no other icon is provided by the upstream source.

thanks for the unowned directories tip. I have read through the docco there. Before I post a new .spec , is this correct?

74 %files
 75 %defattr(-,root,root,-)
 76 %doc AUTHORS ChangeLog COPYING README NEWS
 77 %dir %{_datadir}/%{name}/glade
 78 %dir %{_datadir}/%{name}/pixmaps
 79 %{_bindir}/%{name}
 80 %{_datadir}/%{name}/glade/jackbeat.ui
 81 %{_datadir}/%{name}/pixmaps/knob.png
 82 %{_datadir}/%{name}/pixmaps/jackbeat_logo.png
 83 %{_datadir}/applications/%{name}.desktop
 84 %{_datadir}/pixmaps/%{name}.png

Almost, you must also include explicitly
%dir %{_datadir}/%{name}

OK - thanks again. Done:

SRPM: 
https://docs.google.com/leaf?id=0B5QCFlQBF-VzNWU1NjMyOWQtZDI0ZC00NmY1LTg0ZDItNzQ0YzZiM2Q2Njhh&hl=en&authkey=CKOY0IUE
SPEC:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzNGFmMmU4YjUtYzIwYS00OTUwLThjZmYtZjYwYWZmOTliZjYy&hl=en&authkey=CJaJ0t8I

Few comments : 

- The icon png could be installed with one line rather than 2 :
  Replace
  %{__install} -m 755 -d %{buildroot}/%{_datadir}/pixmaps
  %{__install} -m 644 %{SOURCE1} %{buildroot}/%{_datadir}/pixmaps/
  with 
  %{__install} -Dp -m 644 %{SOURCE1} %{buildroot}/%{_datadir}/pixmaps/%{name}.png

- if you remove all of %{buildroot}/%{_datadir}/%{name}/help no need to remove %{buildroot}/%{_datadir}/%{name}/help/COPYING first. Also, just a wild guess, make sure removing this file doesn't break an About box or something like that in the software.

- The %files section could be much simpler and thus more readable. You want to own everything in %{_datadir}/%{name} so just use that and remove all others line starting with %{_datadir}/%{name}. %dir needs to be used only when you want to own a directory but not what's inside it. Here's how the %files section should look like :
%files 
%defattr(-,root,root,-)
%doc AUTHORS ChangeLog COPYING README NEWS
%{_bindir}/%{name}
%{_datadir}/%{name}
%{_datadir}/applications/%{name}.desktop
%{_datadir}/pixmaps/%{name}.png

Also, please don't forget to bump the release and add a changelog entry every time you make changes to the spec file, including during review.

Thanks Xavier - but do I really need to bump the package number before it has hit bodhi? Isn't that a little misleading? I guess not if the changelog documents all the changes in the reveiw ...

OK - will do. I'm going to move this to gitorius as sharing as a google document is tiresome. Appreciate the feedback nonetheless!

jackbeat-0.7.6-2.fc14.src.rpm:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzODZjMDgwMjQtYjUzZC00MGI1LWEwMmItZWJlNmMxMDNjOTFm&hl=en&authkey=CN2m4tcI
jackbeat.spec:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzODFiMjNiNzUtN2MxMy00ZGRjLTk3NjMtNTc0ZTAxZjU5ODBl&hl=en&authkey=COzn_dsN

Changes as suggested by Xavier
It looks like the COPYING file is needed by the license button in the about box. Well spotted! 

localhost:~/rpmbuild/SRPMS$ rpmlint jackbeat-0.7.6-2.fc14.src.rpm 
jackbeat.src: W: spelling-error %description -l en_US resized -> resined, resided, re sized
jackbeat.src: W: spelling-error %description -l en_US jackeq -> jacket, jacked, jack eq
jackbeat.src: W: spelling-error %description -l en_US olivier -> Olivier
jackbeat.src: W: spelling-error %description -l en_US xung -> sung, rung, lung

1 packages and 0 specfiles checked; 0 errors, 4 warnings.

A couple quick points from me too before we do a full review:

* You don't need BuildRequires (BR) to autoconf and zlib-devel

* You need to BR libxml2-devel and libsamplerate-devel
It's always to build your package in mock to test whether you are missing any BRs.

- You mix up two macro conventions, which is not recommended. For example, if you are using %{__make}, %{__rm} etc, you want to use %{__sed}. Conversely, if you use sed, then you want to use make and rm. I prefer the latter convention but that's just me.

! Please make the %description span 80 columns as much as possible. Currently it is set to 70 columns max.

! At Fedora, we don't put Author information in %description

! It would be good to explain in the spec file as comments where additional patches and sources come from and/or what they are for. The .desktop file can probably be sent upstream. I guess you derived the .png file from pkgdata/pixmaps/jackbeat_logo.png . Can you do a higher resolution one, so this looks nicer in the desktop menu?

* It would be good if you make the .desktop file a little nicer. Here are a few suggestions:
   - Add a "Comment:". Remember that Gnome uses "Comment" whereas KDE uses "GenericName". No need to repeat the application name in the "Comment" or "GenericName".
   - You need to add the category AudioVideo; I think this will be mandatory soon.
   - If you add a X-Jack; category, the application will nicely coexist in our Jack menu with other Jack applications. (did you install the multimedia-menus package?)


Thanks for packaging this!

Brendan, I'm sorry if my comment may have been a little bit confusing. If package a depends on b then there is no need to install b separately, a hast to be installed, of course.
Then just a little comment on your changelog, you should really be a little bit more precise, line numbers _might_ change over the years. ;-)

Thanks guys.

Great tip Orcan - Upstream seems responsive to including the files in the source tarball next version bump and was able to provide me with an SVG icon present from his SVN repository. Can't do anything about its 'chunkiness' I'm afraid - looks the same at hi resolution - I believe that's the authors intention. He also tipped me the MimeType of the app which I had missed in the desktop file.

* Wed Nov 05 2010 Brendan Jones <brendan.jones.it> 0.7.6-3
- removed unnecessary BuildRequires, made use of consistent macros
- commented extra sources
- icon replaced with svg icon from upstream
- Expanded Categories, Comment and added MimeType=application/jab to .desktop

SRPM:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzMjc1ZGE4ZWUtOTBjZS00ZThmLTgyZmQtZTJjOTU0NGFiZDgy&hl=en&authkey=CKP6lLEC
SPEC:
https://docs.google.com/leaf?id=0B5QCFlQBF-VzMTZjZTA5MGQtNGJmMi00ZjQxLTk5ZjctYmJiNTNkZTFiNWQ3&hl=en&authkey=CO28kPUD

I started the review on this package. It needs a little bit of work:

- rpmlint says:
   jackbeat.x86_64: W: spelling-error %description -l en_US resized -> resined, resided, re sized
   jackbeat.x86_64: W: spelling-error %description -l en_US jackeq -> jacket, jacked, jack eq
   jackbeat.x86_64: W: no-manual-page-for-binary jackbeat
These can be ignored.

- koji rawhide build is fine
   http://koji.fedoraproject.org/koji/taskinfo?taskID=2594483

* src/core/pa_ringbuffer* are licensed MIT, src/gui/phat/phatrange* are licensed LPGLv2+. The rest is GPLv2+
Thus the license tag should be "GPLv2+ and MIT and LGPLv2+". See
   http://fedoraproject.org/wiki/Licensing#Good_Licenses

* Macro consistency: You use %{buildroot} ${RPM_BUILD_ROOT} and $RPM_BUILD_ROOT all at once. Stick to one.

* Scriptlets are not use properly. See
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
especially the "desktop-database" section.

! It would be better if you install the icon in /usr/share/icons/hicolor/scalable/apps/ . This is not a requirement though. /usr/share/pixmaps is used usually by old stuff. Note that if you do this, you change this then you need to add the necessary scriptlets. See "icon cache" section in the above link. Also you would need to Require: hicolor-icon-theme

* MimeType entry in the .desktop file should end with a semicolon. You can validate your desktop file by running
   $ desktop-file-validate jackbeat.desktop

* One problem that I noticed with this software is, it doesn't stay connected with jack. it kills jackd sometimes. Do you see this behavior?


I see that you started helping other people with packaging and reviewing, which is very good (We usually ask the potential contributors to do unofficial reviews to test their ability to follow the guidelines). By the way, could you tell me your FAS username?

Thanks Orcan.

My FAS is bsjones.

I have made the changes you requested and have reposted them here:

http://bsjones.fedorapeople.org/jackbeat-0.7.6-4.fc14.src.rpm
and
http://bsjones.fedorapeople.org/jackbeat.spec

I haven't noticed an issue with jack but will try and reproduce. That's concerning.

thanks again for your help.

Brendan





-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Add require for hicolor-icon-theme

http://bsjones.fedorapeople.org/jackbeat-0.7.6-5.fc14.src.rpm
and
http://bsjones.fedorapeople.org/jackbeat.spec






-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Orcan: I'm still yet to reproduce your issue, however I have found others (with regards to pulseaduio) and I'm not comfortable including this in Fedora for now without significant patches. I will liaise with upstream because I think this simple app has potential (its simple for the novice and consumes next to no screen real estate). In the meantime can you post me your .jackdrc - I will close after and file another request when/if these issues have been resolved.

I am still committed to helping you co-maintain other packages in the Audio spin and will contact you outside of this bug. 



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

As you requested:
    $ cat ~/.jackdrc 
    /usr/bin/jackd -v -P20 -dalsa -dhw:0 -r48000 -p1024 -n2 -Xseq
I tried a few other combinations but it just doesn't work.

I see the line
    Jack: jack_client_open PortAudio
in the program output about 20 lines before it shuts jack. This is a similar symptom I observed when I built fluidsynth with portaudio support (Lots of XRUNS leading jackd to shutdown or crash after 2-3 seconds). For this reason, we disabled the fluidsynth's portaudio support in the Fedora package. Over the time I saw a couple other people from other distros report the same issue.

Similarly, I disabled the portaudio support of jackbeat. There is no very easy way to do it. I hacked on the source. After that, I compiled and, guess what, jackbeat works properly. This is really strange. With the unhacked jackbeat, even when I use the -c Jack option, it still tries to interact with portaudio. I wonder why this happens.

I understand that you don't want this package to be in Fedora yet. Maybe you can ask upstream to make portaudio and pulseaudio support optional for this software, and then we can compile it for jack only.


Otherwise the package is in perfect condition, normally I would just go ahead and sponsor you. :(

Let me know when you submit another package. I will be busy for the next couple months, but I will definitely make some time. And I appreciate your offer for helping with Audio production packages. I really need it.

Thanks for your efforts Orcan! I will leave open for the moment and liase with upstream with regards to making pulseaudio and portaudio support optional.

However, I am yet to make jack crash! I have made jackbeat crash by pulling jackd out from under it - it occasionally copes in a less than graceful manner.

I tested using your jackd settings on my rt kernel here, and again adjusted for current stock f14 kernel with a number of zyn's + guitarix with up to three open running jackbeat windows - yet still no crash. Anything you are doing that I may not be?

Pulseaudio seems to get in the way for me when using jack however - I had to kill pulse outright for it to connect to jack - pasuspender was no use here. Other pulse deadlock issues have been reported in other distributions as well (according to the ChangeLog).

Look I won't give up here entirely - hacking around with this source code is a fruitful enough exercise in any case. Also, I'll be keeping an eye on the audio bugs as they come in and any upstream updates to see where I might be able to help out. Thanks again







-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Closing for the time being



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers