Bug 669146 (gnumed-server) - Review Request: gnumed-server - medical practice management - server
Summary: Review Request: gnumed-server - medical practice management - server
Keywords:
Status: CLOSED ERRATA
Alias: gnumed-server
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ankur Sinha (FranciscoD)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-medical
TreeView+ depends on / blocked
 
Reported: 2011-01-12 19:02 UTC by Susmit
Modified: 2011-06-21 17:17 UTC (History)
4 users (show)

Fixed In Version: gnumed-server-15.5-2.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-06-07 07:47:54 UTC
Type: ---
sanjay.ankur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Susmit 2011-01-12 19:02:23 UTC
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.

Comment 2 Jason Tibbitts 2011-02-05 00:49:38 UTC
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.

Comment 3 Susmit 2011-02-09 14:36:21 UTC
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.

Comment 4 Susmit 2011-02-28 02:50:46 UTC
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

Comment 5 Susmit 2011-02-28 02:54:03 UTC
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.

Comment 7 Ankur Sinha (FranciscoD) 2011-03-20 04:16:30 UTC
I'll take this one :)

Comment 8 Ankur Sinha (FranciscoD) 2011-03-23 07:55:33 UTC
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

Comment 9 Ankur Sinha (FranciscoD) 2011-03-23 07:57:02 UTC
Oh, and please check the changelog format. I don't think the %dist tag is supposed to be in it.

Comment 10 Ankur Sinha (FranciscoD) 2011-03-23 08:22:12 UTC
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

Comment 11 Susmit 2011-05-28 07:58:07 UTC
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.

Comment 12 Ankur Sinha (FranciscoD) 2011-05-28 09:41:44 UTC
(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

Comment 13 Susmit 2011-05-28 17:05:26 UTC
> 
> > 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.

Comment 14 Susmit 2011-06-01 04:55:34 UTC
ping.

Comment 15 Ankur Sinha (FranciscoD) 2011-06-02 05:00:18 UTC
http://koji.fedoraproject.org/koji/taskinfo?taskID=3103205

All Issues have been fixed!

XXX APPROVED XXX

Comment 16 Susmit 2011-06-02 05:52:50 UTC
New Package SCM Request
=======================
Package Name: gnumed-server
Short Description: gnumed server component
Owners: susmit
Branches: f14 f15
InitialCC:

Comment 17 Gwyn Ciesla 2011-06-02 12:03:11 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2011-06-07 07:34:49 UTC
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

Comment 19 Fedora Update System 2011-06-07 07:37:04 UTC
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

Comment 20 Susmit 2011-06-07 07:47:54 UTC
Closing as NEXTRELEASE.
Thanks everyone.

Comment 21 Fedora Update System 2011-06-21 17:11:59 UTC
gnumed-server-15.5-2.fc14 has been pushed to the Fedora 14 stable repository.

Comment 22 Fedora Update System 2011-06-21 17:17:53 UTC
gnumed-server-15.5-2.fc15 has been pushed to the Fedora 15 stable repository.


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