Bug 176006
Summary: | Review Request: Streamtuner - a stream directory browser. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Matthias Haase <matthias_haase> | ||||||
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> | ||||||
Status: | CLOSED RAWHIDE | QA Contact: | David Lawrence <dkl> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | fedora-package-review, florin, mpeters | ||||||
Target Milestone: | --- | ||||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2006-01-09 23:05:28 UTC | Type: | --- | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Bug Depends On: | |||||||||
Bug Blocks: | 163779 | ||||||||
Attachments: |
|
Description
Matthias Haase
2005-12-17 08:20:08 UTC
Not a review, just some comments: %define desktop_vendor endur %define name streamtuner %define version 0.99.99 %define release 2.fc4 %define prefix /usr Lose all that. use the real Name / Version / Release in the appropriate places Do not define prefix - do not define desktop_vendor -=- Source0: %{name}-%{version}.tar.gz Should be full url to the upstream tarball Obsoletes: streamtuner-live365, streamtuner-local, streamtuner-xiph Obsoletes: streamtuner-python Why the Obsoletes? I'm assuming because there are or were packages with those names on upstream website? If so that's probably fine - but then you need to add provides for them. --- %build CFLAGS="$RPM_OPT_FLAGS" %configure --prefix=%{prefix} make %{?_smp_mflags} should be %build %configure make %{?_smp_mflags} You don't need to define CFLAGS - rpm will do that. %configure sets the prefix for you --- %makeinstall There's not %install section. You need %install rm -rf %{buildroot} Then you can use %makeinstall I prefer make DESTDIR=%{buildroot} install sometimes %makeinstall results in some wrong path stuff, but it sometimes is OK. desktop-file-install --vendor %{desktop_vendor} --delete-original \ should be desktop-file-install --vendor=fedora \ -=- %find_lang %{name} That means you need to BuildRequires: gettext -=- %files -f %{name}.lang %defattr(-, root, root) %{_bindir}/%{name} %dir %{_libdir}/%{name} %dir %{_libdir}/%{name}/plugins %{_libdir}/%{name}/plugins/*.so %dir %{_datadir}/%{name} %{_datadir}/%{name}/* %dir %{_datadir}/omf/%{name} %{_datadir}/omf/%{name}/* %{_datadir}/applications/%{desktop_vendor}-%{name}.desktop %{_datadir}/pixmaps/%{name}.png %{_datadir}/gtk-doc/html/* %dir %{_datadir}/help %{_datadir}/help/* %doc AUTHORS COPYING INSTALL NEWS README TODO can be simplified greatly. Usually %doc goes at top - and usually INSTALL insn't packaged. Make sure the other files aren't empty as well (NEWS often is, though not always) %files -f %{name}.lang %defattr(-, root, root, -) %doc AUTHORS COPYING NEWS README TODO %{_bindir}/%{name} %{_libdir}/%{name}/ %{_datadir}/%{name}/ %{_datadir}/omf/%{name}/ %{_datadir}/applications/*.desktop %{_datadir}/pixmaps/%{name}.png %{_datadir}/gtk-doc/html/* %{_datadir}/help/ -=- %{_datadir}/help/ though is probably wrong. -=- Also - all your summary lines end in a . They shouldn't. desktop-file-install --vendor %{desktop_vendor} --delete-original \ should be desktop-file-install --vendor=fedora \ Note that you don't need to --delete-original because the original isn't installed in the buildroot (you can, but not need to) also - noticed %define release 2.fc4 When you do it - use %{?dist} tag - ie Release: 2%{?dist} The build system will have dist defined properly. Hi, Michael... ...the required things are changed now. You'll find the changed files here... Spec Name or Url: http://www.bennewitz.com/rpms/streamtuner.spec SRPM Name or Url: http://www.bennewitz.com/rpms/streamtuner-0.99.99-3.src.rpm -- Regards Matthias Need to add the following BuildRequires: desktop-file-utils Otherwise it won't build in mock. [mpeters@utility result]$ ls *.rpm && rpmlint *.rpm streamtuner-0.99.99-3.fc4.i386.rpm streamtuner-0.99.99-3.fc4.src.rpm streamtuner-debuginfo-0.99.99-3.fc4.i386.rpm streamtuner-devel-0.99.99-3.fc4.i386.rpm E: streamtuner obsolete-not-provided streamtuner-live365 E: streamtuner obsolete-not-provided streamtuner-local E: streamtuner obsolete-not-provided streamtuner-xiph E: streamtuner obsolete-not-provided streamtuner-python E: streamtuner useless-explicit-provides live365.so E: streamtuner useless-explicit-provides local.so W: streamtuner-devel no-documentation [mpeters@utility result]$ Since you have the obsoletes set on those packages, it needs to Provide them as well. The explicit .so provides should probably be removed. The warning on the devel package - if there isn't any developer doc to package, it can be ignored. -=- From the build.log of the mock build I did: Installation directories --bindir /usr/bin --datadir /usr/share --libdir /usr/lib --includedir /usr/include Features --enable-shoutcast yes --enable-live365 yes --enable-xiph no (libxml not found) --enable-local yes --enable-local-metadata yes --enable-python yes I suspect you need to add libxml2-devel to the BuildRequires -=- Other than those issues, it looks good. Oh - one more thing - in the %changelog - * Sun Dec 18 2005 Matthias Haase <matthias_haase> - 0.99.99-3.fc4 You should drop the .fc4 from the changelog. That's provided by the %{?dist} tag - so the same spec file could be used to build for fc3/4/5 (assuming it will build on all of them) -=- Please note that I'm happy to informally review this package, it looks likes a good addition - but I am not able to sponsor people - so a reviewer who is able to do so still is needed. I have done some cleanups on the specfile. Again you'll find the changed files here... Spec Name or Url: http://www.bennewitz.com/rpms/streamtuner.spec SRPM Name or Url: http://www.bennewitz.com/rpms/streamtuner-0.99.99-4.fc4.src.rpm Because I have defined %{dist} in ~/.rpmmacros, the filename of the source rpm contains 'fc4' now. And I have considered including streamtuner in extras because some people have ask about this, already using my Fedora rpm from upstream, in order to make it easier to install (yum install streamtuner). Thanks for your review, I'll change some other specfiles later too, following the provided strict style. -- Regards Matthias Excellent - rpmlint is clean - [mpeters@jerusalem result]$ ls *.rpm && rpmlint *.rpm streamtuner-0.99.99-4.fc4.i386.rpm streamtuner-0.99.99-4.fc4.src.rpm streamtuner-debuginfo-0.99.99-4.fc4.i386.rpm streamtuner-devel-0.99.99-4.fc4.i386.rpm W: streamtuner-devel no-documentation [mpeters@jerusalem result]$ from build.log -=- Features --enable-shoutcast yes --enable-live365 yes --enable-xiph yes --enable-local yes --enable-local-metadata yes --enable-python yes -=- builds everything it is suppose to (in mock) (In reply to comment #6) > Excellent - rpmlint is clean - > builds everything it is suppose to (in mock) (In reply to comment #4) > but I am not able to sponsor people - so a reviewer who is able to do so still is needed. Have I to do some extra steps at this stage to get sponsored? -- Regards Matthias Someone with the power to sponsor has to decide to do so. One thing to encourage that is to comment on other peoples packages to demonstrate the understanding of Fedora RPM PackagingGuidelines. http://fedoraproject.org/wiki/Extras/Contributors You are at Step 6 - so someone who has authority to sponsor (from the sponsor list) has to do a formal review of the package. one thing to mention is that some plugins require you to have a mp3 capable audio player to listen to the streams. (In reply to comment #9) > one thing to mention is that some plugins require you to have a mp3 capable > audio player to listen to the streams. Hi, Che... ...streamtuner does not play/decode mp3's _by itself_ and isn't useless without an mp3 capable audio player. You can use streamtuner, for example, together with xmms and without an installed mp3 plug-in for xmms, getting from elsewhere. Same usability as for xmms or beep media player, related to the mp3 decoding stuff. --- Regards Matthias but theres the question if those plugins should maybe be split off... its not worth it but well i see bogus bug reports coming if this doesent go to the faq e.g. After a first look... streamtuner-devel package: * Missing "Requires: gtk2-devel" because else it would break the pkgconfig dependency chain. * The Cflags line in the pkgconfig file is bad. It ought not add -I/usr/include because that is a standard search path for header files already. * The included static libraries and libtool archives are _highly_ questionable. Unless you can state a very good reason why static versions of the plugin (!) DSOs should be shipped, delete them after %install or %exclude them in the %files section. streamtuner binary package: * Category X-Red-Hat-Base in .desktop file is wrong. Should be X-Fedora, because this is not Fedora Core. Run-time observation: * Clicking "Browse" button or "Help > Homepage" menu gives fatal error dialog: Unable to browse Failed to execute child process "epiphany" (no such file or directory) Looks like either a missing customisation or a missing dependency. * "xmms" also seems to be another missing requirement, as an error dialog pops up upon trying to tune in to a station. * "Record" button fails with xterm: Can't execvp streamripper: No such file or directory Looks like more missing dependencies: "xterm", and do we have "streamripper" anywhere yet? spec file issues: * Licence GPL is wrong. The included file COPYING contains is "BSD" licence text. The C source files don't mention GPL either. * Requires: gtk2 >= 2.2.2, curl >= 7.10.8, taglib >= 1.3, libidn >= 0.5.6 All of these must go. There are automatic RPM dependencies on the needed SONAMEs already. Check out "rpm -qR streamtuner". Package resolvers know which packages to install in order to get the required libraries. * Requires: pygtk2 >= 2.4.0 At a first glance it doesn't become clear why this dependency was added. * Concerning all those versions in the [build] requirements, consider dropping them. Unless, of course, you insist on verifying all of them for each and every update. ;) Basically, upstream's configure script ought to check the available build dependencies. That's enough. Trying to keep such versions in sync with updates results in a maintenance nightmare. * -devel package sub-section ought to do "Requires: %{name} = %{version}-%{release} with full %release to keep -devel package and main package in sync. * Missing dependencies on the tools used in the scriptlets: Requires(post): desktop-file-utils scrollkeeper Requires(postun): desktop-file-utils scrollkeeper Although, running update-desktop-database in the scriptlets is not necessary for this package [yet]. > pygtk2
Ah, for the included Python scripts.
(In reply to comment #12) > After a first look... > > > * Clicking "Browse" button or "Help > Homepage" menu gives fatal > error dialog: > > Unable to browse > Failed to execute child process > "epiphany" (no such file or directory) > Looks like either a missing customisation or a missing > dependency. One browser is required and should be added to the requirement. Which browser? Firefox? The default config file should be patched, matching this browser, I think. > * "xmms" also seems to be another missing requirement, as an > error dialog pops up upon trying to tune in to a station. Related to user settings... > * "Record" button fails with > xterm: Can't execvp streamripper: No such file or directory > Looks like more missing dependencies: "xterm", and do we have > "streamripper" anywhere yet? Yes, I have created an additional "streamripper" package already... Xterm is required only for these streamripper feature. > spec file issues: I'll change this and I'll cleanup some others tomorrow. (In reply to comment #14) > Which browser? Firefox? htmlview or gnome-open? (In reply to comment #13) > > pygtk2 > > Ah, for the included Python scripts. All required cleanups are done now (I think so) ;-) Beep-media-player, htmlview and xterm are added to the requirement. I had to patch several files to change the default configuration. You have to delete ~/.streamtuner/ to make these changes visible. The streamripper rpm is an usefull add-on, but isn't a requirement at this time, it uses libmad. The changed files are there already: Spec Url: http://www.bennewitz.com/rpms/streamtuner.spec SRPM Url: http://www.bennewitz.com/rpms/streamtuner-0.99.99-5.fc4.src.rpm -- Matthias Created attachment 122667 [details]
fix .pc file
Created attachment 122668 [details]
spec patch to remove redundant dependencies on hardcoded package names
Dependenies on curl, libidn, taglib and gtk2 are automatic already, see
rpm -qR streamtuner | grep 'curl\|idn\|tag\|gtk'
The new defaults and fixed dependencies work much better here. Running update-desktop-database is still not necessary since the .desktop file doesn't activate any MIME-type assignments. All changes are done and the changed files are there already: Spec Url: http://www.bennewitz.com/rpms/streamtuner.spec SRPM Url: http://www.bennewitz.com/rpms/streamtuner-0.99.99-6.fc4.src.rpm * reported issues fixed * patches visited * spec file looks good * binary package contents look good * upstream locations verified (home page has moved to http://www.nongnu.org/streamtuner/ ) * upstream source is signed APPROVED The remaining unneeded dependency on desktop-file-utils can be deleted in CVS. If you have signed up for an account already, tell me your user name, and I will sponsor you. Else feel free to sign up. > If you have signed up for an account already, tell me your user name,
> and I will sponsor you. Else feel free to sign up.
My user name is 'endur'.
__
Regards
Matthias
> The remaining unneeded dependency on desktop-file-utils can be deleted
> in CVS.
Fixed in CVS. I'm currently at step 18. Branches for FC-3 and FC-4 are missing.
And
$ make plague fails currently with
/usr/bin/plague-client build streamtuner streamtuner-0_99_99-7_fc5 devel
Server returned an error: Insufficient privileges.
Creation of distribution branches in CVS can be requested in the Wiki: http://fedoraproject.org/wiki/Extras/FC4Status http://fedoraproject.org/wiki/Extras/FC3Status If you uploaded your correct and undamaged SSH public key in the accounts system, if you configured plague-client and the needed certificates in your home directory, http://fedoraproject.org/wiki/Extras/BuildSystemClientSetup and if the problem persists, only the accounts system staff can find out what the problem with your account is. Help contact address is at the bottom of https://admin.fedora.redhat.com/accounts/ Verifying the configuration steps and re-uploading your SSH public key might help meanwhile. Although, CVS access would fail due to SSH key misconfiguration, so probably it's plague-client misconfiguration. (In reply to comment #25) > Although, CVS access would fail due to SSH key misconfiguration, so probably > it's plague-client misconfiguration. I can successfully use other plague commands like list and list_builders. > If you uploaded your correct and undamaged SSH public key in the accounts > system This is done already, because there isn't any probblem with the CVS access, as I said. This is very cool, thanks everyone! All builds are done successful.
> it's plague-client misconfiguration.
I couldn't found errors in ~/.plague-client.cfg, but anyway, it is working as
expected today.
--
Matthias
> Spec Url: http://www.bennewitz.com/rpms/streamtuner.spec > SRPM Url: http://www.bennewitz.com/rpms/streamtuner-0.99.99-6.fc4.src.rpm I have removed these files. > Removing the BR will break the build. Removing the Requires a few lines > above would be correct, since the scriptlets no longer run > desktop-update-database. Thanks, yes, of course... I have noticed this later myself and, at once, corrected again. Otherwise, the build hangs at the missed desktop-file-install binary. This ticket can be closed now. |