Bug 199029 - Review Request: jokosher
Review Request: jokosher
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Gordon
Fedora Package Reviews List
:
Depends On: 205575
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-15 21:01 EDT by Christopher Brown
Modified: 2016-08-14 12:28 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-28 19:23:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
peter: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
Rework omf file installation (2.71 KB, patch)
2007-03-05 15:33 EST, Toshio Kuratomi
no flags Details | Diff

  None (edit)
Description Christopher Brown 2006-07-15 21:01:49 EDT
Spec URL: http://www.iammetal.co.uk/jokosher.spec
SRPM URL: http://www.iammetal.co.uk/jokosher-0.1-1.src.rpm
Description: Jokosher is a multi-track non-linear audio editor with a prime focus on usability. It is developed in Python and is Open Source.

The main goals of Jokosher functionality are:

    * The ability to record from any ALSA sound card, including multi-input cards such as the M-Audio Delta 44.
    * Non-destructive editing.
    * Undo/Redo (at least to a reasonable limit, but preferably unlimited).
    * Be able to edit the volume curve in different parts of the track.
    * Resizable track views to easily zoom in and out of a waveform.
    * Support for effects plug-ins, most notably LADSPA.
    * Be able to apply effects to an entire track or a selected portion of a track. When applying effects, there should be the ability to preview the sound with the effect before it is applied.
    * It should be able to master to OGG, MP3 and WAV.
    * It should be able to import OGG, MP3 and WAV.
    * Most important, it should be easy and intuitive to use.
Comment 1 Peter Gordon 2006-07-16 19:11:04 EDT
According to Yum, gnonlin is not in Core, Updates, or Extras, so this package -
with regards to Extras - would not be self-supporting in that it would need a
dependency external to the default repositories. Perhaps you'd also want to
submit Gnonlin for inclusion into Extras as well?

Also, you should not use the Packager: tag in your spec. See:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-c17fb8c1ce9be40da720a2b25d1e2a241062038f

Thirdly, your package would probably break on 64-bit systems, as you hardcode
/lib/ into the path of %_pythondir. Please use a macro for this instead, such as
%{_lib} (or %{_libdir}, etc. as needed). 

Forth, you really should be using %{python_sitedir} and following the %files
section correctly as mentioned in Packaging/Python on the Wiki. See:
http://fedoraproject.org/wiki/Packaging/Python

Lastly, in the %changelog, you simply have a user@host identifier. I'm not
certain, but I believe that this is supposed to be a full and valid email
address (such as "me@my-isp.com" or something similar).
Comment 2 Christopher Brown 2006-07-17 03:45:06 EDT
(In reply to comment #1)
> According to Yum, gnonlin is not in Core, Updates, or Extras, so this package -
> with regards to Extras - would not be self-supporting in that it would need a
> dependency external to the default repositories. Perhaps you'd also want to
> submit Gnonlin for inclusion into Extras as well?

Gnonlin is indeed in extras.

> 
> Also, you should not use the Packager: tag in your spec. See:
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-c17fb8c1ce9be40da720a2b25d1e2a241062038f
> 
> Thirdly, your package would probably break on 64-bit systems, as you hardcode
> /lib/ into the path of %_pythondir. Please use a macro for this instead, such as
> %{_lib} (or %{_libdir}, etc. as needed). 
> 
> Forth, you really should be using %{python_sitedir} and following the %files
> section correctly as mentioned in Packaging/Python on the Wiki. See:
> http://fedoraproject.org/wiki/Packaging/Python
> 
> Lastly, in the %changelog, you simply have a user@host identifier. I'm not
> certain, but I believe that this is supposed to be a full and valid email
> address (such as "me@my-isp.com" or something similar).

Thank you for these. I shall update and resubmit.

Regards
Chris
Comment 3 Christopher Brown 2006-07-17 17:55:25 EDT
(In reply to comment #1)

> Also, you should not use the Packager: tag in your spec. See:
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-c17fb8c1ce9be40da720a2b25d1e2a241062038f
> 

Removed.

> Thirdly, your package would probably break on 64-bit systems, as you hardcode
> /lib/ into the path of %_pythondir. Please use a macro for this instead, such as
> %{_lib} (or %{_libdir}, etc. as needed).

Added, see below.

> 
> Forth, you really should be using %{python_sitedir} and following the %files
> section correctly as mentioned in Packaging/Python on the Wiki. See:
> http://fedoraproject.org/wiki/Packaging/Python

Done. This would appear to be one and the same with your third point.

> 
> Lastly, in the %changelog, you simply have a user@host identifier. I'm not
> certain, but I believe that this is supposed to be a full and valid email
> address (such as "me@my-isp.com" or something similar).

Done.

Thanks again for the pointers. Could you please review again and let me know if
there are any more blockers.

Regards
Chris
Comment 4 Peter Gordon 2006-07-17 20:07:16 EDT
(In reply to comment #2)
> Gnonlin is indeed in extras.

Sorry about that. A `yum clean all` followed by a `yum info gnonlin` shows that
it is indeed in Extras.

On a related note, please bump the release whenever you significantly change the
spec during the review or in CVS (once it's APPROVED). Is the resubmission
located at the same URIs?
Comment 5 Christopher Brown 2006-07-18 03:56:32 EDT
(In reply to comment #4)

> Is the resubmission located at the same URIs?

Yes, it is.

Regards
Chris
Comment 6 Christopher Brown 2006-07-18 19:55:36 EDT
I have now updated the files following the release of jokosher 0.1

These are at:

http://www.iammetal.co.uk/jokosher.spec
http://www.iammetal.co.uk/jokosher-0.1-2.src.rpm

Regards
Chris
Comment 7 Christopher Brown 2006-07-20 13:22:54 EDT
I have now read through all steps in the packaging guidelines on the Wiki. I
would therefore be grateful if someone could sponsor this package as it is my
first attempt at Packaging and Extras submission.

Regards
Chris
Comment 8 Paul F. Johnson 2006-07-27 10:59:44 EDT
Bad : you need to define BuildArch : noarch

rpmlint on the rpm gives

E: script-without-shellbang
/usr/share/doc/jokosher-0.1/userguide/jokosheruserguide.de.html

rpmlint on the debuginfo gives

E: empty-debuginfo-package

I'm currently running mock over the package.

You need to fix the above problems before you go any further as they're blockers.
Comment 9 Paul F. Johnson 2006-07-27 11:12:09 EDT
Fails to build in mock. The problem is in the Source line - you have
%{name}-%{version}-%{release}.tar.gz

%{release} gets expanded to -2.fc6

Typically, the source tarball (not the SRPM you've produced) will be along the
lines of

%{name}-%{version}.tar.gz (or sometimes with something after %{version} such as
-rc2.tar.gz). This is what Source has to point to.

If you've had to create a tarball from the SVN repository, the package name
needs to reflect this.
Comment 10 Paul F. Johnson 2006-07-27 11:30:35 EDT
You're missing some requires as well - check http://www.jokosher.org/download
for details.
Comment 11 Michael Schwendt 2006-07-27 11:31:46 EDT
Mid-air collision, so here are some additional comments:

* I suggest that upstream do not modify/extend/sublicence the GPL with
exceptions, but rather put their distribution licence terms into a separate file.

* Run rpmlint on the binary rpm. From the two errors it reports, one is valid.

* Directory /usr/lib/python2.4/site-packages/jokosher/ is not included.

* Why is it arch-specific and not "BuildArch: noarch"?

* Upstream recommends GStreamer 0.10.9 or above and prefers a CVS snapshot.
Requires: gstreamer >= 0.10.8 does not reflect that.

* Prefer "install -p" over "cp" to preserve time-stamps of files.

* Run-time warning:

Some functionality will not work correctly or at all.

You must have the Python alsaaudio package installed.
Please install python-alsaaudio or fetch from http://www.wilstrup.net/pyalsaaudio/.

* Crashes reproducibly with below message. Steps to reproduce:

1) Start "jokosher"
2) Enter "Preferences".
3) Click "Close".
4) Click "Create a new project".

Starting up
/usr/lib/python2.4/site-packages/jokosher/Jokosher.py:1054:
GtkDeprecationWarning: gtk.threads_init is deprecated, use gtk.gdk.threads_init
instead
  gtk.threads_init()
6783: assertion failed "allocator->lock == mutex" file "dbus-dataslot.c" line 82
function _dbus_data_slot_allocator_alloc
Aborted
Comment 12 Paul F. Johnson 2006-07-27 11:39:24 EDT
The more I look into this package, the more odd it becomes.

There is nothing (I can see) on the website as to the license
Upstream recommending a cvs snapshot (moving target problems)
Upstream not sticking totally to the GPL

Sure, it looks a nice package, but as it stands, is it stable (upstream) enough
to be in Extras?

The functionality issues in #11 give me serious reasons to doubt it's inclusion
at this time (though this could be down [again] to upstream or non-inclusion of
R's on the rpm itself)
Comment 13 Christopher Brown 2006-07-27 14:08:14 EDT
Okay, here goes,

First of all thanks for reviewing and for assigning yourself to this one Paul.
Most comments understood and have been or are being acted upon.

(In reply to comment #8)
> Bad : you need to define BuildArch : noarch

Done

> rpmlint on the rpm gives
> 
> E: script-without-shellbang
> /usr/share/doc/jokosher-0.1/userguide/jokosheruserguide.de.html

Fixed.

> 
> rpmlint on the debuginfo gives
> 
> E: empty-debuginfo-package

Do noarch packages generate debuginfo packages? rpmbuild no longer produces these.

Regards
Chris
Comment 14 Paul F. Johnson 2006-07-27 14:31:30 EDT
noarch doesn't create a debuginfo package.

Can you upload a new spec and src.rpm when you've fixed the other issues please?
Comment 15 Christopher Brown 2006-07-27 14:42:00 EDT
(In reply to comment #9)

> %{name}-%{version}.tar.gz (or sometimes with something after %{version} such as
> -rc2.tar.gz). This is what Source has to point to.

Fixed.

(In reply to comment #10)

> You're missing some requires as well - check http://www.jokosher.org/download
> for details.

Fixed. Alsaaudio is not a strict requirement for the operation of jokosher and
the next version will not require it at all. However I will consider packaging
it if necessary.

(In reply to comment #11)

* I suggest that upstream do not modify/extend/sublicence the GPL with
exceptions, but rather put their distribution licence terms into a separate
file.

I have mentioned this on the development list.

* Run rpmlint on the binary rpm. From the two errors it reports, one is valid.

Now fixed.

* Directory /usr/lib/python2.4/site-packages/jokosher/ is not included.

Please could you clarify.

* Why is it arch-specific and not "BuildArch: noarch"?

Now fixed.

* Upstream recommends GStreamer 0.10.9 or above and prefers a CVS snapshot.
Requires: gstreamer >= 0.10.8 does not reflect that.

Now fixed.

* Prefer "install -p" over "cp" to preserve time-stamps of files.

Now fixed - yes this is much better, thanks.

* Run-time warning:

Some functionality will not work correctly or at all.

You must have the Python alsaaudio package installed.
Please install python-alsaaudio or fetch from http://www.wilstrup.net/pyalsaaudio/.

See above.

* Crashes reproducibly with below message. Steps to reproduce:

1) Start "jokosher"
2) Enter "Preferences".
3) Click "Close".
4) Click "Create a new project".

Starting up
/usr/lib/python2.4/site-packages/jokosher/Jokosher.py:1054:
GtkDeprecationWarning: gtk.threads_init is deprecated, use gtk.gdk.threads_init
instead
  gtk.threads_init()
6783: assertion failed "allocator->lock == mutex" file "dbus-dataslot.c" line 82
function _dbus_data_slot_allocator_alloc
Aborted

I cannot reproduce this however I am running the latest gstreamer and gnonlin
releases although I fail to see how that would alter things. This has not been
reported on the support forums or lists to my knowledge. Perhaps the latest spec
file and requires fixes this. py-dbus maybe?

(In reply to comment #12)

> The more I look into this package, the more odd it becomes.

> There is nothing (I can see) on the website as to the license
> Upstream recommending a cvs snapshot (moving target problems)

Gstreamer have now made a release so no longer dependent on cvs. Gnonlin also
anticipated doing the same in the near future however again, current gnonlin
will work, just not with full features.

> Upstream not sticking totally to the GPL

This has been mentioned to the devs. I would appreciate comments on how much of
a showstopper this is and how to get around it of there is no altering the
stance of how it is licensed upstream.

> Sure, it looks a nice package, but as it stands, is it stable (upstream) enough
> to be in Extras?

I would say yes in the spirit of release early and often. This is not an
admittance of buggy software - the two main issues users currently experience
are due to current version of gstreamer and gnonlin, both of which Requires
indicates.

> The functionality issues in #11 give me serious reasons to doubt it's inclusion
> at this time (though this could be down [again] to upstream or non-inclusion of
> R's on the rpm itself)

I hope the above gives you cause to re-think. I would be happy to see it wait
until fc6 for inclusion. Thanks again for the advice.

Regards
Chris
Comment 16 Paul F. Johnson 2006-07-27 16:04:14 EDT
If the way the GPL is messed with adds restrictions, it causes major problems.
If it removes restrictions, that's fine. My reading is it adds.

The problem with release often and early is that you get releases which haven't
had the time to mature and be debugged correctly. I've known of many security
problems with that system.

"* Directory /usr/lib/python2.4/site-packages/jokosher/ is not included.

Please could you clarify."

The application creates a directory (via the define) in the
python/site-packages/ directory called jokosher. You need in your %files

%{python_dir}/site-packages/jokosher

"> You're missing some requires as well - check http://www.jokosher.org/download
> for details.

Fixed. Alsaaudio is not a strict requirement for the operation of jokosher and
the next version will not require it at all. However I will consider packaging
it if necessary."

Depending on the timeframe of this packages possible acceptance into FE, I'd
look into importing it. It won't harm.

I can also replicate the dbus error on my 64 and 32 bit systems. This will need
to be looked into.
Comment 17 Christopher Brown 2006-07-27 17:58:58 EDT
(In reply to comment #16)
> If the way the GPL is messed with adds restrictions, it causes major problems.
> If it removes restrictions, that's fine. My reading is it adds.

If the exceptions are placed into a separate file, would this resolve the
matter? My reading is that it permits people to use non-GPL compatible plugins
and therefore is not adding restrictions but IANAL.

> 
> The problem with release often and early is that you get releases which haven't
> had the time to mature and be debugged correctly. I've known of many security
> problems with that system.

The only comment I can make here is that this version has few known bugs and
most of those are outside of its control. One reason the package is being pushed
for inclusion in Extras is so that upstream can garner more feedback and improve
Q.A. which will surely help with debugging. As for security problems I really
cannot comment except to say this is an audio editor written in python using Gtk
widgets, not say, a driver with kernel hooks.

> 
> "* Directory /usr/lib/python2.4/site-packages/jokosher/ is not included.
> 
> Please could you clarify."
> 
> The application creates a directory (via the define) in the
> python/site-packages/ directory called jokosher. You need in your %files
> 
> %{python_dir}/site-packages/jokosher

Fixed.

> 
> "> You're missing some requires as well - check http://www.jokosher.org/download
> > for details.
> 
> Fixed. Alsaaudio is not a strict requirement for the operation of jokosher and
> the next version will not require it at all. However I will consider packaging
> it if necessary."
> 
> Depending on the timeframe of this packages possible acceptance into FE, I'd
> look into importing it. It won't harm.

A small patch (which only just missed the 0.1 release deadline) has now removed
this dependency on alsaaudio. 

http://jokosher.python-hosting.com/changeset/466

I have updated the package and spec file accordingly and bumped the version.

> 
> I can also replicate the dbus error on my 64 and 32 bit systems. This will need
> to be looked into.

I have posted on this and will get back to you. Would you please in the meantime
avail us of your dbus version which you can get with:

$ python -c "import dbus; print dbus.version"

Regards
Chris
Comment 18 Paul F. Johnson 2006-07-27 18:43:37 EDT
Change the license to GPL with exceptions

The dbus version comes back with

(0, 51, 0)

Can you please upload a new spec and src rpm for review?
Comment 19 Christopher Brown 2006-07-27 19:09:06 EDT
(In reply to comment #18)
Hello Paul,

> Change the license to GPL with exceptions

Done!

> 
> The dbus version comes back with
> 
> (0, 51, 0)

Hmmm, same here. Can you give the output from

rpm -qa | grep -i python

- we are continuing to investigate.
> 
> Can you please upload a new spec and src rpm for review?

Okay, its at:

http://www.iammetal.co.uk/jokosher.spec
http://www.iammetal.co.uk/jokosher-0.1-4.src.rpm

Regards
Chris
Comment 20 Paul F. Johnson 2006-07-27 19:12:15 EDT
the grep python gives you a list of everything with the word python in it!

I have python 2.4.3-8.FC5 installed.

As there seems to be (currently) a need for pydbus, I'd add it as a Requires
Comment 21 Paul F. Johnson 2006-07-27 19:22:37 EDT
Source: %{tmppath}/SOURCE/%{name}-%{version}-%{release}.tar.gz

No!

Source: %{name}-%{version}.tar.gz or the URL link from the website (eg
http://www.foo.org/package/download/com-1.1.1.tar.gz)

remember, the source is the source you've downloaded from the website the
package came from (unless you've built the archive yourself, which is not that
good an idea, especially if you have patches. You should keep the source
unpatched and provide patches which are incorporated via the spec file)

Please package again. The src rpm will include the patches, the spec file will
apply them in %prep - I can't accept it as is.

Can you put the defines right at the start? - it makes things much simpler to
follow. 

Given the specs, it's unlikely this will work in FC5 (unless things are updated
in FC5). I'd drop the versions from the Requires.

mkdir -p $RPM_BUILD_ROOT%{python_sitelib}/jokosher
mkdir -p $RPM_BUILD_ROOT%{python_sitelib}/jokosher/images
mkdir -p $RPM_BUILD_ROOT%{python_sitelib}/jokosher/Instruments
mkdir -p $RPM_BUILD_ROOT%{python_sitelib}/jokosher/Instruments/images

You only need the final line and the images line. -p creates the path requested

--mode=644, much easier just to have -m 0644

install -d $RPM_BUILD_ROOT%{python_sitelib}/jokosher/images

Not sure these are required as you've already created the directories

install -p jokosher $RPM_BUILD_ROOT%{_bindir}/jokosher
chmod 755 $RPM_BUILD_ROOT%{_bindir}/jokosher

can be written

install -p -m 0755 jokosher $RPM_BUILD_ROOT%{_bindir}/jokosher

Why have you got BR desktop-file-utils, but never use them. What are they for?

Still a way to go, but keep on in there.
Comment 22 Paul F. Johnson 2006-07-28 04:15:01 EDT
"Why have you got BR desktop-file-utils, but never use them. What are they for?"

D'oh! I was having a bad time when I wrote that. Apologies.
Comment 23 Christopher Brown 2006-07-28 04:26:46 EDT
(In reply to comment #20)
> the grep python gives you a list of everything with the word python in it!

Err, yep. That was the idea.

> As there seems to be (currently) a need for pydbus, I'd add it as a Requires

Okay, will do.(In reply to comment #22)
> "Why have you got BR desktop-file-utils, but never use them. What are they for?"
> 
> D'oh! I was having a bad time when I wrote that. Apologies.

Ah well, one less thing for me to adjust. No problem. I will make the necessary
changes and re-submit later today.

Regards
Chris
Comment 24 Paul F. Johnson 2006-08-09 06:47:06 EDT
Any signs of the update?
Comment 25 Christopher Brown 2006-08-10 16:06:37 EDT
(In reply to comment #24)
> Any signs of the update?

I am waiting for confirmation on a patch and will re-submit once I have it.
Should be any day now.

Regards
Chris
Comment 26 Paul F. Johnson 2006-08-20 16:19:39 EDT
10 days further on...

I'm not going to be able to review this week (starting a new job), so if you can
get the update in tonight (20th Aug), I'll see what's happening, failing that,
it'll be after the 27th.
Comment 27 Christopher Brown 2006-08-20 19:08:07 EDT
(In reply to comment #26)
> 10 days further on...

Tell me about it, doesn't time fly.

http://www.iammetal.co.uk/jokosher/jokosher.spec
http://www.iammetal.co.uk/jokosher/jokosher-0.1-5.src.rpm

The only response rpmlint gives is on the license which I understand is okay as
it is.

I have now patched the released 0.1 source and these are also available at
http://www.iammetal.co.uk/jokosher

Regards
Chris
Comment 28 Christopher Brown 2006-08-26 19:03:08 EDT
FYI, I have run it through mock without problems.
Comment 29 Paul F. Johnson 2006-08-28 15:33:35 EDT
rpmlint is not clean on the srpm - all warnings

strange-permission jokosher 0755
3 patches not applied (patch0, 1, 2)
invalid-licence (not worried about this one as discussed before - also applies
to the rpm)

mock does, indeed, build cleanly

The URL you have above for the spec file is invalid.

Looking at the spec file I have some concerns

1. Why have you got patches in the install section - they should be in prep. You
can also make them easier on the eye by using

%patch0 -p1

   and that's it!

2. You need to read
http://fedoraproject.org/wiki/Packaging/Python?highlight=%28python%29 as the
python packaging guides have changed. It means %ghost has gone (amongst other
things)

3. I can't allow install -p -m 0755 %{SOURCE1} %{buildroot}%{_bindir}. All
binaries have to be from the package itself and not just dragged from some
random website or other - the possibility for interception and security problems
is boundless like that. If you need a file (such as patch) you bundle it with
the srpm, this is then installed happily into the SOURCES directory when the
srpm is installed.

4. The line mkdir -p %{buildroot}%{python_sitelib}/%{name} is not required. It's
automatically created by the next mkdir line down.
Comment 30 Paul F. Johnson 2006-09-10 11:24:41 EDT
Anymore progress on this package?
Comment 31 Christopher Brown 2006-09-10 18:31:13 EDT
(In reply to comment #30)
> Anymore progress on this package?

Yes! It is now ready for further review at www.iammetal.co.uk/jokosher

Regards
Chris
Comment 32 Paul F. Johnson 2006-09-19 16:55:17 EDT
Why have you got a binary as SOURCE1 which is installed into %{_bindir}? You
cannot include anything prebuilt - you *must* use the sources and was it not
possible to use sed or perl to fix the __init__.py file rather than install a
fresh one?
Comment 33 Toshio Kuratomi 2006-09-19 17:26:58 EDT
The SOURCE1 isn't a binary.  It's a script.  I can see why you're doing that but
it is kinda ugly.  What's upstream's plans for future releases?  Will they
include that kind of script?  Or make it so Jokosher.py actually can be 
installed to %{bindir}?  Or....?

__init__.py might be better as:
touch %{buildroot}%{python-sitelib}/%{name}/__init__.py

but you have a comment in your changelog about security.  Can you explain if
touch would be a problem?

Putting the png images into %{python-sitelib} isn't a great idea.  Is it hard to
put them into %{_datadir}/%{name} or something similar?

If you use your jokosher script you probably don't want execute permissions on
Jokosher.py.

I'm not sure about the wisdom of making WaveForm.py exectuable.  It loks to me
as though directly executing WaveForm.py makes a test case run... so this isn't
something someone who installs jokosher is going to want to do.  (Correct me if
I'm wrong on that.)
Comment 34 Jima 2006-09-20 17:39:53 EDT
Paul, not to be a stick-in-the-mud, but have you noticed this review blocks
FE-NEEDSPONSOR?  Although I suppose Toshio *is* a sponsor...
Comment 35 Christopher Brown 2006-09-28 16:25:14 EDT
(In reply to comment #33)
> The SOURCE1 isn't a binary.  It's a script.  I can see why you're doing that but
> it is kinda ugly.  What's upstream's plans for future releases?  Will they
> include that kind of script?  Or make it so Jokosher.py actually can be 
> installed to %{bindir}?  Or....?

Plans are to make Jokosher.py executable, if not for 0.2 then for 0.3. 0.2 is
out end of November.
 
> __init__.py might be better as:
> touch %{buildroot}%{python-sitelib}/%{name}/__init__.py

fixed.
 
> but you have a comment in your changelog about security.  Can you explain if
> touch would be a problem?

No problem at all, thank you for suggestion.

> Putting the png images into %{python-sitelib} isn't a great idea.  Is it hard to
> put them into %{_datadir}/%{name} or something similar?

The instrument files also sit in this directoy however this is changing in 0.2.
Can this be accepted for 0.1 or am I better off waiting for 0.2? I am beginning
to think the latter as there are a number of things that this process has
highlighted that are resolved in 0.2

> If you use your jokosher script you probably don't want execute permissions on
> Jokosher.py.

fixed

> I'm not sure about the wisdom of making WaveForm.py exectuable.  It loks to me
> as though directly executing WaveForm.py makes a test case run... so this isn't
> something someone who installs jokosher is going to want to do.  (Correct me if
> I'm wrong on that.)

Waveform.py is a test case although one class does import it, this has since
been fixed. I have fixed this however.

As it stands I do not have the know-how to patch the code to meet the
requirements wrt the images and instrument files. Unless these can be overlooked
along with the script executable I will delay this package until an admittedly
more mature app arrives with 0.2. Input appreciated. I have not made a new
release until above is clarified.
Comment 36 Paul F. Johnson 2006-09-28 16:53:01 EDT
Personally, I'd hang fire to the 0.2 release as it sounds like it will fix a lot
of problems you've encountered.

It may be possible if upstream is using svn or cvs to use a snapshot for the
review which would possibly have the faults highlighted here as being fixed.
Comment 37 David Nielsen 2006-10-16 10:15:33 EDT
mike@flyn.org orphaned gnonlin in Extras so there's another showstopper for ya.
Comment 38 Brian Pepple 2006-10-16 10:59:40 EDT
(In reply to comment #37)
> mike@flyn.org orphaned gnonlin in Extras so there's another showstopper for ya.

I've gone ahead and taken ownership of this, since I'd like to see Jokosher get
into FE as soon as possible.
Comment 39 Paul F. Johnson 2006-11-02 16:58:18 EST
Has there been anymore progress on this package?
Comment 40 Christopher Brown 2006-11-02 17:05:39 EST
Brian Pepple (above) has agreed to take ownership of this and I imagine he is
waiting on the 0.2 release...
Comment 41 Paul F. Johnson 2006-11-02 17:08:13 EST
I think you'll find that was for gnonlin - or at least that's how I read it.
Comment 42 Brian Pepple 2006-11-02 18:28:53 EST
No, I talked to Christopher about taking ownership of Jokosher also, since he
didn't have time to work on it.

I've done some initial work on this, but it makes sense to wait for 0.2 to come
out since most of the outstanding issues will be fixed there.
Comment 43 Steven Garrity 2006-11-20 20:41:39 EST
0.2 is out
Comment 44 Brian Pepple 2006-11-20 20:49:15 EST
Yeah, 0.2 is out, but it depends on current gstreamer CVS, which make it's a
non-starter to go into FE right now.
Comment 45 John (J5) Palmieri 2006-11-28 22:17:33 EST
could we have the package install the one plugin it needs (audiopanorama)?  It
could change the name so it doesn't conflict with a newer gstreamer when
introduced and then depend on later gstreamers when released removing the self
installed plugin.  Jokosher is a really important app and should at least get
into devel for testing to speed up developmentment.  

If need be I can build the CVS version in Rawhide but it would be nice to have
FC6 folk get this so they can play around with it.
Comment 46 Brian Pepple 2006-11-29 01:07:21 EST
(In reply to comment #45)
> Jokosher is a really important app and should at least get
> into devel for testing to speed up developmentment.  
> 
> If need be I can build the CVS version in Rawhide but it would be nice to have
> FC6 folk get this so they can play around with it.

I agree that we should try our best to get this into Extras as soon as possible,
but realistically between FESCo and my other projects, I'm not going to have
much free time to work on this.  So if someone in the cc' list wants to take the
ball and run with it, please feel free to.

Comment 47 Christopher Brown 2006-12-18 10:02:01 EST
Just to update everyone.

Gstreamer-plugins-good is still at 0.10.4 in Fedora Development. This week
should see the release of a version compliant with Jokosher's requirements.
LADSPA is a new requirement in Jokosher 0.2 however it is also packaged in FE.
I'm not sure of policy for updating gstreamer in FC6 however I think this will
be a showstopper until FC7.
Comment 48 Paul F. Johnson 2006-12-27 17:01:31 EST
Was there an update for gstreamer to fix this?
Comment 49 Jeffrey C. Ollie 2006-12-27 22:16:43 EST
gstreamer-plugins-good 0.10.5 was released a few days ago but it hasn't made it
into Fedora yet...
Comment 50 Thomas Vander Stichele 2007-01-26 12:17:55 EST
can the 0.2 jokosher package at least be put up here for people to test ?
Comment 51 Christopher Brown 2007-01-28 15:03:58 EST
(In reply to comment #50)
> can the 0.2 jokosher package at least be put up here for people to test ?

There is no package for 0.2 to test. 0.9 is just around the corner. Personally,
I intend to run Rawhide from F7 Test 1 and I may create a package when this
happens in a week or so. You are of course more than welcome to compile your own
however whilst upstream continues to use bleeding edge tools for which there are
no packages themselves built, dependencies cannot be resolved so there is little
point building them. Fedora 7 will, for the first time, contain all the packages
required to run Jokosher in its present form.
Comment 52 Christopher Brown 2007-02-24 22:50:56 EST
I have created a snapshot package from subversion for feedback and testing.
Things to note:

-I have had to change the permissions on a few files whilst upstream gets them
properly set in svn.
-I have used sed make a one line change to remove a shebang and to strip dos
characters from a userguide file

There are still a large number of bugs outstanding at:

https://launchpad.net/jokosher/+bugs

You can download the rpm, source rpm, spec file and source from:

www.iammetal.co.uk/jokosher

Regards
Chris
Comment 53 David Nielsen 2007-02-25 05:19:17 EST
Doesn't really build on current Development:

Traceback (most recent call last):
  File "setup.py", line 79, in <module>
    targeturi = os.path.join(installdir, "share/gnome/", filepath)
  File "/usr/lib64/python2.5/posixpath.py", line 62, in join
    elif path == '' or path.endswith('/'):
AttributeError: 'NoneType' object has no attribute 'endswith'
error: Bad exit status from /var/tmp/rpm-tmp.94847 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.94847 (%build)
Comment 54 Christopher Brown 2007-02-25 06:19:42 EST
(In reply to comment #53)
> Doesn't really build on current Development:

Okay, thanks for testing David. I'm currently using FC6, plan to move to F7 when
T2 arrives so will take a look at this then.

Regards
Chris
Comment 55 Christopher Brown 2007-03-03 13:35:29 EST
(In reply to comment #53)
> Doesn't really build on current Development:

Hi David,

I am now running F7T2 and have update to Rawhide 3/3/07. I cannot replicate your
above bug. Could you re-test with current tree? Can anyone else try building in
mock with rawhide as the target?

Regards
Chris
Comment 56 David Nielsen 2007-03-03 14:04:18 EST
Still getting that on todays tree.. 64 bit issue?
Comment 57 Christopher Brown 2007-03-03 14:25:24 EST
(In reply to comment #56)
> Still getting that on todays tree.. 64 bit issue?

Yes it could well be. I have had someone on #fedora run it through mock on 64
bit with target of rawhide and it has built fine for them however. It would be
good for someone else to confirm this before I go chasing it down...

Regards
Chris
Comment 58 Toshio Kuratomi 2007-03-04 14:41:54 EST
Where are the srpms?  I downloaded
   http://www.iammetal.co.uk/jokosher-0.9-1.20070225svn.src.rpm
and was able to build it in mock development on x86_64.  But the spec file
doesn't have all the fixes reported in previous comments so I'm unsure if I'm
building the latest version (that was based off an older revision by mistake) or
if i'm building something totally foreign.
Comment 59 David Nielsen 2007-03-04 15:17:33 EST
To be clear, I am attempting to build the latest src.rpm from
http://www.iammetal.co.uk/jokosher/ - namely:
http://www.iammetal.co.uk/jokosher/jokosher-0.9-1.20070225svn.src.rpm

The traceback happens right after 
changing mode of build/scripts-2.5/jokosher from 644 to 755
Comment 60 Christopher Brown 2007-03-04 19:29:26 EST
(In reply to comment #58)
> Where are the srpms? I downloaded
>    http://www.iammetal.co.uk/jokosher-0.9-1.20070225svn.src.rpm
> and was able to build it in mock development on x86_64.  But the spec file
> doesn't have all the fixes reported in previous comments so I'm unsure if I'm
> building the latest version (that was based off an older revision by mistake) or
> if i'm building something totally foreign.

The fixes in previous comments have either been applied upstream or are no
longer relevant. I'm not sure exactly what you're asking. The build now runs
from the setup.py file. The srpms are at the URI you indicated. I am building
pre-release packages from svn in readiness for 0.9 final which is due in a few
weeks.

Good to know it is building okay for you in x86_64. I'm still concerned about
David's issue however - any thoughts? What needs to happen now to get this accepted?

Regards
Chris
Comment 61 Craig Hopkins 2007-03-05 11:21:02 EST
The 25/02/2007 rpm installs but wouldn't load because I didn't have the
pkg_resources module. Could python-setuptools be added as a dependency?
Comment 62 Christopher Brown 2007-03-05 11:28:06 EST
(In reply to comment #61)
> The 25/02/2007 rpm installs but wouldn't load because I didn't have the
> pkg_resources module. Could python-setuptools be added as a dependency?

I added this as a buildrequires but can quite happily add it as a dependency. I
will rebuild and pull updates from svn as well, then re-post. Many thanks for
your comments.

Regards
Chris
Comment 63 Toshio Kuratomi 2007-03-05 15:30:37 EST
(In reply to comment #60)
> The fixes in previous comments have either been applied upstream or are no
> longer relevant. I'm not sure exactly what you're asking. The build now runs
> from the setup.py file. The srpms are at the URI you indicated. I am building
> pre-release packages from svn in readiness for 0.9 final which is due in a few
> weeks.
> 
There were some fixes to the spec file that have been lost as well.

> Good to know it is building okay for you in x86_64. I'm still concerned about
> David's issue however - any thoughts? What needs to happen now to get this
accepted?

I've found David's issue and will attach a patch that you can take upstream.  As
for getting this in, I don't have time for a complete review but I do have a few
comments.  Maybe after you fix these and apply the patch, David can continue to
do the review::

* Cosmetic: The tarball you've created is really a .tar file, not a .tar.gz. 
Rpm knows how to handle it, however, and also compresses its payload so it's not
strictly necessary to fix this.  It would be nice to be accurate when a human
extracts the source rpm and tries to look at the sources, though.  So having
jokosher-0.9.tar or actually gzipping the tarball would be appropriate :-)

* A recent addition to the Packaging Guidelines is that for packaging snapshots
you need to show how to recreate the snapshot either in a script that you
include as another Source line or in a comment.  ie::
  # This tarball is a snapshot.  You can recreate it by doing:
  # svn co -r 321 http://svn.jokosher.org/trunk jokosher-0.9
  # tar -czvf jokosher-20070225.snap.tar.gz  jokosher-0.9

This allows reviewers to easily check that the sources are coming from upstream.

* The BuildArch: noarch is missing from the spec file

* You aren't cleaning the buildroot prior to installing (rpmlint warns about this)

* You aren't installing the omf file and registering with scrollkeeper within
the %post/%postun in the spec file so the help files won't be found.

* You aren't calling update-mime-database or update-desktop-database in the spec
file's %post/%postun so jokosher's mimetype and "mailcap" entries aren't being
created.

* You have a raft of unowned directories.  As an example, changing your file
entries from this:
  %{_datadir}/%{name}/pixmaps/*.png
into this:
  %{_datadir}/%{name}/

will own the jokosher directory and all of its subdirectories and files.

Where you cannot do this because you don't want all of the files inside the
directories you can change from this::
  %exclude %{python_sitelib}/Jokosher/Profiler.py
  %{python_sitelib}/Jokosher/*.py
  %{python_sitelib}/Jokosher/*.pyo
  %{python_sitelib}/Jokosher/*.pyc
into this:
  %exclude %{python_sitelib}/Jokosher/Profiler.py
  %dir %{python_sitelib}/Jokosher
  %{python_sitelib}/Jokosher/*.py
  %{python_sitelib}/Jokosher/*.pyo
  %{python_sitelib}/Jokosher/*.pyc

* It looks like you've got the jokosher help in three places:
/usr/share/gnome/help/jokosher, /usr/share/doc/jokosher-0.9/userguide, and
/usr/share/doc/jokosher-0.9/jokosher

It probably only neds to be in /usr/share/gnome/help/

* You need to use the %find_lang macro to include the *.mo files, not just
include them in the %files section.  The way you've currently got it setup,
people won't be able to specify which languages they're interested in when they
install this.

* David's error is coming from the section of setup.py dealing with installing
omf files.  However, the whole handling of omf files has issues.  Attaching a patch.
Comment 64 Toshio Kuratomi 2007-03-05 15:33:08 EST
Created attachment 149290 [details]
Rework omf file installation

Patch to setup.py that better handles omf file installation.  OMF files are
always installed now.  But we don't register them with scrollkeeper unless the
user is root.
Comment 65 Christopher Brown 2007-03-06 20:30:11 EST
(In reply to comment #63)

> I've found David's issue and will attach a patch that you can take upstream.  As
> for getting this in, I don't have time for a complete review but I do have a few
> comments.  Maybe after you fix these and apply the patch, David can continue to
> do the review::

I have submitted your patch upstream Toshio, thank you for that.
 
> * Cosmetic: The tarball you've created is really a .tar file, not a .tar.gz. 
> Rpm knows how to handle it, however, and also compresses its payload so it's not
> strictly necessary to fix this.  It would be nice to be accurate when a human
> extracts the source rpm and tries to look at the sources, though.  So having
> jokosher-0.9.tar or actually gzipping the tarball would be appropriate :-)

Done.

> * A recent addition to the Packaging Guidelines is that for packaging snapshots
> you need to show how to recreate the snapshot either in a script that you
> include as another Source line or in a comment.  ie::
>   # This tarball is a snapshot.  You can recreate it by doing:
>   # svn co -r 321 http://svn.jokosher.org/trunk jokosher-0.9
>   # tar -czvf jokosher-20070225.snap.tar.gz  jokosher-0.9
> 
> This allows reviewers to easily check that the sources are coming from upstream.

Done.

> * The BuildArch: noarch is missing from the spec file

Fixed.

> * You aren't cleaning the buildroot prior to installing (rpmlint warns about this)

Fixed.
 
> * You aren't installing the omf file and registering with scrollkeeper within
> the %post/%postun in the spec file so the help files won't be found.

Fixed.

> * You aren't calling update-mime-database or update-desktop-database in the spec
> file's %post/%postun so jokosher's mimetype and "mailcap" entries aren't being
> created.

Fixed.

> * You have a raft of unowned directories.  As an example, changing your file
> entries from this:
>   %{_datadir}/%{name}/pixmaps/*.png
> into this:
>   %{_datadir}/%{name}/
> 
> will own the jokosher directory and all of its subdirectories and files.
> 
> Where you cannot do this because you don't want all of the files inside the
> directories you can change from this::
>   %exclude %{python_sitelib}/Jokosher/Profiler.py
>   %{python_sitelib}/Jokosher/*.py
>   %{python_sitelib}/Jokosher/*.pyo
>   %{python_sitelib}/Jokosher/*.pyc
> into this:
>   %exclude %{python_sitelib}/Jokosher/Profiler.py
>   %dir %{python_sitelib}/Jokosher
>   %{python_sitelib}/Jokosher/*.py
>   %{python_sitelib}/Jokosher/*.pyo
>   %{python_sitelib}/Jokosher/*.pyc

Fixed.

> * It looks like you've got the jokosher help in three places:
> /usr/share/gnome/help/jokosher, /usr/share/doc/jokosher-0.9/userguide, and
> /usr/share/doc/jokosher-0.9/jokosher
> 
> It probably only neds to be in /usr/share/gnome/help/

Fixed.
 
> * You need to use the %find_lang macro to include the *.mo files, not just
> include them in the %files section.  The way you've currently got it setup,
> people won't be able to specify which languages they're interested in when they
> install this.

Done.

> * David's error is coming from the section of setup.py dealing with installing
> omf files.  However, the whole handling of omf files has issues.  Attaching a
patch.

Thank you for taking the time to respond Toshio. I have run through rpmlint and
receive no errors. It also appears to build cleanly under mock development on
i386. I will notify as and when your patch is accepted upstream however for the
moment the new packages are in the usual location at:

http://www.iammetal.co.uk/jokosher

Regards
Chris
Comment 66 David Nielsen 2007-03-08 00:08:22 EST
revision 1337 doesn't fix my problem but 1339 does, also to make this build rpms
I had to add the omf file to the %files section. Outside of that jokosher now
runs on my Development box and there was much rejoicing.
Comment 67 Christopher Brown 2007-03-08 03:59:14 EST
(In reply to comment #66)
> revision 1337 doesn't fix my problem but 1339 does, also to make this build rpms
> I had to add the omf file to the %files section. Outside of that jokosher now
> runs on my Development box and there was much rejoicing.

:) Thanks David, good to know. r1339 includes Toshio's fix so I'm pulling that
tonight and rebuilding.

Regards
Chris
Comment 68 Christopher Brown 2007-03-08 19:00:40 EST
Toshio's patch has now been accepted upstream and Davis reports in #66 that it
works for him. Everybody seems happy - please advise what else needs to be done.

Regards
Chris
Comment 69 Peter Gordon 2007-03-13 00:36:35 EDT
I can't sponsor you, but here's a brief unofficial review of your spec/SRPM to
help expedite getting this into Fedora because it's such an awesome little app. :]

Unfortunately, Mock is also quite broken at the moment, so I cannot post
comments about its build-time structure or quality. (However, it seems to run
okay through a standard rpmbuild invocation; and rpmlint is silent on that built
RPM.)

== Unofficial Review of jokosher 0.9-1.20070308svn == 
-----
BAD: The package does not follow the Naming Guidelines: The Release tag should
be "0.%{X}.%{alphatag}" or equivalent as noted in the "Pre-Release packages"
section of the naming guidelines (Packaging/NamingGuidelines on the wiki).

BAD: You have duplicate BuildRequires listed: python-devel and pycairo-devel are
both dependencies of pygtk2-devel. Thus, simply listing pygtk2-devel as a
BuildRequires should pull in the other two automagically; and you should remove
those two from the BuildRequires list.

BAD: %{_datadir}/omf/%{name}/, %{_datadir}/gnome/help/%{name}/, and
%{_datadir}/gnome/help/%{name}/C/ should all be owned by the package. Otherwise
they are orphaned when the package is removed. We don't like clutter in our
filesystems. :]

BAD: The package should invoke desktop-file-install to install the .desktop file
in your %install section. See the "desktop-file-install usage" section on the
wiki's Packaging/Guidelines page for more details.

BAD: Your package does not have an appropriate %clean section. It should contain
simply "rm -rf $RPM_BUILD_ROOT". See 
-----
GOOD :The spec file is named "%{name}.spec" in accordance with Naming Guidelines.

GOOD: BuildRoot is "%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u}
-n)", following the packaging guidelines. 

GOOD: Included documentation (%doc) is OK; and it seems to run well without
these files installed.

GOOD: Package includes an appropriate .desktop file since it is a graphical
application.

GOOD: Macros are used instead of hardcoded file names, and usage of these macros
(including $RPM_BUILD_ROOT) is consistent throughout the spec file.

GOOD: Locale files are handled correctly, using %find_lang appropriately.

GOOD: Package is not relocatable.

GOOD: Package includes appropriate code and content.

GOOD: Package does not own any system files/directories or any files/directories
that conflict with another package.   

GOOD: Package license (GPL) is OK; and a copy of it is included in the package
as documentation (%doc COPYING). The License field in the spec file properly
reflects this.

GOOD: Spec file is legible and in American English.

GOOD: No duplicates are listed in the %files section; and its %defattr line is good.

GOOD: When installed, the application runs well with no apparent segfaults or
major bugs.

GOOD: Appropriate scriplets for MIME-type, Scrollkeeper, and .desktop files are
used.

GOOD: The %files section has a proper %defattr(-,root,root,-) line.
-----
N/A: No non-ASCII characters are needed, so encoding is OK.
N/A: This is a noarch package, so compiler flags are not needed. 
N/A: No static libraries or RPATH exclusions are needed.
N/A: Package includes no configuration files or data, so %config markings are
not needed.
N/A: Package uses Python setuptools for building; so using `make %{?_smp_flags}`
is not needed.
N/A: Package is not a web application.
N/A: Package is noarch; thus no ExcludeArch/ExclusiveArch tweaking is required.
N/A: Package installs no shared libraries; thus %post/%postun scriplets of
/sbin/ldconfig are not needed.
N/A: No large (neither in size nor in quantity) documentation is included, thus
no -doc subpackage is needed.
N/A: No headers, no pkgconfig files, and no static or unsuffixed shared
libraries are included. Thus, no -devel subpackage is needed.
N/A: Package contains no libtool archives (*.la files) 
N/A: Package contains no %description or Summary translations.
-----
MINOR(?): The source tarball does not match that of upstream (in this case, the
upstream tarball is generated from the svn export as you explain in your spec
comments):
    $ md5sum SOURCES/jokosher-0.9*
    24ae1fa6adad7893a58fcd32aaec1ff3  jokosher-0.9-srpm.tar.gz
    26837769e637a9225fb26afe243488db  jokosher-0.9-upstream.tar.gz
However, if I grab another SVN snapshot and tar it up, the md5sum changes yet
again, so this seems to be a simple false positive; and is probably safe to ignore.

MINOR: It's just preference, but instead of listing the directory via %dir and
globbing all of the files inside of it, it's probably simpler to simply list the
directory itself as a parent. RPM will automagically make it grab all the files
and directories in that recursively. E.g., instead of the current:
    %dir %{python_sitelib}/Jokosher
    %{python_sitelib}/Jokosher/*.py
    %{python_sitelib}/Jokosher/*.pyo
    %{python_sitelib}/Jokosher/*.pyc
You could use something like:
    %{python_sitelib}/Jokosher/
Your %exclude line should still hold, so that's ok.
Comment 70 Christopher Brown 2007-03-16 19:58:49 EDT
Hello Peter,

(In reply to comment #69)
> I can't sponsor you, but here's a brief unofficial review of your spec/SRPM to
> help expedite getting this into Fedora because it's such an awesome little app. :]

I'm glad you agree - its been a long time coming :P

> Unfortunately, Mock is also quite broken at the moment, so I cannot post
> comments about its build-time structure or quality. (However, it seems to run
> okay through a standard rpmbuild invocation; and rpmlint is silent on that built
> RPM.)

Mock is working for me and builds fine with the following changes. rpmlint is quiet.

> BAD: The package does not follow the Naming Guidelines: The Release tag should
> be "0.%{X}.%{alphatag}" or equivalent as noted in the "Pre-Release packages"
> section of the naming guidelines (Packaging/NamingGuidelines on the wiki).

This should now be fixed as I have started bumping the %{X} section with each
new build.

> BAD: You have duplicate BuildRequires listed: python-devel and pycairo-devel are
> both dependencies of pygtk2-devel. Thus, simply listing pygtk2-devel as a
> BuildRequires should pull in the other two automagically; and you should remove
> those two from the BuildRequires list.

Fixed.

> BAD: %{_datadir}/omf/%{name}/, %{_datadir}/gnome/help/%{name}/, and
> %{_datadir}/gnome/help/%{name}/C/ should all be owned by the package. Otherwise
> they are orphaned when the package is removed. We don't like clutter in our
> filesystems. :]

Fixed.

> BAD: The package should invoke desktop-file-install to install the .desktop file
> in your %install section. See the "desktop-file-install usage" section on the
> wiki's Packaging/Guidelines page for more details.

Fixed.

> BAD: Your package does not have an appropriate %clean section. It should contain
> simply "rm -rf $RPM_BUILD_ROOT". See 

Fixed.

> MINOR(?): The source tarball does not match that of upstream (in this case, the
> upstream tarball is generated from the svn export as you explain in your spec
> comments):
>     $ md5sum SOURCES/jokosher-0.9*
>     24ae1fa6adad7893a58fcd32aaec1ff3  jokosher-0.9-srpm.tar.gz
>     26837769e637a9225fb26afe243488db  jokosher-0.9-upstream.tar.gz
> However, if I grab another SVN snapshot and tar it up, the md5sum changes yet
> again, so this seems to be a simple false positive; and is probably safe to
ignore.

Okay, let me know if this becomes a problem. Note I am doing an svn export as
opposed to a svn checkout to lose all the usual svn cruft.
 
> MINOR: It's just preference, but instead of listing the directory via %dir and
> globbing all of the files inside of it, it's probably simpler to simply list the
> directory itself as a parent. RPM will automagically make it grab all the files
> and directories in that recursively. E.g., instead of the current:
>     %dir %{python_sitelib}/Jokosher
>     %{python_sitelib}/Jokosher/*.py
>     %{python_sitelib}/Jokosher/*.pyo
>     %{python_sitelib}/Jokosher/*.pyc
> You could use something like:
>     %{python_sitelib}/Jokosher/
> Your %exclude line should still hold, so that's ok.

Fair point. Hopefully it now looks cleaner. 

http://www.iammetal.co.uk/jokosher/

Cheers
Chris
Comment 71 Peter Gordon 2007-03-22 21:30:54 EDT
Thanks, Christopher.

Toshio Kuratomi emailed me about this review saying that he could sponsor you if
I did the formal review. So, I'll re-assign this to myself since Paul seems to
have mysteriously vanished. :|

I'll formally review this when I get home from work tomorrow. Most of the work's
already been done, so it'll be a lot simpler (just testing of build-time
functionality/quality as well as ensuring the fixes of the previous blockers
which you appear to have fixed up nicely).

Thanks.
Comment 72 Peter Gordon 2007-03-24 03:02:09 EDT
Mock is currently grabbing some build roots, so since it's going to take
severals hours for it all I'm going to let it run overnight, so I'll post the
review tomorrow morning.

Thanks.
Comment 73 Peter Gordon 2007-03-24 13:25:02 EDT
(In reply to comment #70)
> > BAD: The package does not follow the Naming Guidelines: The Release tag should
> > be "0.%{X}.%{alphatag}" or equivalent as noted in the "Pre-Release packages"
> > section of the naming guidelines (Packaging/NamingGuidelines on the wiki).
> 
> This should now be fixed as I have started bumping the %{X} section with each
> new build.
> 

That's still not correct. For pre-release snapshots, the final version from
upstream will be Release 1, so everything before it must be Release 0.X, and
with a suffix of the snapshot date tag too.

Everything else looks fixed, with the exception of your dependencies: pycairo is
a dependency of pygtk2, so you should remove the former.

GOOD: Builds fine in mock (x86_64 and i386, FC-6 and Devel); and rpmlint is
silent on the source and built (noarch) packages. 


> Okay, let me know if this becomes a problem. Note I am doing an svn export as
> opposed to a svn checkout to lose all the usual svn cruft.
>  
I just did the svn export again and it came up with a yet different MD5 hash, so
that seems to be entirely a false positive.

It's a minor issue, too, but for some parts in your %files section, you use the
%name macro, yet in others you hardcode "jokosher." Is there a specific reason
for this inconsistency?

You just need to address these three issues, then it'll be approved. Thanks.
Comment 74 Christopher Brown 2007-03-25 18:17:18 EDT
(In reply to comment #73)
> (In reply to comment #70)
> > > BAD: The package does not follow the Naming Guidelines: The Release tag should
> > > be "0.%{X}.%{alphatag}" or equivalent as noted in the "Pre-Release packages"
> > > section of the naming guidelines (Packaging/NamingGuidelines on the wiki).
> > 
> > This should now be fixed as I have started bumping the %{X} section with each
> > new build.
> > 
> 
> That's still not correct. For pre-release snapshots, the final version from
> upstream will be Release 1, so everything before it must be Release 0.X, and
> with a suffix of the snapshot date tag too.

I must confess I find the Naming Guidelines a little confusing but hopefully the
current iteration meets them. I have tried to match it with the kismet example:

     kismet-0-0.1.20040110svn (this is a pre-release, svn checkout of kismet)

> Everything else looks fixed, with the exception of your dependencies: pycairo is
> a dependency of pygtk2, so you should remove the former.

Done.

> It's a minor issue, too, but for some parts in your %files section, you use the
> %name macro, yet in others you hardcode "jokosher." Is there a specific reason
> for this inconsistency?

Fixed.

> You just need to address these three issues, then it'll be approved. Thanks.

Looking forward to it.

www.iammetal.co.uk/jokosher
Comment 75 Peter Gordon 2007-03-25 22:34:36 EDT
The %files and dependencies look good now; thanks for fixing them.

The last little bit is that your naming is still off: Your Version tag should
still be 0.9, since that is what you are distribution snapshots of. Thus, your
full EVR should look something like the following. Also (since this isn't a
data-only package) you should probably be using the %{?dist} tag to facilitate
release upgrades:

Version: 0.9
Release: 0.n.YYYYMMDDsvn%{?dist}

(Increment n for each successive snapshot, then use Release: 1 for the final
version.)

We're almost there. :]
Comment 76 Peter Gordon 2007-03-25 22:36:55 EDT
(In reply to comment #75)
"... since that is what you are distribution snapshots of."

Apologies; that should be "...what you are distributing snapshots of." 

I blame muscle memory. :o
Comment 77 Christopher Brown 2007-03-26 13:39:38 EDT
(In reply to comment #75)
> The %files and dependencies look good now; thanks for fixing them.
> 
> The last little bit is that your naming is still off: Your Version tag should
> still be 0.9, since that is what you are distribution snapshots of. Thus, your
> full EVR should look something like the following. Also (since this isn't a
> data-only package) you should probably be using the %{?dist} tag to facilitate
> release upgrades:
> 
> Version: 0.9
> Release: 0.n.YYYYMMDDsvn%{?dist}
> 
> (Increment n for each successive snapshot, then use Release: 1 for the final
> version.)
> 
> We're almost there. :]
> 

Okay, how is it now?
Comment 78 Peter Gordon 2007-03-26 20:05:43 EDT
(In reply to comment #77)
> Okay, how is it now?

Much better. The only other issue is that you're hardcoding calls to python
directly, instead of using the %{__python} macro; but this is not a blocker
AFAIK, and is something you can fix if needed after you import it.

Everything's fixed up; and this package (jokosher-0.9-0.1.20070325svn) is
therefore APPROVED.

Go aheard with the account creation requests, and Toshio Kuratomi will take you
through the sponsorship process from here and onward; after which you should
flag this as fedora-cvs? and request your branches, then import your package, et
al.. (Thanks, Toshio!)
Comment 79 Toshio Kuratomi 2007-03-26 20:43:43 EDT
Great job, guys!  Christopher, if you're in the Fedora Account System already, I
need to know what you're user name is.  If not, the next step is for you to sign
up with the Fedora Account System.  Instructions are here:
  http://tinyurl.com/37vcrk

If you need help you can post to this bug or find me on IRC #fedora-devel --
abadger1999.
Comment 80 Christopher Brown 2007-03-28 03:57:37 EDT
New Package CVS Request
=======================
Package Name: jokosher
Short Description: Multi-track non-linear audio editor
Owners: snecklifter@gmail.com
Branches: 
InitialCC: 

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