Bug 191239

not a full review yet but some points you need to improve:

- Package does not follow Fedora's package naming guildlines
  (wiki: PackageNamingGuidelines)
  release should be 1, 2, 3, not 2.0
- BuildRequires: desktop-file-utils is missing

%define	desktop_vendor planetccrma
- desktop_vendor is fedora :)
- add to desktop-file-install: --add-category X-Fedora

# distros with 2.4.x kernels should use jackstart as the default
%{?fc1:%define usejackstart 1}
%{?rh9:%define usejackstart 1}
- this define can go since extras doesn't go that far back

-just use rm istead of %__rm macro

> Package does not follow Fedora's package naming guildlines
> (wiki: PackageNamingGuidelines)
> release should be 1, 2, 3, not 2.0

Fixed (I was trying to not needlessly increment the release during the review
process when packages have not yet been released). 

> BuildRequires: desktop-file-utils is missing

Fixed, my mach based build environment already includes it by default and that's
why I did not notice it missing.

> %define	desktop_vendor planetccrma
> desktop_vendor is fedora :)
> add to desktop-file-install: --add-category X-Fedora

Added.

> # distros with 2.4.x kernels should use jackstart as the default
> %{?fc1:%define usejackstart 1}
> %{?rh9:%define usejackstart 1}
> this define can go since extras doesn't go that far back

I erased this but I was hoping I would not have to keep separate spec files for
Planet CCRMA and Fedora Extras (it does not really hurt to have it there). Oh
well, just a little bit more extra pain I guess...

> just use rm istead of %__rm macro

Fixed. Is this a general policy (ie: don't even think of using macros for
commands) or personal preference?

Spec URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl.spec
SRPM URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl-0.2.20-3.src.rpm

(In reply to comment #2)
> > just use rm istead of %__rm macro
> 
> Fixed. Is this a general policy (ie: don't even think of using macros for
> commands) or personal preference?

It's personal preference. I personally consider this change to be a regression
since "%__rm" expends to a fully-qualified pathname, unlike plain "rm". Hence
the rpm build may now behave differently depending on the setting of the
builder's PATH variable (they might have a wrapper script for rm earlier in
their PATH for example, which always asked for confirmation of any deletes),
which is not a good thing.

(In reply to comment #3)
> It's personal preference. I personally consider this change to be a regression
> since "%__rm" expends to a fully-qualified pathname, unlike plain "rm". 

What about %{__make}?  I've been asked to change that to "make" in some reviews.

Fernando - I think people also like to see:
Source0: http://dl.sourceforge.net/qjackctl/qjackctl-0.2.20.tar.gz
instead of 
Source0: http://dl.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
since it is directly wget'able, although I don't know if it's 
official policy or not.

AG

(In reply to comment #4)
> (In reply to comment #3)
> > It's personal preference. I personally consider this change to be a regression
> > since "%__rm" expends to a fully-qualified pathname, unlike plain "rm". 
> 
> What about %{__make}?  I've been asked to change that to "make" in some reviews.

My personal preference is to use the macros where they are available, and
fully-qualified pathnames otherwise, for all commands used in the build process.

> Fernando - I think people also like to see:
> Source0: http://dl.sourceforge.net/qjackctl/qjackctl-0.2.20.tar.gz
> instead of 
> Source0: http://dl.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
> since it is directly wget'able, although I don't know if it's 
> official policy or not.

My preference is:
Source0: http://dl.sf.net/qjackctl/qjackctl-%{version}.tar.gz

Reasons:
* dl.sf.net is a shorter URL :-)
* using the package name explicitly rather than just %{name} is a little more
readable, and the package name is hardly likely to change very often
* the version number is likely to change, so using the %{version} macro helps
maintainability of the package
* whilst the use of the %{version} macro prevents wget from working directly,
you can use spectool from the fedora-rpmdevtools package to retrieve the source
for you - it will automatically expand the macro

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > It's personal preference. I personally consider this change to be a regression
> > > since "%__rm" expends to a fully-qualified pathname, unlike plain "rm". 
> > 
> > What about %{__make}?  I've been asked to change that to "make" in some reviews.
> 
> My personal preference is to use the macros where they are available, and
> fully-qualified pathnames otherwise, for all commands used in the build process.

I agree and that's why I use, when possible, built-in macros.

[I thought it was policy.]

I'm sure this has been discussed to death here before :-)

My take on this: if the results of running a build script depend on overriding a
basic core unix command by the script builder then I'd label that a bug in the
build script. While your example for rm sounds reasonable, it is not in the
context of executing a build script which, I think, cannot/should not be
interactive. Using the macro would actually expose (IMHO) a bug. I'm sure other
more applicable examples could be put forward, of course...

What I don't see is why %{__rm} should be different from any other unix command.
It is just a unix command... 

(I would prefer to keep using macros when possible. I understand this is
probably not so important these days, all builds are done in chroot environments
where they are completely isolated.)

> > Fernando - I think people also like to see:
> My preference is:
> Source0: http://dl.sf.net/qjackctl/qjackctl-%{version}.tar.gz

Sounds good to me, I agree the version number should not be hardwired, otherwise
what's the use of macros? :-)

I'll wait a bit before releasing another version in case there are more changes.

My preference is to only use macros if there's a solid gain to be had. As in,
its something that's expected to change (versions) or it handles something ugly
for you (%configure). I don't see any real gain in using macros such as
%{_make}. And most of all, the spec templates do not use these macros, which
implies to me they should not be used.

It would be nice to have an explicit policy up on the wiki for this, even if
that policy is "Use whatever, just be consistent", much like the %{buildroot} vs
$RPM_BUILD_ROOT policy.

(In reply to comment #6)
> I agree and that's why I use, when possible, built-in macros.
> 
> [I thought it was policy.]

It's not policy, unless the packaging guidelines have changed since I last read
them.

> My take on this: if the results of running a build script depend on overriding a
> basic core unix command by the script builder then I'd label that a bug in the
> build script. While your example for rm sounds reasonable, it is not in the
> context of executing a build script which, I think, cannot/should not be
> interactive. Using the macro would actually expose (IMHO) a bug. I'm sure other
> more applicable examples could be put forward, of course...

I don't understand what you're getting at here. The example I gave was that
someone rebuilding a package had a local "rm" script, earlier in their PATH than
/bin/rm, that prompted for confirmation. If this person rebuilt a package that
used plain "rm", the build would become "interactive" (which is bad), but if
they rebuilt a package that used "%{__rm}" instead, the build would be
non-interactive as it's supposed to be (since %{__rm} would expand to /bin/rm
and hence their local rm script wouldn't be used). Using the macro is hence an
advantage.

(In reply to comment #7)
> My preference is to only use macros if there's a solid gain to be had. As in,
> its something that's expected to change (versions) or it handles something ugly
> for you (%configure). I don't see any real gain in using macros such as
> %{_make}.

I think reproducability of builds is a solid gain.

> And most of all, the spec templates do not use these macros, which
> implies to me they should not be used.

Or perhaps the templates should be updated?

> It would be nice to have an explicit policy up on the wiki for this, even if
> that policy is "Use whatever, just be consistent", much like the %{buildroot} vs
> $RPM_BUILD_ROOT policy.

+1

(In reply to comment #8)
> (In reply to comment #6)
> > I agree and that's why I use, when possible, built-in macros.
> > 
> > [I thought it was policy.]
> 
> It's not policy, unless the packaging guidelines have changed since I last read
> them.
> 
> > My take on this: if the results of running a build script depend on overriding a
> > basic core unix command by the script builder then I'd label that a bug in the
> > build script. While your example for rm sounds reasonable, it is not in the
> > context of executing a build script which, I think, cannot/should not be
> > interactive. Using the macro would actually expose (IMHO) a bug. I'm sure other
> > more applicable examples could be put forward, of course...
> 
> I don't understand what you're getting at here. The example I gave was that
> someone rebuilding a package had a local "rm" script, earlier in their PATH than
> /bin/rm, that prompted for confirmation. If this person rebuilt a package that
> used plain "rm", the build would become "interactive" (which is bad), but if
> they rebuilt a package that used "%{__rm}" instead, the build would be
> non-interactive as it's supposed to be (since %{__rm} would expand to /bin/rm
> and hence their local rm script wouldn't be used). Using the macro is hence an
> advantage.

Arghhh, sorry, just a misunderstanding on my part. For some reason I though that
the "regression" you were mentioning was to use %{__rm}, not to take it out.
Sorry. We are in agreement about the macros. 

Seems to me if you're worried about reproducible builds, really you should be
using a controlled chroot environment. (mock)

Anyway, I just reviewed jack. I'm going to work on Qjackcontrol today.

MUST items:

- rpmlint: Clean
- Package name: Ok
- Spec name: Ok
- Meets packaging guidelines: NEEDSWORK
- License: Ok
- Spec in American English: Ok
- Spec legible: Ok
- Sources match upstream: Ok
- Builds: Ok
- BuildRequires: Ok
- Locales: Ok
- ldconfig: Ok
- Relocation: Ok
- Directory ownership: Ok
- %files: Ok
- %clean: Ok
- Macros: Ok
- Code vs. Content: Ok
- Documentation: Ok
- devel package: Ok
- .desktop file: Ok

SHOULD:

- Includes license text: Ok
- Mock build: Ok
- Builds on all archs: Built on i386, x86_64
- Package functional: Tested on i386, x86_64
- Scriptlets: Ok
- Subpackages: Ok

NEEDSWORK:

Source: seems to need to be http://dl.sf.net/sourceforge/qjackctl/[...] 
otherwise I get a 404 as is.

The Requires: jack-audio-connection-kit >= 0.80.0  is unnecessary as no such old
version is going to be in Extras.

The generic INSTALL instructions should not be packaged.

The desktop file should be included as Source:, not embedded in the spec, as
shown in the Desktop Files section of the packaging guidelines.

(In reply to comment #10)
> Seems to me if you're worried about reproducible builds, really you should be
> using a controlled chroot environment. (mock)

Reproducability of builds is about it working equally well for someone
downloading the SRPM to their desktop system and building it there as it does
for someone using a clean buildsystem like mock.

(In reply to comment #11)
> NEEDSWORK:
> 
> Source: seems to need to be http://dl.sf.net/sourceforge/qjackctl/[...] 
> otherwise I get a 404 as is.

Fixed.

> The Requires: jack-audio-connection-kit >= 0.80.0  is unnecessary as no such old
> version is going to be in Extras.

Fixed (comment: Extras does not live in a vaccum, the requires line would have
addressed Planet CCRMA users that have old versions and migrate to the extras
package - I know, very unlikely, but if experience is any guide if it can happen
it will).

> The generic INSTALL instructions should not be packaged.
> 
> The desktop file should be included as Source:, not embedded in the spec, as
> shown in the Desktop Files section of the packaging guidelines.

Fixed.

Spec URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl.spec
SRPM URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl-0.2.20-4.0.src.rpm

Looks good. I can't give final approval, but this is approve-able in my opinion.
Someone please sponsor Fernando, we need him. :)

And for what its worth, I queried FESCo on the make/rm macro thing. The response
was "choose either, just be consistent".

Hi,

While browsing the wiki I "stumbled" over the "Music and Media Production" SIG,
Since I've always thought that it would be a great idea to integrate planetccrma
and FE I must say that I'm very happy to see this is happening now. I must say
though that I myself sofar have had no need for planetccrma packages.

Well thats enough introduction I guess. The reason I'm introducing myself is
because after stumbling over the SIG I decided to read the fedora-music-list
archives. There I read that you (Fernando) need a sponsor and I can sponsor.

So I'll (quickly) redo Callum's Review (the rules say I must do so as a sponsor)
and if I agree that this package is approve-able you can create an account in
the accounts system and I'll sponsor you.

Some remarks about the above discussion:

(In reply to comment #2)
> > # distros with 2.4.x kernels should use jackstart as the default
> > %{?fc1:%define usejackstart 1}
> > %{?rh9:%define usejackstart 1}
> > this define can go since extras doesn't go that far back
> 
> I erased this but I was hoping I would not have to keep separate spec files for
> Planet CCRMA and Fedora Extras (it does not really hurt to have it there). Oh
> well, just a little bit more extra pain I guess...
> 

You can keep a single specfile if you want, as long as things don't become too
ugly / too much of a kludge. I personally believe that the above 2 lines arenot
too ugly.

(In reply to comment #14)
> And for what its worth, I queried FESCo on the make/rm macro thing. The response
> was "choose either, just be consistent".

Yes, thats what I've always understood (and used as a criteria during reviews) too.

Unfortunatly I cannot do a proper review because the srpm link gives a 404 error,
here are some initial MUST fix items from looking at the spec-file:
* %makeinstall is broken (yeah I know, someone should fix it) please use:
  make install DESTDIR=$RPM_BUILD_ROOT instead. (Or make that %{__make} ......)
* Under %files I see %{_datadir}/icons/qjackctl.png, that is not according to the
  freedesktop.org icon standard, it should go under:
  %{_datadir}/icons/hicolor/32x32/apps
  Where 32x32 is the size of the icon, please do ls /usr/share/icons/hicolor/
  to see the available valid sizes, if the icon doesn't match any pick the 
  closest.
* Once the icon is in the proper case you must add %post(un) script to update 
  the icon-cache see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d

I've got the srpm now, all I needed todo was change the 4.0 release in the URL
to just 4 .

MUST:
=====
* rpmlint output is clean
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-i386
* BR: ok
* No locales
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* .desktop file present and properly installed


MUST fix:
=========
* %makeinstall is broken (yeah I know, someone should fix it) please use:
  make install DESTDIR=$RPM_BUILD_ROOT instead. (Or make that %{__make} ......)
* Under %files I see %{_datadir}/icons/qjackctl.png, that is not according to the
  freedesktop.org icon standard, it should go under:
  %{_datadir}/icons/hicolor/32x32/apps
  Where 32x32 is the size of the icon, please do ls /usr/share/icons/hicolor/
  to see the available valid sizes, if the icon doesn't match any pick the 
  closest.
* Once the icon is in the proper case you must add %post(un) script to update 
  the icon-cache see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d

(In reply to comment #18)
> * %makeinstall is broken (yeah I know, someone should fix it) please use:
>   make install DESTDIR=$RPM_BUILD_ROOT instead. (Or make that %{__make} ......)

Fixed (in which way is makeinstall broken?, I used to not use it and then saw
spec files using it and changed over when it works - I know not all packages
will use the options it uses, is that the broken behavior?).

> * Under %files I see %{_datadir}/icons/qjackctl.png, that is not according to the
>   freedesktop.org icon standard, it should go under:
>   %{_datadir}/icons/hicolor/32x32/apps
>   Where 32x32 is the size of the icon, please do ls /usr/share/icons/hicolor/
>   to see the available valid sizes, if the icon doesn't match any pick the 
>   closest.
> * Once the icon is in the proper case you must add %post(un) script to update 
>   the icon-cache see:
>
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d

Fixed. 

Spec URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl.spec
SRPM URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl-0.2.20-5.src.rpm

(sorry about the broken link before...)
(In reply to comment #15)
> Well thats enough introduction I guess. The reason I'm introducing myself is
> because after stumbling over the SIG I decided to read the fedora-music-list
> archives. There I read that you (Fernando) need a sponsor and I can sponsor.

Thank you! Much appreciated...

A quick question. In my specs I usually include the desktop entry inline with a
cat <<EOF, I changed that on request as the guidelines require the desktop entry
to be a separate source file. Has anyone ever pointed out that that makes it
more difficult to include a full path to the executable that will automatically
reflect whatever is in %{_prefix}? I think it is important that what I package
executes what I package and not whatever is in the path that happens to match
the executable name. Right now I'm just including "Exec=qjackctl" in the desktop
file instead of what I used to do which was "Exec=%{_bindir}/qjackctl". I could
obviously hack a "perl -p -i -e" inline script to replace a placeholder with the
real %{_bindir} but at that point I like it better inline :-)

(In reply to comment #19)
> Fixed (in which way is makeinstall broken?, I used to not use it and then saw
> spec files using it and changed over when it works - I know not all packages
> will use the options it uses, is that the broken behavior?).
> 

There currently is some interesting discussion on f-e-l (fedora-extras-list
mailinglist) just search for DESTDIR in the archives, btw you really should
subscribe ot f-e-l.

> Spec URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl.spec
> SRPM URL: http://ccrma.stanford.edu/planetccrma/extras/qjackctl-0.2.20-5.src.rpm
> 
Looks good -> Approved!

> (sorry about the broken link before...)
No problem.


> A quick question. In my specs I usually include the desktop entry inline with a
> cat <<EOF, I changed that on request as the guidelines require the desktop entry
> to be a separate source file. Has anyone ever pointed out that that makes it
> more difficult to include a full path to the executable that will automatically
> reflect whatever is in %{_prefix}? I think it is important that what I package
> executes what I package and not whatever is in the path that happens to match
> the executable name. Right now I'm just including "Exec=qjackctl" in the desktop
> file instead of what I used to do which was "Exec=%{_bindir}/qjackctl". I could
> obviously hack a "perl -p -i -e" inline script to replace a placeholder with the
> real %{_bindir} but at that point I like it better inline :-)

Erm, I've never though about this before. Everybody uses just the command name
without a full path in the .desktop files without any problems. If there are 2
identically named binaries in different places in the path then that really is a
bug. If you would like to discuss this further please do so on f-e-l, I don't
feek further discussion belongs in this BZ ticket.

Ping?

It has been 4 weeks since I approved this package and said I would sponsor you,
all you have todo is create an account, get sponsored (just wait) and import
this. Why the delay?

(In reply to comment #21)
> Ping?

Pong...

> It has been 4 weeks since I approved this package and said I would sponsor you,
> all you have todo is create an account, get sponsored (just wait) and import
> this. Why the delay?

I've been terribly busy (still are)...
I think I've done everything that's needed and should be "sponsorable" now. 
Thanks for your patience...

Ok, you've been sponsored, so you should be able to import this shortly (donnu
howlong the infrastructure takes to work through this).

Package Change Request
======================
Package Name: qjackctl
New Branches: EL-5 EL-6
Owners: dtimms

[requirement to support rakarrack on el5,6]

Have you asked if the current Fedora maintainer would like to maintain in EPEL?

Please do so, if they don't want to, or don't reply in a week we can make 
the EPEL branches.

"""
On Thu, Jul 8, 2010 at 6:04 PM, David Timms wrote:
> > On 08/07/10 09:31, Fernando Lopez-Lezcano wrote:
>> >> Â  Â  Â  On Mon, 2010-07-05 at 22:54 +1000, David Timms wrote:
>>> >>> Hi Fernando,
>>> >>>
>>> >>> I'm interested in getting the packages I look after sorted for the epel
>>> >>> repo. qjackctl, while not being a Requires of rakarrack, definitely
>>> >>> makes it easy to work with jack applications like rakarrack.
>>> >>>
>>> >>> Are you interested in creating/maintaining epel branches, or should I
>>> >>> make a cvs change request and build the current devel version for that ?
>>> >>>
>>> >>> Also, I could look at grabbing those cvs fixes mentioned on
>>> >>> music@fedoraproject, unless you are already working/testing that ?
>> >>
>> >> Hi David... hmmm, probably you should contact Orcan as he is the current
>> >> maintainer of the Fedora qjackctl packages (he has been migrating a log
>> >> of packages from Planet CCRMA to Fedora). I'm sure he'll be happy to
>> >> help:
> > Hi Oget, what's your feeling toward epel ?
> >
> > I want build some audio bits for epel, but they have requirements like
> > this, thast aren't already branched for epel, so I'm willing to add them:
> > https://bugzilla.redhat.com/show_bug.cgi?id=191239
> >
Hi David,
I am currently maintaining a considerably large number of packages,
both audio related and unrelated, in Fedora and RPMFusion. I don't
think I have time to get into EPEL. Also I don't have much interest in
EPEL. You can ask for a cvs request in the above bug for EPEL, if you
want to maintain it.

By the way,  you can also ask for pkgdb requests in Fedora branches.
https://admin.fedoraproject.org/pkgdb/acls/name/qjackctl

I'll approve them, as having comaintainers is always good :)

Orcan
"""

Accordingly, can the CVS request https://bugzilla.redhat.com/show_bug.cgi?id=191239#c24 by actioned, please ?

CVS done (by process-cvs-requests.py).