Bug 199192
Summary: | Review Request: kadu - Gadu-Gadu client for online messaging | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michał Bentkowski <mr.ecik> | ||||||
Component: | Package Review | Assignee: | Axel Thimm <axel.thimm> | ||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | gajownik, gogers, joi, rdieter | ||||||
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-07-24 15:00:24 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
Michał Bentkowski
2006-07-17 20:53:44 UTC
*** Bug 165878 has been marked as a duplicate of this bug. *** The release number for pre-releases should begin with 0 (as stated in NamingGuidelines), so in this case it would be 0.%{date}svn. Created attachment 132755 [details]
kadu-0.5.0-0.20060716svn
Okay, fixed it. But now, if I would like to make next pre-release, should I
increase number before dot? For example, actual version is 0.20060716svn so
next will be 0.20060720svn? I think official release won't be released quickly,
so doing packages of new snapshots may be necessarily.
Is this a pre-release snapshot of 0.5.0? If so, there should be an additional dot and an integer between the leading "0" and the "20060716svn", like 0.1.20060716svn, see "Pre-release packages" at http://fedoraproject.org/wiki/Packaging/NamingGuidelines Created attachment 132763 [details]
0.5.0-0.1.20060716svn.spec
Yes, you're right, I haven't read Packing Naming Guidelines carefully :/ So
there is new spec file with correct release number.
(In reply to comment #5) > Created an attachment (id=132763) [edit] > 0.5.0-0.1.20060716svn.spec > > Yes, you're right, I haven't read Packing Naming Guidelines carefully :/ So > there is new spec file with correct release number. I have done it right in my not-yet-released package and here I give a wrong advice. (In reply to comment #6) > I have done it right in my not-yet-released package and here I give a wrong advice. Never mind ;-) Well... maybe someone will decide to review this package? Removing FE-NEEDSPONSOR as MichaÅ has been sponsored. Manus manum lavat, reviewing the bug in the next comments. :) MUST items ========== - rpmlint output (see below): not OK + package name: OK - (latest) spec file name (0.5.0-0.1.20060716svn.spec): not OK, please use kadu.spec again + license is open source: OK + license in sources (GPL2) and license in specfile (GPL) match: OK - license (COPYING) included, but not in %doc: not OK (?) o American English: OK (but there is a trivial typo: s/Develpoments/Development/ in %description devel and missing "by" at some "made by" in other descriptions) + specfile legible: OK (but see comments below) - sources match upstream: not OK http://www.kadu.net/download/snapshots/kadu-20060716.tar.bz2 doesn't exist anymore + compiles and build into binary rpms: OK on i386 + sane BRs: OK + locales: OK (doesn't use locale folder directly, installs in private location) + ldconfig usage: OK (none neccessary, no shared libs under %{_libdir}) + owns its directories: OK + %files duplicate: OK (none reported) - permissions: not OK (see below) + %clean section: OK + system macros: OK (used) + contains code: OK + doc subpackage: OK (not needed) + %doc contents do not influence runtime: OK + devel package (contents): OK + devel package (dependencies): OK + %{name}.desktop/desktop-file-install: OK rpmlint: ======== errors: E: kadu-debuginfo empty-debuginfo-package E: kadu explicit-lib-dependency libsndfile warnings: W: kadu-alsa_sound no-documentation W: kadu-alsa_sound unstripped-binary-or-object /usr/lib64/kadu/modules/alsa_sound.so W: kadu-amarok no-documentation W: kadu-amarok unstripped-binary-or-object /usr/lib64/kadu/modules/amarok.so W: kadu-arts_sound no-documentation W: kadu-arts_sound unstripped-binary-or-object /usr/lib64/kadu/modules/arts_sound.so W: kadu-dcopexport no-documentation W: kadu-dcopexport unstripped-binary-or-object /usr/lib64/kadu/modules/dcopexport.so W: kadu-devel no-documentation W: kadu-esd_sound no-documentation W: kadu-esd_sound unstripped-binary-or-object /usr/lib64/kadu/modules/esd_sound.so W: kadu-exec_notify no-documentation W: kadu-exec_notify unstripped-binary-or-object /usr/lib64/kadu/modules/exec_notify.so W: kadu-ext_info no-documentation W: kadu-ext_info unstripped-binary-or-object /usr/lib64/kadu/modules/ext_info.so W: kadu-filedesc no-documentation W: kadu-filedesc unstripped-binary-or-object /usr/lib64/kadu/modules/filedesc.so W: kadu-iwait4u no-documentation W: kadu-iwait4u unstripped-binary-or-object /usr/lib64/kadu/modules/iwait4u.so W: kadu-led_notify no-documentation W: kadu-led_notify unstripped-binary-or-object /usr/lib64/kadu/modules/led_notify.so W: kadu-miastoplusa_sms no-documentation W: kadu-miastoplusa_sms unstripped-binary-or-object /usr/lib64/kadu/modules/miastoplusa_sms.so W: kadu mixed-use-of-spaces-and-tabs W: kadu no-documentation W: kadu setup-not-quiet W: kadu-spellchecker no-documentation W: kadu-spellchecker unstripped-binary-or-object /usr/lib64/kadu/modules/spellchecker.so W: kadu-tabs no-documentation W: kadu-tabs unstripped-binary-or-object /usr/lib64/kadu/modules/tabs.so W: kadu-theme-crystal16 no-documentation W: kadu-theme-crystal22 no-documentation W: kadu-theme-gg6_compatible no-documentation W: kadu-theme-glass16 no-documentation W: kadu-theme-glass22 no-documentation W: kadu-theme-nuvola16 no-documentation W: kadu-theme-nuvola22 no-documentation W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/account_management.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/arts_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/autoaway.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/autoresponder.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/config_wizard.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/dcc.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/default_sms.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/dsp_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/encryption.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/ext_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/hints.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/migration.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/sms.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/voice.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/window_notify.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/x11_docking.so W: kadu-weather no-documentation W: kadu-weather unstripped-binary-or-object /usr/lib64/kadu/modules/weather.so W: kadu-xmms no-documentation W: kadu-xmms unstripped-binary-or-object /usr/lib64/kadu/modules/xmms.so comments: ========= - build should include debugging symbols, e.g. the buildlog has: > Compile with debug symbols: no Please compile with debug symbols and let rpm automagically extract them out into the debuginfo package. - permissions of shared libs should be executable to allow for debuginfo extraction - "Requires: libsndfile" is not neccessary (unless it needs something from %{_bindir}/sndfile-* at runtime - "mixed-use-of-spaces-and-tabs": Use emacs to tabbify/untabbify the specfile - "%setup -D -T -n kadu" is missing "-q" - "unstripped-binary-or-object": Fix by making the modules executable either with chmod at the end of %install of with %attr/%defattr - self-defined macros: In general I'm not against self-defined macros to make packages more flexible or legible. In this case some macros make the specfile harder to read than neccessary for example _kadudir being %{_datadir}/kadu. I wouldn't call this a blocker, but I would recommend to unwrap most of these macros to increase legibility. Please install rpmlint on your system and use it on the packages while developing the specfile. First, thank for review! > o American English: OK (but there is a trivial typo: s/Develpoments/ Development/ > in %description devel and missing "by" at some "made by" in other > descriptions) Fixed. > - sources match upstream: not OK > http://www.kadu.net/download/snapshots/kadu-20060716.tar.bz2 doesn't exist > anymore Fixed; the address was changed. > comments: > ========= > > - build should include debugging symbols, e.g. the buildlog has: > > Compile with debug symbols: no > Please compile with debug symbols and let rpm automagically extract them out > into the debuginfo package. Fixed. > - permissions of shared libs should be executable to allow for debuginfo > extraction Fixed. > > - "Requires: libsndfile" is not neccessary (unless it needs something from > %{_bindir}/sndfile-* at runtime Require deleted. > - "mixed-use-of-spaces-and-tabs": Use emacs to tabbify/untabbify the specfile Now, it's OK. > - "%setup -D -T -n kadu" is missing "-q" Fixed. > - "unstripped-binary-or-object": Fix by making the modules executable either > with chmod at the end of %install of with %attr/%defattr Fixed. > - self-defined macros: In general I'm not against self-defined macros to make > packages more flexible or legible. In this case some macros make the specfile > harder to read than neccessary for example _kadudir being %{_datadir}/kadu. > I wouldn't call this a blocker, but I would recommend to unwrap most of these > macros to increase legibility. No, I won't unwrap these macros. This is still SVN pre-release, some things may change in future and macros make me easier to change path. > Please install rpmlint on your system and use it on the packages while > developing the specfile. I've already installed it, but most of your errors wasn't showed for me by rpmlint, do you use any arguments to it? I'll give new addresses for SRPM and SPEC in next comment. New files: Spec URL: http://ecik.zspswidwin.pl/kadu/kadu.spec SRPM URL: http://ecik.zspswidwin.pl/kadu/kadu-0.5.0-0.2.20060716svn.src.rpm >- build should include debugging symbols, e.g. the buildlog has:
>> Compile with debug symbols: no
> Please compile with debug symbols and let rpm automagically extract them out
> into the debuginfo package.
note that --enable-debug compiles in very noisy console output (binary is ~8-10%
bigger)
> > Please install rpmlint on your system and use it on the packages while > > developing the specfile. > > I've already installed it, but most of your errors wasn't showed for me by > rpmlint, do you use any arguments to it? No, just use it on the src.rpm as well as on the binary rpm. Maybe you only used it on the src.rpm? > note that --enable-debug compiles in very noisy console output (binary is > ~8-10% bigger) Isn't this removed when stripping off the symbols? rpm does that automatically and generates debuginfo packages with these symbols. So usually one installs the stripped binary w/o the extra space and when an issue comes up one can install the matching debuginfo rpm and run gdb on it. I wrote: > - "unstripped-binary-or-object": Fix by making the modules executable either > with chmod at the end of %install of with %attr/%defattr That was wrong. I rebuilt the latest src.rpm and the unstripped binaries are still around. You need to chmod then, %attr alone doesn't help as it is taken into account *after* the debuginfo creation. Sorry for the misleading information. Can you fix that and also %exclude %{_kadudir}/AUTHORS %{_kadudir}/ChangeLog %{_kadudir}/COPYING %{_kadudir}/HISTORY %{_kadudir}/README %{_kadudir}/THANKS and instead add them with %doc? All other points were checked and are OK. For reference: + spec file name: OK + sources match upstream: OK + American English: OK + permissions: OK (on manifest level) - license (COPYING) included, but not in %doc: not OK - rpmlint output: $ rpmlint *rpm | grep -v kadu-.*no-documentation W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/voice.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/autoaway.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/config_wizard.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/migration.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/window_notify.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/dcc.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/account_management.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/autoresponder.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/arts_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/sms.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/hints.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/dsp_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/x11_docking.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/default_sms.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/ext_sound.so W: kadu unstripped-binary-or-object /usr/lib64/kadu/modules/encryption.so W: kadu no-documentation W: kadu-alsa_sound unstripped-binary-or-object /usr/lib64/kadu/modules/alsa_sound.so W: kadu-amarok unstripped-binary-or-object /usr/lib64/kadu/modules/amarok.so W: kadu-arts_sound unstripped-binary-or-object /usr/lib64/kadu/modules/arts_sound.so W: kadu-dcopexport unstripped-binary-or-object /usr/lib64/kadu/modules/dcopexport.so W: kadu-esd_sound unstripped-binary-or-object /usr/lib64/kadu/modules/esd_sound.so W: kadu-exec_notify unstripped-binary-or-object /usr/lib64/kadu/modules/exec_notify.so W: kadu-ext_info unstripped-binary-or-object /usr/lib64/kadu/modules/ext_info.so W: kadu-filedesc unstripped-binary-or-object /usr/lib64/kadu/modules/filedesc.so W: kadu-iwait4u unstripped-binary-or-object /usr/lib64/kadu/modules/iwait4u.so W: kadu-led_notify unstripped-binary-or-object /usr/lib64/kadu/modules/led_notify.so W: kadu-miastoplusa_sms unstripped-binary-or-object /usr/lib64/kadu/modules/miastoplusa_sms.so W: kadu-spellchecker unstripped-binary-or-object /usr/lib64/kadu/modules/spellchecker.so W: kadu-tabs unstripped-binary-or-object /usr/lib64/kadu/modules/tabs.so W: kadu-weather unstripped-binary-or-object /usr/lib64/kadu/modules/weather.so W: kadu-xmms unstripped-binary-or-object /usr/lib64/kadu/modules/xmms.so If you fix the above two issues this output will vanish and the package approved :) (In reply to comment #14) > I wrote: > > - "unstripped-binary-or-object": Fix by making the modules executable either > > with chmod at the end of %install of with %attr/%defattr > That was wrong. I rebuilt the latest src.rpm and the unstripped binaries are > still around. You need to chmod then, %attr alone doesn't help as it is taken > into account *after* the debuginfo creation. Sorry for the misleading information. I hope them is stripped good, now :-) > > Can you fix that and also %exclude > > %{_kadudir}/AUTHORS > %{_kadudir}/ChangeLog > %{_kadudir}/COPYING > %{_kadudir}/HISTORY > %{_kadudir}/README > %{_kadudir}/THANKS > > and instead add them with %doc? As I write in my first post I cannot do it, because Kadu uses these files in About window so they must be in proper directory. New URLs: Spec URL: http://ecik.zspswidwin.pl/kadu/kadu.spec SRPM URL: http://ecik.zspswidwin.pl/kadu/kadu-0.5.0-0.3.20060716svn.src.rpm I noticed very stupid mistake in %changelog entry. 23 July is not Saturday as I wrote, but Sunday ;-) I sent corrected spec file, but I didn't change SRPM file. > As I write in my first post I cannot do it, because Kadu uses these files
> in About window so they must be in proper directory.
Would it be OK to have them at both locations? At least for the license it's
important to have it under %doc, too.
(In reply to comment #15) > As I write in my first post I cannot do it, because Kadu uses these files > in About window so they must be in proper directory. Maybe adding a small patch? (In reply to comment #19) > Maybe adding a small patch? I'm not convinced to it. Files in %doc should be unicode coded, but kadu doc files is iso8859-2 coded and if we change it, in About window characters could be wrong. The most reasonable solution to me is include in %doc only COPYING file. >> note that --enable-debug compiles in very noisy console output (binary is >> ~8-10% bigger) > >Isn't this removed when stripping off the symbols? no > rpm does that automatically and generates debuginfo packages with these > symbols. how would rpm know which (used in code) symbols to strip? how would it remove _kdebug_with_mask calls? how would it remove code between #ifdef DEBUG_ENABLED and #endif? you need to disable debug or make a patch for Kadu sources On debug symbols: rpm strips out all debugging symbols. E.g. everything that -g adds is removed again and placed into the debuginfo package. In case of a bug/core dump etc. You can install the debuginfo package and examine the issue with gdb. Of course if there are different code paths chosen with debuging enabled, that's another story. But we were discussing about enabling debug symbols, nothing more. On COPYING: Yes, please add another copy of COPYING to %doc. Don't make the application dependent on %doc. (In reply to comment #22) > On COPYING: > > Yes, please add another copy of COPYING to %doc. Don't make the application > dependent on %doc. Okay, done. URLs: Spec URL: http://ecik.zspswidwin.pl/kadu/kadu.spec SRPM URL: http://ecik.zspswidwin.pl/kadu/kadu-0.5.0-0.4.20060716svn.src.rpm Package approved. Thanks a lot, package built successfully. A few comments: * Requires: qt is superfluous. * Since you used the macro: %define _themesdir %{_datadir}/%{name}/themes It appears that %{_datadir}/%{name} ends up being unowned. * -devel includes: %{_libdir}/libgadu.a The packaging guidelines mandate (recommend strongly?) that static libs be omitted. That is, unless you can make a (strong) case to justify its inclusion. * Personally, I'd say making that many subpkgs is nutty (and a maintainance nightmare), but that's just me. (: (In reply to comment #26) > * Requires: qt > is superfluous. In fact, many packages don't use it, I'll remove it soon. > * -devel includes: > %{_libdir}/libgadu.a > The packaging guidelines mandate (recommend strongly?) that static libs be > omitted. That is, unless you can make a (strong) case to justify its > inclusion. As I see, packaging guidelines says .la files should be omitted, not .a. And many packages include .a files in their -devel > * Personally, I'd say making that many subpkgs is nutty (and a maintainance > nightmare), but that's just me. (: Yes, but Polish Fedora Community should be happy with it ;-) > As I see, packaging guidelines says .la files should be omitted, not .a. Look a little closer. (: http://fedoraproject.org/wiki/Packaging/Guidelines item 15. Exclusion of Static libraries > And many packages include .a files in their -devel We're working on bringing pkgs into compliance... I'm still not convinced of that, but okay, I'll fix it in next release. > I'm still not convinced of that
The PackagingGuidelines, imo, are pretty clear.
What about it are you not convinced (or do you disagree with the guideline)?
Okay, I had to inquire about some things, now everything is clear. It'll be fixed. |