Spec URL: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec SRPM URL: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.5-1.fc14.src.rpm Description: GNUmed is suitable for any health care provider interested in keeping a sound and comprehensive medical record. It is currently in use with GPs and physical therapists. GNUmed safely operates on networks of a few to many users, and supports secure, remote access. It does also operate on a single computer, which makes it possible to initially examine the software, and may suit doctors or nurse clinicians serving rural or disadvantaged areas with limited infrastructure.
New upstream release. Spec URL: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec SRPM URL: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.6-1.fc14.src.rpm
A few comments; too many things wrong to do a full review at this time. I sure hope this isn't a copy of the package upstream is currently offering, because I'm afraid that making an acceptable package is going to make it difficult to keep much compatibility with that one. The Source0: URL is invalid; upstream only seems to have 14.5 there. Given that, I probably shouldn't build this, but what the heck, it's a disposable build machine. rpmlint says: gnumed-server.noarch: W: only-non-binary-in-usr-lib This is very problematic. The package references _libdir, so the location of the files depends on the architecture it was built on, but it's a noarch package. Plus things reference /usr/lib explicitly while that stuff is in /usr/lib64 on my build. Are you sure the stuff that's in _libdir shouldn't be in _datadir instead? What there is arch-specific? gnumed-server.noarch: W: no-documentation gnumed-server.noarch: W: no-manual-page-for-binary gm-remove_person.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-move_backups_offsite.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-backup_database.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_database.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-adjust_db_settings.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-bootstrap_server gnumed-server.noarch: W: no-manual-page-for-binary gm-backup_data.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_data.sh gnumed-server.noarch: W: no-manual-page-for-binary gm-zip+sign_backups.sh It's nice to have _some_ documentation, and it looks like this package has some. At least there's a license text and some manpages, buried under _libdir. Is there some reason those shouldn't be installed into the proper places? Why are the same scripts installed in two places? How do you configure this package? Nothing is installed under /etc and nothing is marked as %conf. If this is a server, how do you start and stop it? Doesn't it need an initscript? Why does this require a local postgres server? Will it not work with a remote one? You don't need BuildRoot:, %clean or the buildroot cleaning in %install unless you intend to support EL4 or EL5 with the same spec.
Thanks for the comments. Please find the inline comments. > A few comments; too many things wrong to do a full review at this time. I sure > hope this isn't a copy of the package upstream is currently offering, because > I'm afraid that making an acceptable package is going to make it difficult to > keep much compatibility with that one. No, my packaging skills are not world class yet. ;) > The Source0: URL is invalid; upstream only seems to have 14.5 there. Given > that, I probably shouldn't build this, but what the heck, it's a disposable > build machine. They did release a 14.6, but for some reason it is not in the directory. I shall be working with upstream to get it resolved. > rpmlint says: > gnumed-server.noarch: W: only-non-binary-in-usr-lib > This is very problematic. The package references _libdir, so the location of > the files depends on the architecture it was built on, but it's a noarch > package. Plus things reference /usr/lib explicitly while that stuff is in > /usr/lib64 on my build. Are you sure the stuff that's in _libdir shouldn't be > in _datadir instead? What there is arch-specific? I am having a look. > gnumed-server.noarch: W: no-documentation > gnumed-server.noarch: W: no-manual-page-for-binary gm-remove_person.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-move_backups_offsite.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-backup_database.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_database.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-adjust_db_settings.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-bootstrap_server > gnumed-server.noarch: W: no-manual-page-for-binary gm-backup_data.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_data.sh > gnumed-server.noarch: W: no-manual-page-for-binary gm-zip+sign_backups.sh > It's nice to have _some_ documentation, and it looks like this package has > some. At least there's a license text and some manpages, buried under _libdir. > Is there some reason those shouldn't be installed into the proper places? I am having a look. > Why are the same scripts installed in two places? > How do you configure this package? Nothing is installed under /etc and nothing > is marked as %conf. My mistake. Sorry. > If this is a server, how do you start and stop it? Doesn't it need an > initscript? The way it works is this: One copies it over to relevant directories and then run a bootstrap script which sets up the database and the necessary configurations. > Why does this require a local postgres server? Will it not work with a remote > one? The upstream wiki mentions that it _requires_ a local server. Thanks for the initial review, I shall get these corrected.
Hi, I have cleared the above issues. The only thing I am a little bit confused about are the dependencies. Do we really need mailx and bzip2 to be stated explicitely? The new spec file is: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec New SPRM is: http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.6-2.fc14.src.rpm
1. Rpmlint output is fine. 2. Mock build is ok. $ rpmlint gnumed-server.spec ../SRPMS/gnumed-server-14.6-2.fc14.src.rpm ../RPMS/noarch/gnumed-server-14.6-2.fc14.noarch.rpm gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_database gnumed-server.noarch: W: no-manual-page-for-binary gm-zip+sign_backups gnumed-server.noarch: W: no-manual-page-for-binary gm-move_backups_offsite gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_data 2 packages and 1 specfiles checked; 0 errors, 4 warnings. I have manually checked that these files do not have man pages. I shall work with upstream to see if they can include these.
New upstream release and a few fixes. http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.7-1.fc14.src.rpm
I'll take this one :)
Review: [+] OK [-] NA [X] issue + Package meets naming and packaging guidelines + Spec file matches base package name. + Spec has consistant macro usage. + Meets Packaging Guidelines. + License X License field in spec matches X License file included in package + Spec in American English + Spec is legible. + Sources match upstream md5sum: - Package needs ExcludeArch - BuildRequires correct - Spec handles locales/find_lang - Package is relocatable and has a reason to be. + Package has %defattr and permissions on files is good. + Package has a correct %clean section. + Package has correct buildroot %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) + Package is code or permissible content. - Doc subpackage needed/used. + Packages %doc files don't affect runtime. - Headers/static libs in -devel subpackage. - Spec has needed ldconfig in post and postun - .pc files in -devel subpackage/requires pkgconfig - .so files in -devel subpackage. - -devel package Requires: %{name} = %{version}-%{release} - .la files are removed. X Package is a GUI app and has a .desktop file + Package compiles and builds on at least one arch. + Package has no duplicate files in %files. + Package doesn't own any directories other packages own. X Package owns all the directories it creates. The directories here are not owned by the package: %doc %{_defaultdocdir}/%{name}/* %{_sharedstatedir}/%{name}/* You'll either have to use %dir %{_defaultdocdir}/%{name} or simply %doc %{_defaultdocdir}/%{name} + No rpmlint output. [ankur@ankur noarch]$ rpmlint gnumed-server-14.7-1.fc16.noarch.rpm ~/rpmbuild/SPECS/gnumed-server.spec ~/rpmbuild/SRPMS/gnumed-server-14.7-1.fc14.src.rpm gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_database gnumed-server.noarch: W: no-manual-page-for-binary gm-zip+sign_backups gnumed-server.noarch: W: no-manual-page-for-binary gm-move_backups_offsite gnumed-server.noarch: W: no-manual-page-for-binary gm-restore_data /home/ankur/rpmbuild/SPECS/gnumed-server.spec: W: invalid-url Source0: http://www.gnumed.de/downloads/server/v14/gnumed-server.14.7.tgz HTTP Error 404: Not Found gnumed-server.src: W: invalid-url Source0: http://www.gnumed.de/downloads/server/v14/gnumed-server.14.7.tgz HTTP Error 404: Not Found 2 packages and 1 specfiles checked; 0 errors, 6 warnings. + final provides and requires are sane: (include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done manually indented after checking each line. I also remove the rpmlib junk and anything provided by glibc.) == gnumed-server-14.7-1.fc16.noarch.rpm == Provides: config(gnumed-server) = 14.7-1.fc16 gnumed-server = 14.7-1.fc16 Requires: /bin/bash /bin/sh /usr/bin/env bzip2 config(gnumed-server) = 14.7-1.fc16 gnupg2 mailx mx openssl postgresql postgresql-client postgresql-filedump python-psycopg2 rsync SHOULD Items: + Should build in mock. http://koji.fedoraproject.org/koji/taskinfo?taskID=2935502 + Should build on all supported archs - Should function as described. - Should have sane scriptlets. - Should have subpackages require base package with fully versioned depend. + Should have dist tag + Should package latest version - check for outstanding bugs on package. (For core merge reviews) Issues: 1. Some of the files show a GPL license (not a v2). This needs to be added to the spec. A copy of the v1 license should also be included in the package. 2. Clean and buildroot sections aren't needed anymore. (not a blocker!!) 3. The package is a GUI tool right? In that case a desktop file must be added to the package as described here http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files 4. Doesn't the package require Python? I don't see it in the Requires. Please check on this. 5. I haven't been able to check the functioning (yet). 6. Please check the other issues outlined above too (ownership etc.) Thanks, Ankur
Oh, and please check the changelog format. I don't think the %dist tag is supposed to be in it.
Some suggestions: (None of these are blockers! :) ) - Instead of moving the example conf file, why not just write up a fedora.README file and add it to docs? That way, the example file stays, and a user will have a concrete source of documentation on how to set the server up? - You don't need to gzip the man pages. IIRC rpm does that automagically. - Is there a reason you're using mv instead of a simple rename? For example: for script in `ls ./*.sh`; do mv $script `echo $script|sed 's/.sh//'`; done could simply be written as $ rename ".sh" "" *.sh - You need to add a comment describing the patch. The patch should also be named as gnumed-server-%reason.patch. Again, not a blocker, but this is the preferred method. Thanks, Ankur
1. This is a server, not a GUI. 2. Not compressing man pages gives error. 3. All other comments taken care of. 4. $rpmlint RPMS/noarch/gnumed-server-14.8-1.fc14.noarch.rpm gives gnumed-server.noarch: E: incorrect-fsf-address /usr/share/doc/gnumed-server/GnuPublicLicense.txt This is more of an annoyance I guess. 4. Can you please recheck? http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.8-1.fc14.src.rpm Thanks.
(In reply to comment #11) > 1. This is a server, not a GUI. Okay > 2. Not compressing man pages gives error. It doesn't. rpmbuild does automagically gzip the man pages. I made this change to your spec: #for man in `ls doc/*.*`; \ # do gzip $man; \ #done #cp -p doc/*.8.gz %{buildroot}%{_mandir}/man8 #cp -p doc/*.1.gz %{buildroot}%{_mandir}/man1 cp -p doc/*.8 %{buildroot}%{_mandir}/man8 cp -p doc/*.1 %{buildroot}%{_mandir}/man1 and built the resulting srpm in mock. This is what I got: [ankur@ankur SRPMS]$ rpm -qd /var/lib/mock/fedora-rawhide-i386/result/gnumed-server-14.8-1.fc16.noarch.rpm -p /usr/share/doc/gnumed-server/GnuPublicLicense.txt /usr/share/doc/gnumed-server/README /usr/share/doc/gnumed-server/schema/gnumed-schema-no_audit.dot /usr/share/doc/gnumed-server/schema/gnumed-schema.html /usr/share/man/man1/gm-remove_person.1.gz /usr/share/man/man8/gm-adjust_db_settings.8.gz /usr/share/man/man8/gm-backup_data.8.gz /usr/share/man/man8/gm-backup_database.8.gz /usr/share/man/man8/gm-bootstrap_server.8.gz /usr/share/man/man8/gm-fixup_server.8.gz /usr/share/man/man8/gm-set_gm-dbo_password.8.gz /usr/share/man/man8/gm-upgrade_server.8.gz Looks okay :) > 3. All other comments taken care of. About the gpl patch. Is it really required? Just including a License as SOURCE1 wouldn't suffice? > > 4. > > $rpmlint RPMS/noarch/gnumed-server-14.8-1.fc14.noarch.rpm gives > gnumed-server.noarch: E: incorrect-fsf-address > /usr/share/doc/gnumed-server/GnuPublicLicense.txt > > This is more of an annoyance I guess. I've asked the devel list on how one should be handling this. We'll probably have to patch it to the new address. Some tweaks are needed to the files section. There are unowned directories. This is what it should look like: %defattr(-,root,root,-) %doc %{_defaultdocdir}/%{name}/ %{_sharedstatedir}/%{name}/ %{_bindir}/gm-* %{_mandir}/man8/gm-* %{_mandir}/man1/gm-* %config(noreplace) %{_sysconfdir}/%{name}/ In %defattr(-,root,root,-) %doc %{_defaultdocdir}/%{name}/* %{_sharedstatedir}/%{name}/* %{_bindir}/gm-* %{_mandir}/man8/gm-* %{_mandir}/man1/gm-* %config(noreplace) %{_sysconfdir}/%{name}/* the %doc %{_defaultdocdir}/%{name}/ %{_sharedstatedir}/%{name}/ %config(noreplace) %{_sysconfdir}/%{name}/ directories are unowned. The BuildRoot tag isn't required anymore. Please comment on the patches in the spec. These look like the only issues. I'll do a full review again by monday. Thanks, Ankur
> > > 2. Not compressing man pages gives error. > > It doesn't. rpmbuild does automagically gzip the man pages. I made this change > to your spec: > > #for man in `ls doc/*.*`; \ > # do gzip $man; \ > #done > #cp -p doc/*.8.gz %{buildroot}%{_mandir}/man8 > #cp -p doc/*.1.gz %{buildroot}%{_mandir}/man1 > cp -p doc/*.8 %{buildroot}%{_mandir}/man8 > cp -p doc/*.1 %{buildroot}%{_mandir}/man1 > > and built the resulting srpm in mock. Ok. I did not look at the cp. :) > > 3. All other comments taken care of. > > About the gpl patch. Is it really required? Just including a License as SOURCE1 > wouldn't suffice? Yes, I guess. > > 4. > > > > $rpmlint RPMS/noarch/gnumed-server-14.8-1.fc14.noarch.rpm gives > > gnumed-server.noarch: E: incorrect-fsf-address > I've asked the devel list on how one should be handling this. We'll probably > have to patch it to the new address. No, we don't patch licences. https://bugzilla.redhat.com/show_bug.cgi?id=700095 I shall work with upstream to get it corrected. But this is not a blocker I guess. > Some tweaks are needed to the files section. There are unowned directories. > This is what it should look like: Done. > The BuildRoot tag isn't required anymore. Done. > Please comment on the patches in the spec. Done. http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server.spec http://susmit.fedorapeople.org/packaging/gnumed-server/gnumed-server-14.8-1.fc14.src.rpm Thanks.
ping.
http://koji.fedoraproject.org/koji/taskinfo?taskID=3103205 All Issues have been fixed! XXX APPROVED XXX
New Package SCM Request ======================= Package Name: gnumed-server Short Description: gnumed server component Owners: susmit Branches: f14 f15 InitialCC:
Git done (by process-git-requests).
gnumed-server-15.5-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/gnumed-server-15.5-2.fc14
gnumed-server-15.5-2.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/gnumed-server-15.5-2.fc15
Closing as NEXTRELEASE. Thanks everyone.
gnumed-server-15.5-2.fc14 has been pushed to the Fedora 14 stable repository.
gnumed-server-15.5-2.fc15 has been pushed to the Fedora 15 stable repository.