Bug 222388
Summary: | Review Request: gnucash - personal finance management | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Bill Nottingham <notting> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | rvokal |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | j:
fedora-review+
j: fedora-cvs+ |
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-06-09 03:54:38 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 |
Description
Bill Nottingham
2007-01-11 23:11:18 UTC
Is there a reason why the gconf2 --makefile-uninstall-rule is missing? http://fedoraproject.org/wiki/Packaging/ScriptletSnippets Obviously, because once you install it, you will never ever want to uninstall it, evar. Or, I screwed up. Fixed & uploaded. ok, I would be happy to review this package... Look for a full review in a bit (after I can figure out why doing a mockbuild of this on my test box causes it to lockup). OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPL/GFDL) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: 5755b05a3eaebab392fe9ad49073beb2 gnucash-2.0.4.tar.bz2 5755b05a3eaebab392fe9ad49073beb2 gnucash-2.0.4.tar.bz2.1 ffc058efd0283a4b43ca31980c40db49 gnucash-docs-2.0.1.tar.bz2 ffc058efd0283a4b43ca31980c40db49 gnucash-docs-2.0.1.tar.bz2.1 afa10712d00b6a90aef0dc7fbb116ff30ded91cb gnucash-2.0.4.tar.bz2 afa10712d00b6a90aef0dc7fbb116ff30ded91cb gnucash-2.0.4.tar.bz2.1 ce04f51e8eeb8324b7abca6bf84ddb18562cf6b4 gnucash-docs-2.0.1.tar.bz2 ce04f51e8eeb8324b7abca6bf84ddb18562cf6b4 gnucash-docs-2.0.1.tar.bz2.1 See below - BuildRequires correct OK - Spec handles locales/find_lang See below - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package is a GUI app and has a .desktop file OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. See below - No rpmlint output. OK - final provides and requires are sane SHOULD Items: See below - Should build in mock. OK - Should build on all supported archs OK - Should have dist tag OK - Should package latest version Issues: 1. Is this package built often from svn snapshots? ie, are the %if's for svn building needed anymore? 2. Is the %defattr(-,root,root,755) needed? Or will %defattr(-,root,root,-) work? 3. Does rpm fail at finding the perl requires? Is the '%define __perl_requires %{nil}' still needed? 4. Doesn't seem to build here in mock/devel. The build.log has at the end: checking for libgsf-1 >= 1.12.2 libgsf-gnome-1 >= 1.12.2... Package libgsf- gnome-1 was not found in the pkg-config search path. Perhaps you should add the directory containing `libgsf-gnome-1.pc' to the PKG_CONFIG_PATH environment variable No package 'libgsf-gnome-1' found configure: error: Library requirements (libgsf-1 >= 1.12.2 libgsf-gnome-1 >= 1.12.2) not met; consider adjusting the PKG_CONFIG_PATH environment variable if your libraries are in a nonstandard prefix so pkg-config can find them. error: Bad exit status from /var/tmp/rpm-tmp.55382 (%build) Looks like missing 'Buildrequires: libgsf-gnome-devel' Adding that gets it building on devel here. 5. The "--disable-sql" seems to have been added back in fc4. Is it worth re-enabling now? 6. Should the --with-cairo be commented in or out? 7. Our friend rpmlint says: E: gnucash obsolete-not-provided gnucash-backend-postgres I don't know how long ago the gnucash-backend-postgres was removed, but it might be good to provide gnucash-backend-postgres as long as the obsolete is still there. E: gnucash invalid-soname /usr/lib/libgncqof-backend-qsf.so libgncqof-backend- qsf.so E: gnucash invalid-soname /usr/lib/libgnc-backend-file.so libgnc-backend-file.so Can be ignored. I think rpmlint can't handle things with - in the filename when it's not a major version number. W: gnucash non-conffile-in-etc /etc/gconf/schemas/apps_gnucash_warnings.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/apps_gnucash_history.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_prices.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_window_pages_register.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_reconcile.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_hbci.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_common.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/apps_gnucash_general.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_business_common.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_print_checks.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_import_generic_matcher.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_totd.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_window_pages_account_tree.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_scheduled_transctions.schemas W: gnucash non-conffile-in-etc /etc/gconf/schemas/ apps_gnucash_dialog_commodities.schemas I think as you say that all these can be ignored. E: gnucash non-executable-script /usr/share/xml/gnucash/xsl/vcard- gnccustomer.pl 0644 Might nuke the #!/usr/bin/perl from this script or make it executable? E: gnucash shell-syntax-error-in-%post There seems to be a unattached done in the %post. W: gnucash unversioned-explicit-obsoletes gnucash-backend-postgres typically it's good to add a version thats being obsoleted. E: gnucash hardcoded-library-path in /usr/lib/debug Is the '%exclude /usr/lib/debug' needed? E: gnucash-debuginfo script-without-shebang /usr/src/debug/gnucash-2.0.4/src/ import-export/import-commodity-matcher.c Should be mode 644? 8. .la files should be nuked unless there is a good reason to keep them. 9. It looks like the docs aren't released for each main gnucash release. Perhaps it would make sense to split them into a gnucash-docs package? That would save people 10MB of update when just gnucash was updated. 10. Does %{?_smp_mflags} not work with this package? Unless it breaks something, that should be added to both the main make and the docs make. FYI, I just did a test build here... the smp_flags is a no go. It blows up... scratch item 10 off. On item 2: The %defattr(-,root,root,-) seems to generate ok permissions from a quick look. (In reply to comment #4) > Issues: > > 1. Is this package built often from svn snapshots? > ie, are the %if's for svn building needed anymore? It was in the run-up to 2.0. I haven't really built it much from SVN since then. It could be removed as it does dirty up the spec file a lot. > 2. Is the > %defattr(-,root,root,755) > needed? Or will > %defattr(-,root,root,-) > work? 755 is the default, so just (-,root,root) should work. Fixed. > 3. Does rpm fail at finding the perl requires? > Is the '%define __perl_requires %{nil}' still needed? Ooh, good catch. This was done to avoid generating perl requires on something that we didn't ship that was satisfied by installing an Extras package. I'll try without it to see if it can be removed. > 4. Doesn't seem to build here in mock/devel. > The build.log has at the end: > > checking for libgsf-1 >= 1.12.2 libgsf-gnome-1 >= 1.12.2... Package libgsf- > gnome-1 was not found in the pkg-config search path. Perhaps you should add the > directory containing `libgsf-gnome-1.pc' to the PKG_CONFIG_PATH environment > variable No package 'libgsf-gnome-1' found > configure: error: Library requirements (libgsf-1 >= 1.12.2 libgsf-gnome-1 >= > 1.12.2) not met; consider adjusting the PKG_CONFIG_PATH environment variable if > your libraries are in a nonstandard prefix so pkg-config can find them. > error: Bad exit status from /var/tmp/rpm-tmp.55382 (%build) > > Looks like missing 'Buildrequires: libgsf-gnome-devel' > Adding that gets it building on devel here. Hm, wonder if that was split. Will check. > 5. The "--disable-sql" seems to have been added back in fc4. > Is it worth re-enabling now? The SQL support is deprecated, lacking a maintainer, and not supported upstream. > 6. Should the --with-cairo be commented in or out? It's an option that the builtin libgoffice/libgsf snapshots took, that ended up not working. (In Core, we built this with the builtin goffice support.) As we're using separate goffice/gsf, the comment can be removed. > 7. Our friend rpmlint says: > > E: gnucash obsolete-not-provided gnucash-backend-postgres > > I don't know how long ago the gnucash-backend-postgres was removed, > but it might be good to provide gnucash-backend-postgres as long > as the obsolete is still there. Well, we don't provide the functionality, and it was always built from the gnucash SRPM. I'm not seeing how you could get into a situation where you'd need the provide. > E: gnucash non-executable-script /usr/share/xml/gnucash/xsl/vcard- > gnccustomer.pl 0644 > > Might nuke the #!/usr/bin/perl from this script or make it executable? It's an example script, so having it non-executable makes sense to me. (Not sure why they install it in datadir instead of doc) > E: gnucash shell-syntax-error-in-%post > > There seems to be a unattached > done > in the %post. Good catch, fixed. > W: gnucash unversioned-explicit-obsoletes gnucash-backend-postgres > > typically it's good to add a version thats being obsoleted. See above - not sure how you'd get to where it's needed. > E: gnucash hardcoded-library-path in /usr/lib/debug > > Is the '%exclude /usr/lib/debug' needed? Otherwise %{_libdir}/* will pick it up. > E: gnucash-debuginfo script-without-shebang /usr/src/debug/gnucash-2.0.4/src/ > import-export/import-commodity-matcher.c > > Should be mode 644? Possibly. Not sure it matters in debuginfo, but fixed. > 8. .la files should be nuked unless there is a good reason to keep them. gnucash uses g-wrap, which uses libltdl to load shared objects. Unfortunate, but required. > 9. It looks like the docs aren't released for each main gnucash release. > Perhaps it would make sense to split them into a gnucash-docs package? > That would save people 10MB of update when just gnucash was updated. Not sure how it helps when you have the main package always requiring gnucash-docs. New stuff uploaded to http://people.redhat.com/notting/review/ with said fixes. (In reply to comment #6) 1. ok. 2. ok. 3. ok. 4. ok. Yeah, libgsf-gnome-devel was split off recently in devel. 5. ok. 6. ok. 7. ok. All makes sense. 8. ok. 9. >> 9. It looks like the docs aren't released for each main gnucash release. >> Perhaps it would make sense to split them into a gnucash-docs package? >> That would save people 10MB of update when just gnucash was updated. > >Not sure how it helps when you have the main package always requiring gnucash- docs. Well, for instance if you have Requires: gnucash-docs = 2.0.1 now, you can upgrade the main package without needing docs updates until the next time they update the docs. I don't know how often upstream updates the main package without the docs also being updated however, so this might not be worth the trouble. A few additional items: - There is a 'Requires: qbanking'. Nothing currently provides that... Should that be 'aqbanking'? Any particular version? - Oddly the changes you made in the rpath disabling seem to have fixed whatever I was seeing with the smp_mflags not working. This cuts buildtime here from about 70min to about 23min. Can you confirm adding smp_mflags works now? (In reply to comment #8) > >> 9. It looks like the docs aren't released for each main gnucash release. > >> Perhaps it would make sense to split them into a gnucash-docs package? > >> That would save people 10MB of update when just gnucash was updated. > > > >Not sure how it helps when you have the main package always requiring gnucash- > docs. > > Well, for instance if you have Requires: gnucash-docs = 2.0.1 now, you > can upgrade the main package without needing docs updates until the next > time they update the docs. I don't know how often upstream updates the > main package without the docs also being updated however, so this might > not be worth the trouble. Hm, possibly. Let me see how much pain it is to split, shouldn't be too much. > A few additional items: > > - There is a 'Requires: qbanking'. Nothing currently provides that... > Should that be 'aqbanking'? Any particular version? See currently-under-review aqbanking package - it's split off. > - Oddly the changes you made in the rpath disabling seem to have fixed > whatever I was seeing with the smp_mflags not working. This cuts buildtime > here from about 70min to about 23min. Can you confirm adding smp_mflags > works now? I'll need to test - I've still seen errors where the guile bindings/C libraries get out of sync and fail. gnucash-docs split off, bug 227210. New gnucash spec & srpm uploaded to reflect this. Sounds good. I can review that at some point here if no one beats me to it. ;) On this package, the only thing I see left is to check if smp_mflags works all the time, or if it still causes problems. If you could do that before the next build that would be fine... This package is APPROVED. Don't forget to close it NEXTRELEASE once you have tested the smp_mflags and such. make with -j33 yields... gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I../.. -I../.. -I../.. -I../../lib/libc -I../../src -I../../src/core-utils -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -std=gnu99 -pthread -I/usr/lib/g-wrap/include -pthread -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wdeclaration-after-statement -Wno-pointer-sign -D_FORTIFY_SOURCE=2 -MT gw-gnc-module.lo -MD -MP -MF .deps/gw-gnc-module.Tpo -c gw-gnc-module.c -fPIC -DPIC -o .libs/gw-gnc-module.o gw-gnc-module.c:167: error: expected identifier or '(' before 'if' gw-gnc-module.c:183: warning: data definition has no type or storage class gw-gnc-module.c:183: warning: type defaults to 'int' in declaration of 'typespec' gw-gnc-module.c:183: error: conflicting types for 'typespec' So... no. Ah well, it would have been nice to have. I guess my test was on a dual core box, so smp_mflags was only 2... perhaps the higher number of jobs triggers something. ;( Is built now. Package CVS Request =================== Package Name: gnucash New Branches: EL-4 EL-5 CVS done. |